From 006d896f4376dbbbff0a958d5fe8639ab0c070d5 Mon Sep 17 00:00:00 2001 From: schwarze <> Date: Thu, 23 Jul 2020 17:15:35 +0000 Subject: Fix a bug in PEM_X509_INFO_read_bio(3) that is very likely to cause use-after-free and double-free issues in calling programs. The bug was introduced in SSLeay-0.6.0 released on June 21, 1996 and has been present since OpenBSD 2.4. I found the bug while documenting the function. The bug could bite in two ways that looked quite different from the perspective of the calling code: * If a stack was passed in that already contained some X509_INFO objects and an error occurred, all the objects passed in would be freed, but without removing the freed pointers from the stack, so the calling code would probable continue to access the freed pointers and eventually free them a second time. * If the input BIO contained at least two valid PEM objects followed by at least one PEM object causing an error, at least one freed pointer would be put onto the stack, even though the function would return NULL rather than the stack. But the calling code would still have a pointer to the stack, so it would be likely to access the new bogus pointers sooner or later. Fix all this by remembering the size of the input stack on entry and cutting it back to exactly that size when exiting due to an error, but no further. While here, do some related cleanup: * Garbage collect the automatic variables "error" and "i" which were only used at one single place each. * Use NULL rather than 0 for pointers. I like bugfixes that make the code four lines shorter, reduce the number of variables by one, reduce the number of brace-blocks by one, reduce the number if if-statements by one, and reduce the number of else-clauses by one. Tweaks and OK tb@. --- src/lib/libcrypto/pem/pem_info.c | 38 +++--- src/regress/lib/libcrypto/Makefile | 3 +- src/regress/lib/libcrypto/pem/Makefile | 8 ++ src/regress/lib/libcrypto/pem/x509_info.c | 184 ++++++++++++++++++++++++++++++ 4 files changed, 211 insertions(+), 22 deletions(-) create mode 100644 src/regress/lib/libcrypto/pem/Makefile create mode 100644 src/regress/lib/libcrypto/pem/x509_info.c (limited to 'src') diff --git a/src/lib/libcrypto/pem/pem_info.c b/src/lib/libcrypto/pem/pem_info.c index f02aaa8bb4..9561b5f4df 100644 --- a/src/lib/libcrypto/pem/pem_info.c +++ b/src/lib/libcrypto/pem/pem_info.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pem_info.c,v 1.22 2017/01/29 17:49:23 beck Exp $ */ +/* $OpenBSD: pem_info.c,v 1.23 2020/07/23 17:15:35 schwarze Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -101,29 +101,28 @@ PEM_X509_INFO_read_bio(BIO *bp, STACK_OF(X509_INFO) *sk, pem_password_cb *cb, void *pp; unsigned char *data = NULL; const unsigned char *p; - long len, error = 0; + long len; int ok = 0; - STACK_OF(X509_INFO) *ret = NULL; - unsigned int i, raw, ptype; - d2i_of_void *d2i = 0; + int num_in, ptype, raw; + STACK_OF(X509_INFO) *ret = sk; + d2i_of_void *d2i = NULL; - if (sk == NULL) { + if (ret == NULL) { if ((ret = sk_X509_INFO_new_null()) == NULL) { PEMerror(ERR_R_MALLOC_FAILURE); - return 0; + return NULL; } - } else - ret = sk; + } + num_in = sk_X509_INFO_num(ret); if ((xi = X509_INFO_new()) == NULL) goto err; for (;;) { raw = 0; ptype = 0; - i = PEM_read_bio(bp, &name, &header, &data, &len); - if (i == 0) { - error = ERR_GET_REASON(ERR_peek_last_error()); - if (error == PEM_R_NO_START_LINE) { + if (!PEM_read_bio(bp, &name, &header, &data, &len)) { + if (ERR_GET_REASON(ERR_peek_last_error()) == + PEM_R_NO_START_LINE) { ERR_clear_error(); break; } @@ -286,22 +285,19 @@ start: ok = 1; err: - if (xi != NULL) - X509_INFO_free(xi); if (!ok) { - for (i = 0; ((int)i) < sk_X509_INFO_num(ret); i++) { - xi = sk_X509_INFO_value(ret, i); - X509_INFO_free(xi); - } + while (sk_X509_INFO_num(ret) > num_in) + X509_INFO_free(sk_X509_INFO_pop(ret)); if (ret != sk) sk_X509_INFO_free(ret); ret = NULL; } - + X509_INFO_free(xi); free(name); free(header); free(data); - return (ret); + + return ret; } diff --git a/src/regress/lib/libcrypto/Makefile b/src/regress/lib/libcrypto/Makefile index a5b4cc3855..8d6d40d62e 100644 --- a/src/regress/lib/libcrypto/Makefile +++ b/src/regress/lib/libcrypto/Makefile @@ -1,4 +1,4 @@ -# $OpenBSD: Makefile,v 1.38 2020/07/14 18:33:34 jsing Exp $ +# $OpenBSD: Makefile,v 1.39 2020/07/23 17:15:35 schwarze Exp $ SUBDIR += aead SUBDIR += aeswrap @@ -33,6 +33,7 @@ SUBDIR += init SUBDIR += md4 SUBDIR += md5 SUBDIR += pbkdf2 +SUBDIR += pem SUBDIR += pkcs7 SUBDIR += poly1305 SUBDIR += rand diff --git a/src/regress/lib/libcrypto/pem/Makefile b/src/regress/lib/libcrypto/pem/Makefile new file mode 100644 index 0000000000..34561e217f --- /dev/null +++ b/src/regress/lib/libcrypto/pem/Makefile @@ -0,0 +1,8 @@ +# $OpenBSD: Makefile,v 1.1 2020/07/23 17:15:35 schwarze Exp $ + +PROG = x509_info +LDADD = -lcrypto +WARNINGS = Yes +CFLAGS += -Werror + +.include diff --git a/src/regress/lib/libcrypto/pem/x509_info.c b/src/regress/lib/libcrypto/pem/x509_info.c new file mode 100644 index 0000000000..e6b5388a71 --- /dev/null +++ b/src/regress/lib/libcrypto/pem/x509_info.c @@ -0,0 +1,184 @@ +/* $OpenBSD: x509_info.c,v 1.1 2020/07/23 17:15:35 schwarze Exp $ */ +/* + * Copyright (c) 2020 Ingo Schwarze + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include +#include + +#include +#include +#include +#include + +static const char *const bogus_pem = "\ +-----BEGIN BOGUS----- \n\ +-----END BOGUS----- \n\ +"; + +static const char *const cert_pem = "\ +-----BEGIN CERTIFICATE----- \n\ +MIIDpTCCAo2gAwIBAgIJAPYm3GvOr5eTMA0GCSqGSIb3DQEBBQUAMHAxCzAJBgNV \n\ +BAYTAlVLMRYwFAYDVQQKDA1PcGVuU1NMIEdyb3VwMSIwIAYDVQQLDBlGT1IgVEVT \n\ +VElORyBQVVJQT1NFUyBPTkxZMSUwIwYDVQQDDBxPcGVuU1NMIFRlc3QgSW50ZXJt \n\ +ZWRpYXRlIENBMB4XDTE0MDUyNDE0NDUxMVoXDTI0MDQwMTE0NDUxMVowZDELMAkG \n\ +A1UEBhMCVUsxFjAUBgNVBAoMDU9wZW5TU0wgR3JvdXAxIjAgBgNVBAsMGUZPUiBU \n\ +RVNUSU5HIFBVUlBPU0VTIE9OTFkxGTAXBgNVBAMMEFRlc3QgQ2xpZW50IENlcnQw \n\ +ggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC0ranbHRLcLVqN+0BzcZpY \n\ ++yOLqxzDWT1LD9eW1stC4NzXX9/DCtSIVyN7YIHdGLrIPr64IDdXXaMRzgZ2rOKs \n\ +lmHCAiFpO/ja99gGCJRxH0xwQatqAULfJVHeUhs7OEGOZc2nWifjqKvGfNTilP7D \n\ +nwi69ipQFq9oS19FmhwVHk2wg7KZGHI1qDyG04UrfCZMRitvS9+UVhPpIPjuiBi2 \n\ +x3/FZIpL5gXJvvFK6xHY63oq2asyzBATntBgnP4qJFWWcvRx24wF1PnZabxuVoL2 \n\ +bPnQ/KvONDrw3IdqkKhYNTul7jEcu3OlcZIMw+7DiaKJLAzKb/bBF5gm/pwW6As9 \n\ +AgMBAAGjTjBMMAwGA1UdEwEB/wQCMAAwDgYDVR0PAQH/BAQDAgXgMCwGCWCGSAGG \n\ ++EIBDQQfFh1PcGVuU1NMIEdlbmVyYXRlZCBDZXJ0aWZpY2F0ZTANBgkqhkiG9w0B \n\ +AQUFAAOCAQEAJzA4KTjkjXGSC4He63yX9Br0DneGBzjAwc1H6f72uqnCs8m7jgkE \n\ +PQJFdTzQUKh97QPUuayZ2gl8XHagg+iWGy60Kw37gQ0+lumCN2sllvifhHU9R03H \n\ +bWtS4kue+yQjMbrzf3zWygMDgwvFOUAIgBpH9qGc+CdNu97INTYd0Mvz51vLlxRn \n\ +sC5aBYCWaZFnw3lWYxf9eVFRy9U+DkYFqX0LpmbDtcKP7AZGE6ZwSzaim+Cnoz1u \n\ +Cgn+QmpFXgJKMFIZ82iSZISn+JkCCGxctZX1lMvai4Wi8Y0HxW9FTFZ6KBNwwE4B \n\ +zjbN/ehBkgLlW/DWfi44DvwUHmuU6QP3cw== \n\ +-----END CERTIFICATE----- \n\ +"; + +int +main(void) +{ + BIO *bp; + STACK_OF(X509_INFO) *skin, *skout; + X509_INFO *info0, *info1; + const char *errdata; + unsigned long errcode; + int errcount, errflags, num; + + errcount = 0; + if ((skin = sk_X509_INFO_new_null()) == NULL) + err(1, "sk_X509_INFO_new_null"); + + /* Test with empty input. */ + + if ((bp = BIO_new_mem_buf("", 0)) == NULL) + err(1, "BIO_new_mem_buf(empty)"); + if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) == NULL) + err(1, "empty input: %s", + ERR_error_string(ERR_get_error(), NULL)); + if (skout != skin) + errx(1, "empty input did not return the same stack"); + skout = NULL; + if ((num = sk_X509_INFO_num(skin)) != 0) + errx(1, "empty input created %d X509_INFO objects", num); + BIO_free(bp); + + /* Test with bogus input. */ + + if ((bp = BIO_new_mem_buf(bogus_pem, strlen(bogus_pem))) == NULL) + err(1, "BIO_new_mem_buf(bogus_pem)"); + if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) != NULL) + errx(1, "success with bogus input on first try"); + if ((num = sk_X509_INFO_num(skin)) != 0) + errx(1, "bogus input created %d X509_INFO objects", num); + if (BIO_reset(bp) != 1) + errx(1, "BIO_reset"); + + /* Populate stack and test again with bogus input. */ + + if ((info0 = X509_INFO_new()) == NULL) + err(1, "X509_INFO_new"); + info0->references = 2; /* X509_INFO_up_ref(3) doesn't exist. */ + if (sk_X509_INFO_push(skin, info0) != 1) + err(1, "sk_X509_INFO_push"); + if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) != NULL) + errx(1, "success with bogus input on second try"); + if ((num = sk_X509_INFO_num(skin)) != 1) + errx(1, "bogus input changed stack size from 1 to %d", num); + if (sk_X509_INFO_value(skin, 0) != info0) + errx(1, "bogus input changed stack content"); + if (info0->references != 2) { + warnx("bogus input changed ref count from 2 to %d", + info0->references); + info0->references = 2; + errcount++; + } + BIO_free(bp); + + /* Use a real certificate object. */ + + if ((bp = BIO_new_mem_buf(cert_pem, strlen(cert_pem))) == NULL) + err(1, "BIO_new_mem_buf(cert_pem)"); + if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) == NULL) { + errdata = NULL; + errflags = 0; + while ((errcode = ERR_get_error_line_data(NULL, NULL, + &errdata, &errflags)) != 0) + if (errdata != NULL && (errflags & ERR_TXT_STRING)) + warnx("%s --- %s", + ERR_error_string(errcode, NULL), + errdata); + else + warnx("%s", ERR_error_string(errcode, NULL)); + err(1, "real input: parsing failed"); + } + if (skout != skin) + errx(1, "real input did not return the same stack"); + skout = NULL; + if ((num = sk_X509_INFO_num(skin)) != 2) + errx(1, "real input changed stack size from 1 to %d", num); + if (sk_X509_INFO_value(skin, 0) != info0) + errx(1, "real input changed stack content"); + if (info0->references != 2) + errx(1, "real input changed ref count from 2 to %d", + info0->references); + info1 = sk_X509_INFO_pop(skin); + if (info1->x509 == NULL) + errx(1, "real input did not create a certificate"); + X509_INFO_free(info1); + info1 = NULL; + BIO_free(bp); + + /* Two real certificates followed by bogus input. */ + + if ((bp = BIO_new(BIO_s_mem())) == NULL) + err(1, "BIO_new"); + if (BIO_puts(bp, cert_pem) != strlen(cert_pem)) + err(1, "BIO_puts(cert_pem) first copy"); + if (BIO_puts(bp, cert_pem) != strlen(cert_pem)) + err(1, "BIO_puts(cert_pem) second copy"); + if (BIO_puts(bp, bogus_pem) != strlen(bogus_pem)) + err(1, "BIO_puts(bogus_pem)"); + if ((skout = PEM_X509_INFO_read_bio(bp, skin, NULL, NULL)) != NULL) + errx(1, "success with real + bogus input"); + if ((num = sk_X509_INFO_num(skin)) != 1) { + warnx("real + bogus input changed stack size from 1 to %d", + num); + while (sk_X509_INFO_num(skin) > 1) + sk_X509_INFO_pop(skin); + errcount++; + } + if (sk_X509_INFO_value(skin, 0) != info0) + errx(1, "real + bogus input changed stack content"); + if (info0->references != 2) { + warnx("real + bogus input changed ref count from 2 to %d", + info0->references); + errcount++; + } + BIO_free(bp); + info0->references = 1; + X509_INFO_free(info0); + sk_X509_INFO_free(skin); + + if (errcount > 0) + errx(1, "%d errors detected", errcount); + return 0; +} -- cgit v1.2.3-55-g6feb