From c0a630d30b96a6c40a3162487d154158bd4e788f Mon Sep 17 00:00:00 2001 From: Tobias Frauenschlaeger Date: Fri, 21 Aug 2026 22:31:29 +0000 Subject: [PATCH] Store an alt name with its entry in one allocation Parsing the subject alternative names of a certificate allocated a DNS_entry and then a second buffer for the name it points at, which the code itself flagged with a "consider one malloc" note. A chain carrying six names cost twelve allocations on every handshake that verifies it. AltNameNewEx() allocates the entry with room for the name behind it and copies the name in, leaving nameStored at 0 so FreeAltNames() does not try to release it separately. The failure path in SetDNSEntry() now hands the entry to FreeAltNames() rather than freeing the two buffers by hand, which also releases the ipString and ridString a partly built entry may already hold. --- src/x509.c | 19 +++------- tests/api/test_asn.c | 49 ++++++++++++++++++++++++++ tests/api/test_asn.h | 4 ++- wolfcrypt/src/asn.c | 77 ++++++++++++++++++++++++++++------------- wolfssl/wolfcrypt/asn.h | 11 ++++-- 5 files changed, 118 insertions(+), 42 deletions(-) diff --git a/src/x509.c b/src/x509.c index b9cdb5f14a8..66850941c18 100644 --- a/src/x509.c +++ b/src/x509.c @@ -3119,7 +3119,6 @@ int wolfSSL_X509_add_altname_ex(WOLFSSL_X509* x509, const char* name, word32 nameSz, int type) { DNS_entry* newAltName = NULL; - char* nameCopy = NULL; if (x509 == NULL) return WOLFSSL_FAILURE; @@ -3127,25 +3126,17 @@ int wolfSSL_X509_add_altname_ex(WOLFSSL_X509* x509, const char* name, if ((name == NULL) || (nameSz == 0)) return WOLFSSL_SUCCESS; - newAltName = AltNameNew(x509->heap); - if (newAltName == NULL) + /* AltNameNewEx() takes a signed length. */ + if (nameSz > (word32)INT_MAX) return WOLFSSL_FAILURE; - nameCopy = (char*)XMALLOC(nameSz + 1, x509->heap, DYNAMIC_TYPE_ALTNAME); - if (nameCopy == NULL) { - XFREE(newAltName, x509->heap, DYNAMIC_TYPE_ALTNAME); + /* One block, so nothing to unwind on failure. */ + newAltName = AltNameNewEx(name, (int)nameSz, x509->heap); + if (newAltName == NULL) return WOLFSSL_FAILURE; - } - - XMEMCPY(nameCopy, name, nameSz); - - nameCopy[nameSz] = '\0'; newAltName->next = x509->altNames; newAltName->type = type; - newAltName->len = (int)nameSz; - newAltName->name = nameCopy; - newAltName->nameStored = 1; x509->altNames = newAltName; return WOLFSSL_SUCCESS; diff --git a/tests/api/test_asn.c b/tests/api/test_asn.c index 08995f4fa0d..de996b15362 100644 --- a/tests/api/test_asn.c +++ b/tests/api/test_asn.c @@ -5072,3 +5072,52 @@ int test_wc_AsnFeatureCoverage(void) #endif /* !NO_ASN && HAVE_ECC && USE_CERT_BUFFERS_256 && !HAVE_FIPS */ return EXPECT_RESULT(); } + +/* AltNameNewEx() stores the name inside the entry's own allocation, so + * FreeAltNames() releases both with one free. Check the copy, the length, the + * terminator, and that a NULL or empty name still yields a usable entry. + */ +int test_wc_AltNameNewEx(void) +{ + EXPECT_DECLS; +#if !defined(NO_ASN) && !defined(NO_CERTS) && \ + (defined(WOLFSSL_TEST_CERT) || defined(OPENSSL_EXTRA) || \ + defined(OPENSSL_EXTRA_X509_SMALL) || defined(WOLFSSL_PUBLIC_ASN)) + const char name[] = "example.com"; + DNS_entry* entry = NULL; + + ExpectNotNull(entry = AltNameNewEx(name, (int)XSTRLEN(name), NULL)); + if (entry != NULL) { + ExpectIntEQ(entry->len, (int)XSTRLEN(name)); + ExpectNotNull(entry->name); + ExpectIntEQ(XMEMCMP(entry->name, name, XSTRLEN(name)), 0); + /* The name is NUL terminated and part of the entry's allocation. */ + ExpectIntEQ(entry->name[XSTRLEN(name)], '\0'); + ExpectIntEQ(entry->nameStored, 0); + /* The name region sits after the struct in the entry's own block, + * which is the property that makes one allocation and one free + * correct. */ + ExpectTrue((const char*)entry < entry->name); + ExpectTrue(entry->name < + (const char*)entry + sizeof(DNS_entry) + XSTRLEN(name) + 1); + } + FreeAltNames(entry, NULL); + entry = NULL; + + /* A length with no name to go with it is rejected rather than leaving + * len covering bytes that were never written, and a negative length is + * rejected rather than used as a size. */ + ExpectNull(AltNameNewEx(NULL, 1, NULL)); + ExpectNull(AltNameNewEx(name, -1, NULL)); + + /* An empty name is still a valid entry with a terminated string. */ + ExpectNotNull(entry = AltNameNewEx(NULL, 0, NULL)); + if (entry != NULL) { + ExpectIntEQ(entry->len, 0); + ExpectNotNull(entry->name); + ExpectIntEQ(entry->name[0], '\0'); + } + FreeAltNames(entry, NULL); +#endif + return EXPECT_RESULT(); +} diff --git a/tests/api/test_asn.h b/tests/api/test_asn.h index 3611ea9e799..dcfe0b2c0ab 100644 --- a/tests/api/test_asn.h +++ b/tests/api/test_asn.h @@ -59,6 +59,7 @@ int test_wc_DecodeExtKeyUsage_ssh_oid_collision(void); int test_wc_DecodeExtKeyUsage_oidSumCollision(void); int test_wc_AsnDecisionCoverage(void); int test_wc_AsnFeatureCoverage(void); +int test_wc_AltNameNewEx(void); #define TEST_ASN_DECLS \ TEST_DECL_GROUP("asn", test_SetAsymKeyDer), \ @@ -95,6 +96,7 @@ int test_wc_AsnFeatureCoverage(void); TEST_DECL_GROUP("asn", test_wc_DecodeExtKeyUsage_ssh_oid_collision), \ TEST_DECL_GROUP("asn", test_wc_DecodeExtKeyUsage_oidSumCollision), \ TEST_DECL_GROUP("asn", test_wc_AsnDecisionCoverage), \ - TEST_DECL_GROUP("asn", test_wc_AsnFeatureCoverage) + TEST_DECL_GROUP("asn", test_wc_AsnFeatureCoverage), \ + TEST_DECL_GROUP("asn", test_wc_AltNameNewEx) #endif /* WOLFCRYPT_TEST_ASN_H */ diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 3d458e6df0f..621cba5a08b 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -13134,6 +13134,43 @@ void FreeAltNames(DNS_entry* altNames, void* heap) } } +/* Allocate an alt name entry holding a NUL terminated copy of the name in the + * same block. nameStored stays 0, so the name is never freed on its own. + * + * str Name to store. May be NULL only when strLen is 0. + * strLen Length of the name in bytes, zero or more. + * heap Heap hint for the allocation. + * returns the new alt name, or NULL on failure. + */ +DNS_entry* AltNameNewEx(const char* str, int strLen, void* heap) +{ + DNS_entry* ret; + char* name; + + /* len would otherwise cover bytes that were never written. */ + if ((strLen < 0) || ((str == NULL) && (strLen > 0))) + return NULL; + + ret = (DNS_entry*)XMALLOC(sizeof(DNS_entry) + (size_t)strLen + 1, heap, + DYNAMIC_TYPE_ALTNAME); + if (ret == NULL) + return NULL; + + XMEMSET(ret, 0, sizeof(DNS_entry)); + name = (char*)ret + sizeof(DNS_entry); + if ((str != NULL) && (strLen > 0)) + XMEMCPY(name, str, (size_t)strLen); + name[strLen] = '\0'; + ret->name = name; + ret->len = strLen; +#ifdef WC_ASN_NO_HEAP + ret->entryStored = 1; /* heap-allocated node; FreeAltNames frees it */ +#endif + + (void)heap; + return ret; +} + /* malloc and initialize a new alt name structure */ DNS_entry* AltNameNew(void* heap) { @@ -15269,11 +15306,8 @@ static int SetDNSEntry(void* heap, DNS_entry* pool, word32* poolUsed, const char* str, int strLen, int type, DNS_entry** entries) { - DNS_entry* dnsEntry; + DNS_entry* dnsEntry = NULL; int ret = 0; -#ifndef WC_ASN_NO_HEAP - char *dnsEntry_name = NULL; -#endif #ifdef WC_ASN_NO_HEAP /* No heap: borrow a pool slot; name points into the source DER. */ @@ -15309,31 +15343,20 @@ static int SetDNSEntry(void* heap, DNS_entry* pool, word32* poolUsed, #else (void)pool; (void)poolUsed; - /* TODO: consider one malloc. */ - /* Allocate DNS Entry object. */ - dnsEntry = AltNameNew(heap); - if (dnsEntry == NULL) { - ret = MEMORY_E; + /* Screened here so a NULL from AltNameNewEx() means out of memory and + * nothing else. */ + if ((strLen < 0) || ((str == NULL) && (strLen > 0))) { + ret = BAD_FUNC_ARG; } if (ret == 0) { - /* Allocate DNS Entry name - length of string plus 1 for NUL. */ - dnsEntry->name = dnsEntry_name = (char*)XMALLOC((size_t)strLen + 1, - heap, DYNAMIC_TYPE_ALTNAME); - if (dnsEntry->name == NULL) { + /* Allocate DNS Entry object holding the name. */ + dnsEntry = AltNameNewEx(str, strLen, heap); + if (dnsEntry == NULL) { ret = MEMORY_E; } - else { - dnsEntry->nameStored = 1; - } } if (ret == 0) { - /* Set tag type, name length, name and NUL terminate name. */ dnsEntry->type = type; - dnsEntry->len = strLen; - if (str != NULL && strLen > 0) { - XMEMCPY(dnsEntry_name, str, (size_t)strLen); - } - dnsEntry_name[strLen] = '\0'; #ifdef WOLFSSL_RID_ALT_NAME /* store registeredID as a string */ @@ -15348,12 +15371,16 @@ static int SetDNSEntry(void* heap, DNS_entry* pool, word32* poolUsed, #endif if (ret == 0) { ret = AddDNSEntryToList(entries, dnsEntry); + /* The list owns it from here. */ + if (ret == 0) + dnsEntry = NULL; } - /* failure cleanup */ + /* Only reached when the entry was never linked. Clear next so + * FreeAltNames() frees this node alone. */ if (ret != 0 && dnsEntry != NULL) { - XFREE(dnsEntry_name, heap, DYNAMIC_TYPE_ALTNAME); - XFREE(dnsEntry, heap, DYNAMIC_TYPE_ALTNAME); + dnsEntry->next = NULL; + FreeAltNames(dnsEntry, heap); } #endif diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 91cb91afd47..2a7a737c642 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -1548,8 +1548,12 @@ struct DNS_entry { int type; /* i.e. ASN_DNS_TYPE */ int len; /* actual DNS len */ const char* - name; /* actual DNS name; under WC_ASN_NO_HEAP this points into - * the source DER and is NOT NUL-terminated - use len */ + name; /* actual DNS name; under WC_ASN_NO_HEAP a parsed entry + * points into the source DER and is NOT NUL-terminated + * - use len */ + /* 1 = name is its own allocation and FreeAltNames() releases it. + * 0 = name is borrowed from the DER, or inside the entry's own + * block. Either way, never free it on its own. */ int nameStored; #ifdef WOLFSSL_IP_ALT_NAME char* ipString; /* human readable form of IP address */ @@ -2438,6 +2442,7 @@ typedef enum MimeStatus #ifdef WOLFSSL_API_PREFIX_MAP #define FreeAltNames wc_FreeAltNames #define AltNameNew wc_AltNameNew + #define AltNameNewEx wc_AltNameNewEx #define AltNameDup wc_AltNameDup #ifndef IGNORE_NAME_CONSTRAINTS #define FreeNameSubtrees wc_FreeNameSubtrees @@ -2498,6 +2503,8 @@ WOLFSSL_LOCAL int StreamOctetString(const byte* inBuf, word32 inBufSz, WOLFSSL_ASN_API void FreeAltNames(DNS_entry* altNames, void* heap); WOLFSSL_ASN_API DNS_entry* AltNameNew(void* heap); +WOLFSSL_ASN_API DNS_entry* AltNameNewEx(const char* str, int strLen, + void* heap); WOLFSSL_ASN_API DNS_entry* AltNameDup(DNS_entry* from, void* heap); #if defined(WOLFSSL_ASN_TEMPLATE) && defined(WOLFSSL_CERT_GEN) && \ defined(WOLFSSL_ALT_NAMES)