From 8b2f57152e74fd65c748fae4903308ea5e753ed8 Mon Sep 17 00:00:00 2001 From: Theo Buehler Date: Tue, 8 Dec 2020 15:06:43 +0000 Subject: Fix a NULL dereference in GENERAL_NAME_cmp() Comparing two GENERAL_NAME structures containing an EDIPARTYNAME can lead to a crash. This enables a denial of service attack for an attacker who can control both sides of the comparison. Issue reported to OpenSSL on Nov 9 by David Benjamin. OpenSSL shared the information with us on Dec 1st. Fix from Matt Caswell (OpenSSL) with a few small tweaks. ok jsing --- lib/libcrypto/asn1/asn1.h | 3 ++- lib/libcrypto/asn1/asn1_err.c | 3 ++- lib/libcrypto/asn1/asn1_lib.c | 4 +++- lib/libcrypto/asn1/tasn_dec.c | 22 +++++++++++++++++- lib/libcrypto/asn1/tasn_enc.c | 21 ++++++++++++++++- lib/libcrypto/x509/x509_genn.c | 52 +++++++++++++++++++++++++++++++++++++----- 6 files changed, 94 insertions(+), 11 deletions(-) (limited to 'lib/libcrypto') diff --git a/lib/libcrypto/asn1/asn1.h b/lib/libcrypto/asn1/asn1.h index 0a8da415fb4..76c294ada8c 100644 --- a/lib/libcrypto/asn1/asn1.h +++ b/lib/libcrypto/asn1/asn1.h @@ -1,4 +1,4 @@ -/* $OpenBSD: asn1.h,v 1.53 2018/11/30 04:51:19 jeremy Exp $ */ +/* $OpenBSD: asn1.h,v 1.54 2020/12/08 15:06:42 tb Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1137,6 +1137,7 @@ void ERR_load_ASN1_strings(void); #define ASN1_R_BAD_OBJECT_HEADER 102 #define ASN1_R_BAD_PASSWORD_READ 103 #define ASN1_R_BAD_TAG 104 +#define ASN1_R_BAD_TEMPLATE 230 #define ASN1_R_BMPSTRING_IS_WRONG_LENGTH 214 #define ASN1_R_BN_LIB 105 #define ASN1_R_BOOLEAN_IS_WRONG_LENGTH 106 diff --git a/lib/libcrypto/asn1/asn1_err.c b/lib/libcrypto/asn1/asn1_err.c index 5cc355084f0..e2c56deb5ba 100644 --- a/lib/libcrypto/asn1/asn1_err.c +++ b/lib/libcrypto/asn1/asn1_err.c @@ -1,4 +1,4 @@ -/* $OpenBSD: asn1_err.c,v 1.21 2018/03/29 02:29:24 inoguchi Exp $ */ +/* $OpenBSD: asn1_err.c,v 1.22 2020/12/08 15:06:42 tb Exp $ */ /* ==================================================================== * Copyright (c) 1999-2011 The OpenSSL Project. All rights reserved. * @@ -85,6 +85,7 @@ static ERR_STRING_DATA ASN1_str_reasons[] = { {ERR_REASON(ASN1_R_BAD_OBJECT_HEADER) , "bad object header"}, {ERR_REASON(ASN1_R_BAD_PASSWORD_READ) , "bad password read"}, {ERR_REASON(ASN1_R_BAD_TAG) , "bad tag"}, + {ERR_REASON(ASN1_R_BAD_TEMPLATE) , "bad template"}, {ERR_REASON(ASN1_R_BMPSTRING_IS_WRONG_LENGTH), "bmpstring is wrong length"}, {ERR_REASON(ASN1_R_BN_LIB) , "bn lib"}, {ERR_REASON(ASN1_R_BOOLEAN_IS_WRONG_LENGTH), "boolean is wrong length"}, diff --git a/lib/libcrypto/asn1/asn1_lib.c b/lib/libcrypto/asn1/asn1_lib.c index 5dc520c428e..d760cccd4d9 100644 --- a/lib/libcrypto/asn1/asn1_lib.c +++ b/lib/libcrypto/asn1/asn1_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: asn1_lib.c,v 1.44 2018/11/17 09:34:11 tb Exp $ */ +/* $OpenBSD: asn1_lib.c,v 1.45 2020/12/08 15:06:42 tb Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -388,6 +388,8 @@ ASN1_STRING_cmp(const ASN1_STRING *a, const ASN1_STRING *b) { int i; + if (a == NULL || b == NULL) + return -1; i = (a->length - b->length); if (i == 0) { i = memcmp(a->data, b->data, a->length); diff --git a/lib/libcrypto/asn1/tasn_dec.c b/lib/libcrypto/asn1/tasn_dec.c index 70dc355ca1f..4b08e90404a 100644 --- a/lib/libcrypto/asn1/tasn_dec.c +++ b/lib/libcrypto/asn1/tasn_dec.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tasn_dec.c,v 1.37 2019/04/01 15:48:04 jsing Exp $ */ +/* $OpenBSD: tasn_dec.c,v 1.38 2020/12/08 15:06:42 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2000. */ @@ -210,6 +210,16 @@ asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, break; case ASN1_ITYPE_MSTRING: + /* + * It never makes sense for multi-strings to have implicit + * tagging, so if tag != -1, then this looks like an error in + * the template. + */ + if (tag != -1) { + ASN1error(ASN1_R_BAD_TEMPLATE); + goto err; + } + p = *in; /* Just read in tag and class */ ret = asn1_check_tlen(NULL, &otag, &oclass, NULL, NULL, @@ -245,6 +255,16 @@ asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in, long len, it, tag, aclass, opt, ctx); case ASN1_ITYPE_CHOICE: + /* + * It never makes sense for CHOICE types to have implicit + * tagging, so if tag != -1, then this looks like an error in + * the template. + */ + if (tag != -1) { + ASN1error(ASN1_R_BAD_TEMPLATE); + goto err; + } + if (asn1_cb && !asn1_cb(ASN1_OP_D2I_PRE, pval, it, NULL)) goto auxerr; diff --git a/lib/libcrypto/asn1/tasn_enc.c b/lib/libcrypto/asn1/tasn_enc.c index d103c4d096a..5d95f035ef2 100644 --- a/lib/libcrypto/asn1/tasn_enc.c +++ b/lib/libcrypto/asn1/tasn_enc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tasn_enc.c,v 1.22 2019/04/01 15:48:04 jsing Exp $ */ +/* $OpenBSD: tasn_enc.c,v 1.23 2020/12/08 15:06:42 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 2000. */ @@ -61,6 +61,7 @@ #include #include +#include #include static int asn1_i2d_ex_primitive(ASN1_VALUE **pval, unsigned char **out, @@ -152,9 +153,27 @@ ASN1_item_ex_i2d(ASN1_VALUE **pval, unsigned char **out, const ASN1_ITEM *it, break; case ASN1_ITYPE_MSTRING: + /* + * It never makes sense for multi-strings to have implicit + * tagging, so if tag != -1, then this looks like an error in + * the template. + */ + if (tag != -1) { + ASN1error(ASN1_R_BAD_TEMPLATE); + return 0; + } return asn1_i2d_ex_primitive(pval, out, it, -1, aclass); case ASN1_ITYPE_CHOICE: + /* + * It never makes sense for CHOICE types to have implicit + * tagging, so if tag != -1, then this looks like an error in + * the template. + */ + if (tag != -1) { + ASN1error(ASN1_R_BAD_TEMPLATE); + return 0; + } if (asn1_cb && !asn1_cb(ASN1_OP_I2D_PRE, pval, it, NULL)) return 0; i = asn1_get_choice_selector(pval, it); diff --git a/lib/libcrypto/x509/x509_genn.c b/lib/libcrypto/x509/x509_genn.c index 848006acf40..dadf6f1e403 100644 --- a/lib/libcrypto/x509/x509_genn.c +++ b/lib/libcrypto/x509/x509_genn.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509_genn.c,v 1.1 2020/06/04 15:19:31 jsing Exp $ */ +/* $OpenBSD: x509_genn.c,v 1.2 2020/12/08 15:06:42 tb Exp $ */ /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL * project 1999. */ @@ -117,16 +117,17 @@ OTHERNAME_free(OTHERNAME *a) ASN1_item_free((ASN1_VALUE *)a, &OTHERNAME_it); } +/* Uses explicit tagging since DIRECTORYSTRING is a CHOICE type */ static const ASN1_TEMPLATE EDIPARTYNAME_seq_tt[] = { { - .flags = ASN1_TFLG_IMPLICIT | ASN1_TFLG_OPTIONAL, + .flags = ASN1_TFLG_EXPLICIT | ASN1_TFLG_OPTIONAL, .tag = 0, .offset = offsetof(EDIPARTYNAME, nameAssigner), .field_name = "nameAssigner", .item = &DIRECTORYSTRING_it, }, { - .flags = ASN1_TFLG_IMPLICIT | ASN1_TFLG_OPTIONAL, + .flags = ASN1_TFLG_EXPLICIT, .tag = 1, .offset = offsetof(EDIPARTYNAME, partyName), .field_name = "partyName", @@ -324,6 +325,37 @@ GENERAL_NAME_dup(GENERAL_NAME *a) return ASN1_item_dup(&GENERAL_NAME_it, a); } +static int +EDIPARTYNAME_cmp(const EDIPARTYNAME *a, const EDIPARTYNAME *b) +{ + int res; + + /* + * Shouldn't be possible in a valid GENERAL_NAME, but we handle it + * anyway. OTHERNAME_cmp treats NULL != NULL, so we do the same here. + */ + if (a == NULL || b == NULL) + return -1; + if (a->nameAssigner == NULL && b->nameAssigner != NULL) + return -1; + if (a->nameAssigner != NULL && b->nameAssigner == NULL) + return 1; + /* If we get here, both have nameAssigner set or both unset. */ + if (a->nameAssigner != NULL) { + res = ASN1_STRING_cmp(a->nameAssigner, b->nameAssigner); + if (res != 0) + return res; + } + /* + * partyName is required, so these should never be NULL. We treat it in + * the same way as the a == NULL || b == NULL case above. + */ + if (a->partyName == NULL || b->partyName == NULL) + return -1; + + return ASN1_STRING_cmp(a->partyName, b->partyName); +} + /* Returns 0 if they are equal, != 0 otherwise. */ int GENERAL_NAME_cmp(GENERAL_NAME *a, GENERAL_NAME *b) @@ -334,8 +366,11 @@ GENERAL_NAME_cmp(GENERAL_NAME *a, GENERAL_NAME *b) return -1; switch (a->type) { case GEN_X400: + result = ASN1_TYPE_cmp(a->d.x400Address, b->d.x400Address); + break; + case GEN_EDIPARTY: - result = ASN1_TYPE_cmp(a->d.other, b->d.other); + result = EDIPARTYNAME_cmp(a->d.ediPartyName, b->d.ediPartyName); break; case GEN_OTHERNAME: @@ -384,8 +419,11 @@ GENERAL_NAME_set0_value(GENERAL_NAME *a, int type, void *value) { switch (type) { case GEN_X400: + a->d.x400Address = value; + break; + case GEN_EDIPARTY: - a->d.other = value; + a->d.ediPartyName = value; break; case GEN_OTHERNAME: @@ -420,8 +458,10 @@ GENERAL_NAME_get0_value(GENERAL_NAME *a, int *ptype) *ptype = a->type; switch (a->type) { case GEN_X400: + return a->d.x400Address; + case GEN_EDIPARTY: - return a->d.other; + return a->d.ediPartyName; case GEN_OTHERNAME: return a->d.otherName; -- cgit v1.2.3