Potential use after free in callers of X509v3_asid_add_id_or_range()

[botovq]

The RFC 3779 code is undocumented, not very well designed and thus rather dangerous public OpenSSL API. It's full of traps and bugs. It should at least be warned against if not documented. Fortunately, almost no one uses it, but there is now an attempt at exposing bindings to Rust: sfackler/rust-openssl#2053 .

The only internal caller v2i_ASIdentifiers() of X509v3_asid_ad_id_or_range() allocates and passes one or two ASN1_INTEGERs and frees them on failure.

Towards the end of X509v3_asid_ad_id_or_range(), ownership of min and max is transferred to aor:

openssl/crypto/x509/v3_asid.c

Lines 218 to 221 in 9e75a0b

	ASN1_INTEGER_free(aor->u.range->min);
	aor->u.range->min = min;
	ASN1_INTEGER_free(aor->u.range->max);
	aor->u.range->max = max;

Subsequently, aor is pushed onto a part of asid. If the push fails, aor is freed and min and max with it.

openssl/crypto/x509/v3_asid.c

Lines 223 to 229 in 9e75a0b

	if (!(sk_ASIdOrRange_push((*choice)->u.asIdsOrRanges, aor)))
	goto err;
	return 1;
	err:
	ASIdOrRange_free(aor);
	return 0;

Here's the fix I landed earlier today: openbsd/src@9b790dc .

[bernd-edlinger]

how about this for a simple fix?

diff --git a/crypto/x509/v3_asid.c b/crypto/x509/v3_asid.c
index d1c3dd5d9f..30a62399bf 100644
--- a/crypto/x509/v3_asid.c
+++ b/crypto/x509/v3_asid.c
@@ -208,6 +208,8 @@ int X509v3_asid_add_id_or_range(ASIdentifiers *asid,
     }
     if ((aor = ASIdOrRange_new()) == NULL)
         return 0;
+    if (!sk_ASIdOrRange_reserve((*choice)->u.asIdsOrRanges, 1))
+        goto err;
     if (max == NULL) {
         aor->type = ASIdOrRange_id;
         aor->u.id = min;

maybe with a comment at the sk_ASIdOrRange_push below, that it cannot fail
due to the reservation?

[botovq]

On Mon, Nov 13, 2023 at 06:59:01PM -0800, Bernd Edlinger wrote: how about this for a simple fix?

Sure, that will fix part of the many bugs here, but you won't be able to backport this to 1.1 (if you care to do so).

[bernd-edlinger]

I will certainly back port that to my 1.1.1 feature branch:
https://github.com/bernd-edlinger/openssl/tree/openssl-111-features
I have lots of similar fixes in the queue.

[botovq]

the point is that this api can still leave asid in an incoherent state. you should not have an empty stack there, for example. incrementally populating asid is a bad idea to begin with.

…

On Tue, Nov 14, 2023, at 10:55 AM, Bernd Edlinger wrote: I will certainly back port that to my 1.1.1 feature branch: https://github.com/bernd-edlinger/openssl/tree/openssl-111-features I have lots of similar fixes in the queue. — Reply to this email directly, view it on GitHub <#22700 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACVVQA7IIXFEI5I3WLUBDA3YENEZVAVCNFSM6AAAAAA7HGRBCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBZHE4DCNJUGE>. You are receiving this because you authored the thread.Message ID: ***@***.***>