From f22d7684aed13a9ae9ea6554b7a3e52fdfa4f193 Mon Sep 17 00:00:00 2001 From: tb <> Date: Tue, 8 Dec 2020 15:06:42 +0000 Subject: [PATCH] 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 [ mikhailnov: ported to v3.2.0, changed src/lib/libcrypto/x509/x509_genn.c to src/lib/libcrypto/x509v3/v3_genn.c ] --- src/lib/libcrypto/asn1/asn1.h | 3 +- src/lib/libcrypto/asn1/asn1_err.c | 3 +- src/lib/libcrypto/asn1/asn1_lib.c | 4 ++- src/lib/libcrypto/asn1/tasn_dec.c | 22 ++++++++++++- src/lib/libcrypto/asn1/tasn_enc.c | 21 +++++++++++- src/lib/libcrypto/x509v3/v3_genn.c | 52 ++++++++++++++++++++++++++---- 6 files changed, 94 insertions(+), 11 deletions(-) diff --git a/src/lib/libcrypto/asn1/asn1.h b/src/lib/libcrypto/asn1/asn1.h index 0a8da415fb..76c294ada8 100644 --- a/src/lib/libcrypto/asn1/asn1.h +++ b/src/lib/libcrypto/asn1/asn1.h @@ -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/src/lib/libcrypto/asn1/asn1_err.c b/src/lib/libcrypto/asn1/asn1_err.c index 5cc355084f..e2c56deb5b 100644 --- a/src/lib/libcrypto/asn1/asn1_err.c +++ b/src/lib/libcrypto/asn1/asn1_err.c @@ -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/src/lib/libcrypto/asn1/asn1_lib.c b/src/lib/libcrypto/asn1/asn1_lib.c index 5dc520c428..d760cccd4d 100644 --- a/src/lib/libcrypto/asn1/asn1_lib.c +++ b/src/lib/libcrypto/asn1/asn1_lib.c @@ -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/src/lib/libcrypto/asn1/tasn_dec.c b/src/lib/libcrypto/asn1/tasn_dec.c index 70dc355ca1..4b08e90404 100644 --- a/src/lib/libcrypto/asn1/tasn_dec.c +++ b/src/lib/libcrypto/asn1/tasn_dec.c @@ -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/src/lib/libcrypto/asn1/tasn_enc.c b/src/lib/libcrypto/asn1/tasn_enc.c index d103c4d096..5d95f035ef 100644 --- a/src/lib/libcrypto/asn1/tasn_enc.c +++ b/src/lib/libcrypto/asn1/tasn_enc.c @@ -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/src/lib/libcrypto/x509v3/v3_genn.c b/src/lib/libcrypto/x509v3/v3_genn.c index 848006acf4..dadf6f1e40 100644 --- a/src/lib/libcrypto/x509v3/v3_genn.c +++ b/src/lib/libcrypto/x509v3/v3_genn.c @@ -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;