From 04a4b9d9b4067f424283f0e5c80bcd9ea6aff6c0 Mon Sep 17 00:00:00 2001 From: tb <> Date: Sat, 11 Nov 2023 09:35:21 +0000 Subject: 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 --- src/lib/libcrypto/x509/x509_asid.c | 134 ++++++++++++++++++++++++++----------- 1 file changed, 96 insertions(+), 38 deletions(-) (limited to 'src/lib') diff --git a/src/lib/libcrypto/x509/x509_asid.c b/src/lib/libcrypto/x509/x509_asid.c index 95b1acb1e4..ecd35b1f1d 100644 --- a/src/lib/libcrypto/x509/x509_asid.c +++ b/src/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); -- cgit v1.2.3-55-g6feb