From 9b790dc7bd2fca0d59a0ba1870e0a50f867b36fb Mon Sep 17 00:00:00 2001 From: tb Date: Sat, 11 Nov 2023 09:35:21 +0000 Subject: [PATCH] Fix a few bugs in X509v3_asid_add*() These 'builder' functions, usually used together, can result in corrupt ASIdentifiers on failure. In general, no caller should ever try to recover from OpenSSL API failure. There are simply too many traps. We can still make an effort to leave the objects in unmodified state on failure. This is tricky because ownership transfer happens. Unfortunately a really clean version of this seems impossible, maybe a future iteration will bring improvements... The nasty bit here is that the caller of X509v3_asid_add_id_or_range() can't know from the return value whether ownership of min and max was transferred or not. An inspection of (*choice)->u.range is required. If a caller frees min and max after sk_ASIdOrRange_push() failed, there is a double free. All these complications could have been avoided if the API interface had simply used uint32_t instead of ASN1_INTEGERs. The entire RFC 3779 API was clearly written without proper review. I don't know if there ever was an actual consumer before rpki-client. If it existed, nobody with the requisite skill set looked at it in depth. ok beck for the general direction with a lot of input and ok jsing --- lib/libcrypto/x509/x509_asid.c | 134 +++++++++++++++++++++++---------- 1 file changed, 96 insertions(+), 38 deletions(-) diff --git a/lib/libcrypto/x509/x509_asid.c b/lib/libcrypto/x509/x509_asid.c index 95b1acb1e4a..ecd35b1f1d8 100644 --- a/lib/libcrypto/x509/x509_asid.c +++ b/lib/libcrypto/x509/x509_asid.c @@ -1,4 +1,4 @@ -/* $OpenBSD: x509_asid.c,v 1.40 2023/04/19 12:30:09 jsg Exp $ */ +/* $OpenBSD: x509_asid.c,v 1.41 2023/11/11 09:35:21 tb Exp $ */ /* * Contributed to the OpenSSL Project by the American Registry for * Internet Numbers ("ARIN"). @@ -406,8 +406,12 @@ int X509v3_asid_add_inherit(ASIdentifiers *asid, int which) { ASIdentifierChoice **choice; + ASIdentifierChoice *aic = NULL; + int ret = 0; + if (asid == NULL) - return 0; + goto err; + switch (which) { case V3_ASID_ASNUM: choice = &asid->asnum; @@ -416,19 +420,76 @@ X509v3_asid_add_inherit(ASIdentifiers *asid, int which) choice = &asid->rdi; break; default: - return 0; + goto err; } - if (*choice == NULL) { - if ((*choice = ASIdentifierChoice_new()) == NULL) - return 0; - if (((*choice)->u.inherit = ASN1_NULL_new()) == NULL) - return 0; - (*choice)->type = ASIdentifierChoice_inherit; + + if (*choice != NULL) { + if ((*choice)->type != ASIdentifierChoice_inherit) + goto err; + } else { + if ((aic = ASIdentifierChoice_new()) == NULL) + goto err; + if ((aic->u.inherit = ASN1_NULL_new()) == NULL) + goto err; + aic->type = ASIdentifierChoice_inherit; + + *choice = aic; + aic = NULL; } - return (*choice)->type == ASIdentifierChoice_inherit; + + ret = 1; + + err: + ASIdentifierChoice_free(aic); + + return ret; } LCRYPTO_ALIAS(X509v3_asid_add_inherit); +static int +ASIdOrRanges_add_id_or_range(ASIdOrRanges *aors, ASN1_INTEGER *min, + ASN1_INTEGER *max) +{ + ASIdOrRange *aor = NULL; + ASRange *asr = NULL; + int ret = 0; + + /* Preallocate since we must not fail after sk_ASIdOrRange_push(). */ + if (max != NULL) { + if ((asr = ASRange_new()) == NULL) + goto err; + } + + if ((aor = ASIdOrRange_new()) == NULL) + goto err; + if (sk_ASIdOrRange_push(aors, aor) <= 0) + goto err; + + if (max == NULL) { + aor->type = ASIdOrRange_id; + aor->u.id = min; + } else { + ASN1_INTEGER_free(asr->min); + asr->min = min; + ASN1_INTEGER_free(asr->max); + asr->max = max; + + aor->type = ASIdOrRange_range; + aor->u.range = asr; + asr = NULL; + } + + aor = NULL; + + ret = 1; + + err: + ASIdOrRange_free(aor); + ASRange_free(asr); + + return ret; +} + /* * Add an ID or range to an ASIdentifierChoice. */ @@ -437,9 +498,12 @@ X509v3_asid_add_id_or_range(ASIdentifiers *asid, int which, ASN1_INTEGER *min, ASN1_INTEGER *max) { ASIdentifierChoice **choice; - ASIdOrRange *aor; + ASIdentifierChoice *aic = NULL, *new_aic = NULL; + int ret = 0; + if (asid == NULL) - return 0; + goto err; + switch (which) { case V3_ASID_ASNUM: choice = &asid->asnum; @@ -448,39 +512,33 @@ X509v3_asid_add_id_or_range(ASIdentifiers *asid, int which, ASN1_INTEGER *min, choice = &asid->rdi; break; default: - return 0; - } - if (*choice != NULL && (*choice)->type == ASIdentifierChoice_inherit) - return 0; - if (*choice == NULL) { - if ((*choice = ASIdentifierChoice_new()) == NULL) - return 0; - (*choice)->u.asIdsOrRanges = sk_ASIdOrRange_new(ASIdOrRange_cmp); - if ((*choice)->u.asIdsOrRanges == NULL) - return 0; - (*choice)->type = ASIdentifierChoice_asIdsOrRanges; + goto err; } - if ((aor = ASIdOrRange_new()) == NULL) - return 0; - if (max == NULL) { - aor->type = ASIdOrRange_id; - aor->u.id = min; + + if ((aic = *choice) != NULL) { + if (aic->type != ASIdentifierChoice_asIdsOrRanges) + goto err; } else { - aor->type = ASIdOrRange_range; - if ((aor->u.range = ASRange_new()) == NULL) + if ((aic = new_aic = ASIdentifierChoice_new()) == NULL) + goto err; + aic->u.asIdsOrRanges = sk_ASIdOrRange_new(ASIdOrRange_cmp); + if (aic->u.asIdsOrRanges == NULL) goto err; - ASN1_INTEGER_free(aor->u.range->min); - aor->u.range->min = min; - ASN1_INTEGER_free(aor->u.range->max); - aor->u.range->max = max; + aic->type = ASIdentifierChoice_asIdsOrRanges; } - if (!(sk_ASIdOrRange_push((*choice)->u.asIdsOrRanges, aor))) + + if (!ASIdOrRanges_add_id_or_range(aic->u.asIdsOrRanges, min, max)) goto err; - return 1; + + *choice = aic; + aic = new_aic = NULL; + + ret = 1; err: - ASIdOrRange_free(aor); - return 0; + ASIdentifierChoice_free(new_aic); + + return ret; } LCRYPTO_ALIAS(X509v3_asid_add_id_or_range); -- 2.20.1