Commit 74b1b8909e for openssl.org
commit 74b1b8909e765e98c962a04b9fa73874b376ac0a
Author: Mounir IDRASSI <mounir.idrassi@idrix.fr>
Date: Sat Jul 4 15:19:10 2026 +0900
x509: keep RFC 3779 stacks hole-free after a failed canonize
ASIdentifierChoice_canonize and IPAddressOrRanges_canonize left NULL
slots in the live stack when a merge succeeded before a later failure
(overlap, inverted range, or OOM). The NULLs were closed up only on
the success path, so a failed X509v3_asid_canonize() /
X509v3_addr_canonize() left the object poisonable: a retry of either
function dereferences a NULL in the sort comparator during the re-sort,
the i2r extension printers dereference the hole when printing (both in
all builds), and on the asid side is_canonical trips extract_min_max's
ossl_assert(aor != NULL), an abort in debug builds.
This is a regression introduced by e59165a1e3 ("crypto/x509: replace
O(N^2) RFC 3779 canonicalisation merge with linear sweep"): the
previous delete-per-merge loop kept the stack hole-free at every step,
so a failed canonize always left the object sorted, partially merged
and structurally valid.
Run the compaction pass unconditionally, whether the sweep succeeded
or failed, restoring that contract: the stack is always left partially
canonicalized but hole-free, safe to inspect, print, encode, free, or
retry. In v3_addr.c this also routes the bare return-0 error paths
through the done: cleanup label.
New tests cover the crash vectors (is_canonical then retry) on both
the asid and addr sides. The existing *_error_midsweep tests are
strengthened with the same inspect/retry assertions.
Fixes #31856
Assisted-by: ZCode:GLM-5.2
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Bob Beck <beck@openssl.org>
MergeDate: Mon Jul 27 07:59:21 2026
(Merged from https://github.com/openssl/openssl/pull/31857)
diff --git a/crypto/x509/v3_addr.c b/crypto/x509/v3_addr.c
index e245e2b08a..73ad2345b9 100644
--- a/crypto/x509/v3_addr.c
+++ b/crypto/x509/v3_addr.c
@@ -828,39 +828,42 @@ int X509v3_addr_is_canonical(IPAddrBlocks *addr)
* After the initial sort, the merge runs as a single linear sweep
* over the list using a write index. Adjacent entries are folded
* into the previous output by replacing it with a freshly built
- * merged range; both old entries are then freed and the source slot
- * is left NULL so the asn1 free machinery does not double-free on a
- * subsequent abort. Total cost is O(N log N) sort + O(N) merge,
- * with no stack deletes inside the loop.
+ * merged range; both old entries are then freed and the consumed
+ * source slot is recorded as NULL. Total cost is O(N log N) sort +
+ * O(N) merge, with no stack deletes inside the loop.
+ *
+ * The NULL slots are closed up by a single compaction pass at the end,
+ * which runs whether the sweep succeeded or failed. This guarantees
+ * the live stack is always left hole-free, with every occupied slot
+ * non-NULL, so on error the caller may safely inspect, print, encode,
+ * free, or retry canonize on the object. The sort comparator
+ * dereferences every slot with no NULL guard, so a retry (which
+ * re-sorts) would crash on a hole. is_canonical tolerates NULL by
+ * returning non-canonical, but the object is still structurally
+ * invalid.
*/
static int IPAddressOrRanges_canonize(IPAddressOrRanges *aors,
const unsigned afi)
{
int length = length_from_afi(afi);
int read, write = 0, n;
+ int ret = 0;
sk_IPAddressOrRange_sort(aors);
n = sk_IPAddressOrRange_num(aors);
- /*
- * Error paths below all `return 0` directly. Slots at
- * [write..read-1] are NULL (from earlier iterations) and slots at
- * [read..n-1] still hold their original entries; the caller's
- * normal teardown walks the whole stack and frees each non-NULL
- * slot safely, so leaving the stack in this mixed state is sound.
- */
for (read = 0; read < n; read++) {
IPAddressOrRange *cur = sk_IPAddressOrRange_value(aors, read);
unsigned char c_min[ADDR_RAW_BUF_LEN], c_max[ADDR_RAW_BUF_LEN];
if (!extract_min_max(cur, c_min, c_max, length))
- return 0;
+ goto done;
/*
* Punt inverted range.
*/
if (memcmp(c_min, c_max, length) > 0)
- return 0;
+ goto done;
if (write > 0) {
IPAddressOrRange *prev = sk_IPAddressOrRange_value(aors,
@@ -870,13 +873,13 @@ static int IPAddressOrRanges_canonize(IPAddressOrRanges *aors,
int j;
if (!extract_min_max(prev, p_min, p_max, length))
- return 0;
+ goto done;
/*
* Reject overlap with the previous accepted entry.
*/
if (memcmp(p_max, c_min, length) >= 0)
- return 0;
+ goto done;
/*
* Adjacency test: does c_min - 1 equal p_max? Work on a
@@ -892,11 +895,12 @@ static int IPAddressOrRanges_canonize(IPAddressOrRanges *aors,
IPAddressOrRange *merged;
if (!make_addressRange(&merged, p_min, c_max, length))
- return 0;
+ goto done;
/*
* Replace prev with merged, free the originals, and
- * NULL the source slot so the stack does not retain a
- * second reference to cur.
+ * record the consumed source slot as NULL; the epilogue
+ * compacts NULLs out of the live stack so it is left
+ * hole-free whether we succeed or fail.
*/
(void)sk_IPAddressOrRange_set(aors, write - 1, merged);
IPAddressOrRange_free(prev);
@@ -918,13 +922,38 @@ static int IPAddressOrRanges_canonize(IPAddressOrRanges *aors,
write++;
}
+ ret = 1;
+
+done:
/*
- * Compaction succeeded: every slot at [write..n-1] is NULL, so
- * popping the tail leaves the canonicalised list at [0..write-1].
+ * The sweep above NULLs source slots as it folds entries. Whether
+ * we succeeded or bailed out on an error, the stack must be left
+ * hole-free, with every occupied slot non-NULL, so that the caller
+ * may inspect, print, encode, free, or even retry canonize on the
+ * object without dereferencing NULL. Slide every non-NULL slot
+ * forward to close the holes, then pop the vacated tail. On success
+ * this collapses [write..n-1] (all NULL) to length `write`; on error
+ * it removes the possibly empty contiguous run of NULL slots in
+ * [write, read), preserving the processed output in [0, write) and
+ * the untouched original entries in [read, n).
*/
- while (sk_IPAddressOrRange_num(aors) > write)
- (void)sk_IPAddressOrRange_pop(aors);
- return 1;
+ {
+ int w = 0, r;
+
+ for (r = 0; r < sk_IPAddressOrRange_num(aors); r++) {
+ IPAddressOrRange *v = sk_IPAddressOrRange_value(aors, r);
+
+ if (v != NULL) {
+ if (w != r)
+ (void)sk_IPAddressOrRange_set(aors, w, v);
+ w++;
+ }
+ }
+ while (sk_IPAddressOrRange_num(aors) > w)
+ (void)sk_IPAddressOrRange_pop(aors);
+ }
+
+ return ret;
}
/*
diff --git a/crypto/x509/v3_asid.c b/crypto/x509/v3_asid.c
index c00ded15f9..05aabcc9f6 100644
--- a/crypto/x509/v3_asid.c
+++ b/crypto/x509/v3_asid.c
@@ -351,10 +351,17 @@ int X509v3_asid_is_canonical(ASIdentifiers *asid)
* After the initial sort, the merge runs as a single linear sweep
* over the list using a write index. Each entry is examined once;
* adjacent / mergeable entries extend the previous output's upper
- * bound in O(1) and the source slot is left NULL so the asn1 free
- * machinery does not double-free on a subsequent abort. Total cost
- * is O(N log N) sort + O(N) merge, with no stack deletes inside the
- * loop.
+ * bound in O(1) and the consumed source slot is recorded as NULL.
+ * Total cost is O(N log N) sort + O(N) merge, with no stack deletes
+ * inside the loop.
+ *
+ * The NULL slots are closed up by a single compaction pass at the end,
+ * which runs whether the sweep succeeded or failed. This guarantees
+ * the live stack is always left hole-free, with every occupied slot
+ * non-NULL, so on error the caller may safely inspect, print, encode,
+ * free, or retry canonize on the object. The sort comparator
+ * dereferences every slot with no NULL guard, and is_canonical's call
+ * to extract_min_max would hit its ossl_assert(aor != NULL) on a hole.
*/
static int ASIdentifierChoice_canonize(ASIdentifierChoice *choice)
{
@@ -470,8 +477,10 @@ static int ASIdentifierChoice_canonize(ASIdentifierChoice *choice)
}
ASIdOrRange_free(cur);
/*
- * NULL the source slot so any later teardown does not
- * walk a freed pointer. We do not advance `write`.
+ * Record the consumed source slot as NULL; the epilogue
+ * compacts NULLs out of the live stack so it is left
+ * hole-free whether we succeed or fail. We do not
+ * advance `write`.
*/
(void)sk_ASIdOrRange_set(choice->u.asIdsOrRanges, read, NULL);
continue;
@@ -494,15 +503,35 @@ static int ASIdentifierChoice_canonize(ASIdentifierChoice *choice)
done:
/*
- * On success every slot at [write..n-1] is NULL, so popping the
- * tail leaves the canonicalised list at [0..write-1]. On error we
- * leave the tail untouched; the slots are either NULL (from earlier
- * iterations) or original entries the loop never reached, both of
- * which the caller's ASIdentifierChoice_free path handles safely.
+ * The sweep above NULLs source slots as it folds entries. Whether
+ * we succeeded or bailed out on an error, the stack must be left
+ * hole-free, with every occupied slot non-NULL, so that the caller
+ * may inspect, print, encode, free, or even retry canonize on the
+ * object without dereferencing NULL (the sort comparator in
+ * particular has no NULL guard). Slide every non-NULL slot forward
+ * to close the holes, then pop the vacated tail. On success this
+ * collapses [write..n-1] (all NULL) to length `write`; on error it
+ * removes the possibly empty contiguous run of NULL slots in
+ * [write, read), preserving the processed output in [0, write) and
+ * the untouched original entries in [read, n).
*/
- if (ret) {
- while (sk_ASIdOrRange_num(choice->u.asIdsOrRanges) > write)
+ {
+ int w = 0, r;
+
+ for (r = 0; r < sk_ASIdOrRange_num(choice->u.asIdsOrRanges); r++) {
+ ASIdOrRange *v = sk_ASIdOrRange_value(choice->u.asIdsOrRanges, r);
+
+ if (v != NULL) {
+ if (w != r)
+ (void)sk_ASIdOrRange_set(choice->u.asIdsOrRanges, w, v);
+ w++;
+ }
+ }
+ while (sk_ASIdOrRange_num(choice->u.asIdsOrRanges) > w)
(void)sk_ASIdOrRange_pop(choice->u.asIdsOrRanges);
+ }
+
+ if (ret) {
/* Paranoia */
if (!ossl_assert(ASIdentifierChoice_is_canonical(choice)))
ret = 0;
diff --git a/test/v3ext.c b/test/v3ext.c
index e2b6441197..6d8c8f3dae 100644
--- a/test/v3ext.c
+++ b/test/v3ext.c
@@ -753,12 +753,11 @@ end:
* Trigger an overlap-detection error partway through the linear
* merge. The first V3EXT_TEST_LARGE_N / 2 entries are adjacent and
* mergeable; entry K is a duplicate of entry K-1 (overlap). The
- * canonize call must return 0, and the caller's normal teardown of
- * the choice must safely free the stack -- some slots hold merged
- * results, some hold NULL (from earlier merges), and some hold
- * originals that the loop never reached. ASan / UBSan-instrumented
- * builds will catch any double-free or use-after-free in the
- * teardown that the mixed-state-on-error invariant claims to avoid.
+ * canonize call must return 0, and the resulting object must remain
+ * valid: the failed canonize must leave the stack partially
+ * canonicalized but hole-free, so that inspecting it, retrying
+ * canonize, and freeing it are all safe. ASan / UBSan-instrumented
+ * builds will catch any double-free or use-after-free in those walks.
*/
static int test_asid_canonize_error_midsweep(void)
{
@@ -801,9 +800,18 @@ static int test_asid_canonize_error_midsweep(void)
goto err;
/*
- * Successful return below relies on ASIdentifiers_free walking
- * the partially-compacted stack without UAF or double-free.
- * Under ASan / UBSan that walk is the actual test.
+ * The object must also be safe to inspect and to retry, not only
+ * to free: both calls walk (and the second re-sorts) the stack,
+ * so they would crash on any NULL slot left by the mid-sweep merge.
+ */
+ if (!TEST_int_eq(X509v3_asid_is_canonical(asid), 0)
+ || !TEST_int_eq(X509v3_asid_canonize(asid), 0))
+ goto err;
+
+ /*
+ * Successful return below relies on ASIdentifiers_free walking the
+ * partially-canonicalized, hole-free stack without UAF or
+ * double-free. Under ASan / UBSan that walk is the actual test.
*/
testresult = 1;
err:
@@ -817,13 +825,11 @@ err:
* in IPAddressOrRanges_canonize. Construct a list whose first half is
* adjacent and mergeable, with a duplicate at position k that hits the
* overlap check after a series of merges has driven write < read.
- * The canonize call must return 0, and the family's normal teardown
- * (sk_IPAddressFamily_pop_free) must safely walk the partially
- * compacted stack -- ASan / UBSan catches any double-free or UAF the
- * mixed-state-on-error invariant would otherwise miss. Because the
- * v3_addr.c canonize uses direct `return 0` rather than a `done:`
- * cleanup label, the teardown invariant for this file is different
- * from the asid path and warrants its own coverage.
+ * The canonize call must return 0, and the resulting object must
+ * remain valid: the failed canonize must leave the stack partially
+ * canonicalized but hole-free, so that inspecting it, retrying
+ * canonize, and freeing it are all safe. ASan / UBSan catches any
+ * double-free or UAF in those walks.
*/
static int test_addr_canonize_error_midsweep(void)
{
@@ -860,10 +866,20 @@ static int test_addr_canonize_error_midsweep(void)
if (!TEST_int_eq(X509v3_addr_canonize(addr), 0))
goto end;
+ /*
+ * The object must also be safe to inspect and to retry, not only
+ * to free: both calls walk (and the second re-sorts) the stack,
+ * so they would crash on any NULL slot left by the mid-sweep merge.
+ */
+ if (!TEST_int_eq(X509v3_addr_is_canonical(addr), 0)
+ || !TEST_int_eq(X509v3_addr_canonize(addr), 0))
+ goto end;
+
/*
* Successful return below relies on sk_IPAddressFamily_pop_free
- * walking the partially-compacted aors stack without UAF or
- * double-free. Under ASan / UBSan that walk is the actual test.
+ * walking the partially-canonicalized, hole-free aors stack
+ * without UAF or double-free. Under ASan / UBSan that walk is
+ * the actual test.
*/
testresult = 1;
end:
@@ -871,6 +887,102 @@ end:
return testresult;
}
+/*
+ * Verify that an ASIdentifiers object remains safe to inspect and to
+ * retry after a canonize() call fails. The input [1, 2, 2] fails
+ * because 2 overlaps the merged [1, 2] range; the merge of 1 and 2
+ * runs first, so the failure happens mid-sweep. canonize() must
+ * return 0 and leave the object in a state where is_canonical() and
+ * a second canonize() both run without crashing and return 0.
+ */
+static int test_asid_canonize_failure_then_inspect(void)
+{
+ ASIdentifiers *asid = NULL;
+ ASN1_INTEGER *val = NULL;
+ int testresult = 0;
+
+ if (!TEST_ptr(asid = ASIdentifiers_new()))
+ goto err;
+
+ if (!TEST_ptr(val = ASN1_INTEGER_new())
+ || !TEST_true(ASN1_INTEGER_set_int64(val, 1))
+ || !TEST_true(X509v3_asid_add_id_or_range(asid, V3_ASID_ASNUM,
+ val, NULL)))
+ goto err;
+ val = NULL;
+ if (!TEST_ptr(val = ASN1_INTEGER_new())
+ || !TEST_true(ASN1_INTEGER_set_int64(val, 2))
+ || !TEST_true(X509v3_asid_add_id_or_range(asid, V3_ASID_ASNUM,
+ val, NULL)))
+ goto err;
+ val = NULL;
+ if (!TEST_ptr(val = ASN1_INTEGER_new())
+ || !TEST_true(ASN1_INTEGER_set_int64(val, 2))
+ || !TEST_true(X509v3_asid_add_id_or_range(asid, V3_ASID_ASNUM,
+ val, NULL)))
+ goto err;
+ val = NULL;
+
+ /* canonize must reject the overlap. */
+ if (!TEST_int_eq(X509v3_asid_canonize(asid), 0))
+ goto err;
+
+ /* The object must be safe to inspect and to retry after the failure. */
+ if (!TEST_int_eq(X509v3_asid_is_canonical(asid), 0))
+ goto err;
+ if (!TEST_int_eq(X509v3_asid_canonize(asid), 0))
+ goto err;
+
+ testresult = 1;
+err:
+ ASN1_INTEGER_free(val);
+ ASIdentifiers_free(asid);
+ return testresult;
+}
+
+/*
+ * Verify that an IPAddrBlocks object remains safe to inspect and to
+ * retry after a canonize() call fails. The input
+ * [1.0.0.0/32, 1.0.0.1/32, 1.0.0.1/32] fails because the third
+ * prefix overlaps the merged range of the first two; that merge runs
+ * first, so the failure happens mid-sweep. canonize() must return 0
+ * and leave the object in a state where is_canonical() and a second
+ * canonize() both run without crashing and return 0.
+ */
+static int test_addr_canonize_failure_then_inspect(void)
+{
+ IPAddrBlocks *addr = NULL;
+ unsigned char ip0[4] = { 1, 0, 0, 0 };
+ unsigned char ip1[4] = { 1, 0, 0, 1 };
+ int testresult = 0;
+
+ if (!TEST_ptr(addr = sk_IPAddressFamily_new_null()))
+ goto end;
+
+ if (!TEST_true(X509v3_addr_add_prefix(addr, IANA_AFI_IPV4, NULL,
+ ip0, 32))
+ || !TEST_true(X509v3_addr_add_prefix(addr, IANA_AFI_IPV4, NULL,
+ ip1, 32))
+ || !TEST_true(X509v3_addr_add_prefix(addr, IANA_AFI_IPV4, NULL,
+ ip1, 32)))
+ goto end;
+
+ /* canonize must reject the overlap. */
+ if (!TEST_int_eq(X509v3_addr_canonize(addr), 0))
+ goto end;
+
+ /* The object must be safe to inspect and to retry after the failure. */
+ if (!TEST_int_eq(X509v3_addr_is_canonical(addr), 0))
+ goto end;
+ if (!TEST_int_eq(X509v3_addr_canonize(addr), 0))
+ goto end;
+
+ testresult = 1;
+end:
+ sk_IPAddressFamily_pop_free(addr, IPAddressFamily_free);
+ return testresult;
+}
+
/*
* Exercise the merge arm where `cur` is itself a range (rather than a
* single integer), hitting the `case ASIdOrRange_range` detach branch
@@ -943,9 +1055,10 @@ err:
* well-formed adjacent integers; entry k is an explicitly inverted
* range (min = 1000, max = 100). X509v3_asid_add_id_or_range does
* not validate min <= max for ranges, so the bad entry is admitted
- * into the list, and canonize must detect it on the sweep. The
- * teardown under ASan / UBSan verifies that the early-exit path
- * leaves the asIdsOrRanges stack in a freeable state.
+ * into the list, and canonize must detect it on the sweep. Like the
+ * overlap case, the failure happens after earlier merges have run, so
+ * canonize must return 0 and leave the object safe to inspect, retry,
+ * and free.
*
* The addr-side counterpart of this branch (v3_addr.c:849) is not
* reachable through the public API: make_addressRange refuses to
@@ -997,6 +1110,15 @@ static int test_asid_canonize_inverted_midsweep(void)
if (!TEST_int_eq(X509v3_asid_canonize(asid), 0))
goto err;
+ /*
+ * The object must also be safe to inspect and to retry, not only
+ * to free: both calls walk (and the second re-sorts) the stack,
+ * so they would crash on any NULL slot left by the mid-sweep merge.
+ */
+ if (!TEST_int_eq(X509v3_asid_is_canonical(asid), 0)
+ || !TEST_int_eq(X509v3_asid_canonize(asid), 0))
+ goto err;
+
testresult = 1;
err:
ASN1_INTEGER_free(val);
@@ -1076,6 +1198,8 @@ int setup_tests(void)
ADD_TEST(test_addr_interleaved_canonize);
ADD_TEST(test_asid_canonize_error_midsweep);
ADD_TEST(test_addr_canonize_error_midsweep);
+ ADD_TEST(test_asid_canonize_failure_then_inspect);
+ ADD_TEST(test_addr_canonize_failure_then_inspect);
ADD_TEST(test_asid_range_merge_canonize);
ADD_TEST(test_asid_canonize_inverted_midsweep);
#endif /* OPENSSL_NO_RFC3779 */