Add public alt-name list APIs#10768
Conversation
|
retest this please |
|
Expose the internal SAN-list helpers as public wc_ APIs so callers can build a DNS_entry list, encode it into a DER GeneralNames SEQUENCE, and populate a Cert directly: - wc_SetDNSEntry() - append a typed alt-name entry to a list - wc_FlattenAltNames() - encode a list into a buffer (thin wrapper) - wc_SetAltNamesFromList() - encode a list straight into cert->altNames/Sz Declarations live in asn.h (they use the DNS_entry type) and are gated by the existing WOLFSSL_ASN_API export macro; doxygen notes the WOLFSSL_PUBLIC_ASN/ OPENSSL_EXTRA export requirement. Adds a wolfCrypt test covering the success, NULL-list, NULL-output, BUFFER_E, and Cert paths.
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] New public wc_SetDNSEntry() does no argument validation, unlike its sibling wrappers —
wolfcrypt/src/asn.c:14528-14532 - [Medium] Test does not cover documented NULL-names path of wc_SetAltNamesFromList or wc_SetDNSEntry error paths —
wolfcrypt/test/test.c:26581-26606 - [Low] API availability mismatch: wc_SetDNSEntry can build a list that cannot be flattened in some build configs —
wolfssl/wolfcrypt/asn.h:2393-2397
Review generated by Skoll
| * given name, set its type/length, and append it to the linked list. The list | ||
| * is freed with FreeAltNames() and can be flattened with | ||
| * wc_FlattenAltNames(). */ | ||
| int wc_SetDNSEntry(void* heap, const char* str, int strLen, int type, |
There was a problem hiding this comment.
🟠 [Medium] New public wc_SetDNSEntry() does no argument validation, unlike its sibling wrappers
This PR promotes the previously static internal SetDNSEntry() to a public API via a bare pass-through wrapper. The other two wrappers added in this same PR validate their pointer arguments (wc_SetAltNamesFromList checks cert == NULL, and wc_FlattenAltNames/FlattenAltNames checks output == NULL), but wc_SetDNSEntry performs no validation at all. Because it now accepts caller-supplied values from outside the library, malformed inputs reach SetDNSEntry() directly: a negative strLen flows into XMALLOC((size_t)strLen + 1, ...) and XMEMCPY(dnsEntry_name, str, (size_t)strLen) where the negative value casts to a huge size_t (out-of-bounds copy / crash); a NULL entries is dereferenced in AddDNSEntryToList(*lst...); and a NULL str with non-zero strLen causes a NULL read in XMEMCPY. The doxygen for this function does not document any BAD_FUNC_ARG return for these cases. wolfSSL convention is for public wc_ functions to return a non-void error and validate inputs at the boundary.
Fix: Add boundary validation (NULL str, NULL entries, negative strLen) in the public wrapper and document the BAD_FUNC_ARG return in the doxygen, to be consistent with the other two wrappers added in this PR and with wolfSSL public-API conventions.
| ret = WC_TEST_RET_ENC_EC(len); | ||
| } | ||
|
|
||
| /* wc_SetAltNamesFromList() encodes the same list straight into a Cert and |
There was a problem hiding this comment.
🟠 [Medium] Test does not cover documented NULL-names path of wc_SetAltNamesFromList or wc_SetDNSEntry error paths
The new test exercises the success paths, NULL-output, BAD_FUNC_ARG, BUFFER_E, and the NULL-cert path well. However, wc_SetAltNamesFromList() documents that names may be NULL, in which case cert->altNamesSz is set to 0 (this is a distinct branch: FlattenAltNames returns 0 and the function stores 0). That documented behavior is not asserted by any test. Likewise, none of the wc_SetDNSEntry() error returns are exercised. These are reasonable gaps to close while a dedicated test is being added.
Fix: Add an assertion that wc_SetAltNamesFromList(cert, NULL) returns 0 and sets cert->altNamesSz == 0, matching the documented contract. Optionally add a wc_SetDNSEntry error-path case.
| WOLFSSL_ASN_API void FreeAltNames(DNS_entry* altNames, void* heap); | ||
| WOLFSSL_ASN_API DNS_entry* AltNameNew(void* heap); | ||
| WOLFSSL_ASN_API DNS_entry* AltNameDup(DNS_entry* from, void* heap); | ||
| #if defined(WOLFSSL_ASN_TEMPLATE) && \ |
There was a problem hiding this comment.
🔵 [Low] API availability mismatch: wc_SetDNSEntry can build a list that cannot be flattened in some build configs
wc_SetDNSEntry is gated by WOLFSSL_ASN_TEMPLATE && (WOLFSSL_CERT_GEN || !defined(NO_CERTS)), while its companions wc_FlattenAltNames and wc_SetAltNamesFromList are gated by WOLFSSL_CERT_GEN && WOLFSSL_ALT_NAMES. In a build with !NO_CERTS and WOLFSSL_ASN_TEMPLATE but without WOLFSSL_CERT_GEN/WOLFSSL_ALT_NAMES, a caller can construct a DNS_entry list via wc_SetDNSEntry (whose doxygen \sa points at wc_FlattenAltNames) but has no public function to encode it. This is a benign surface inconsistency, not a build break, but it makes the documented usage flow unavailable in that configuration.
Fix: Consider aligning the guards (or documenting the gap) so the wc_SetDNSEntry -> wc_FlattenAltNames flow advertised in the doxygen is available together. Mirroring the internal availability is acceptable, but a short note in the docs would avoid confusion.
Promotes wolfSSL's internal SAN-list helpers to a public
wc_API so apps can build SAN entries, encode them, and attach them to aCertbefore signing without using internal functions.New functions
wc_SetDNSEntry()DNS_entrylist.wc_FlattenAltNames()GeneralNamesSEQUENCE (general buffer primitive).wc_SetAltNamesFromList()cert->altNames/altNamesSz- mirrorswc_SetAltNamesBuffer.Notes
asn.h(they use theDNS_entrytype) and share theWOLFSSL_ASN_APIexport gate (WOLFSSL_PUBLIC_ASN/OPENSSL_EXTRA/…); doxygen documents this.WOLFSSL_CERT_GEN && WOLFSSL_ALT_NAMES.Testing
Adds
flattenAltNames_testcovering success, NULL-list, NULL-output,BUFFER_E, and theCertpaths. Builds clean and passes under--enable-all.Used in wolfCert.