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)