Store an alt name with its entry in one allocation - #11342
Open
Frauschi wants to merge 1 commit into
Open
Conversation
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.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11342
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
|
Contributor
Author
|
Jenkins retest this please - history lost. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One of six independent branches that cut allocations in the TLS and certificate layers. Self-contained and touches no TLS code.
What changes
Parsing the subject alternative names of a certificate allocated a
DNS_entryand then a second buffer for the name it points at - a shape 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, so a six name chain costs six allocations.wolfSSL_X509_add_altname_ex()uses the same helper.The failure path in
SetDNSEntry()now hands the entry toFreeAltNames()instead of freeing two buffers by hand, so anyipStringorridStringa later step allocates is released with it. Nothing leaked before this - no in-tree path can fail after those buffers are built - it is hardening against a future failure path.API note
An entry built this way reports
nameStored == 0, which parsing already used for a name borrowed from the input DER. Either way the name is not a separate allocation and must not be freed on its own. Application code that walksWOLFSSL_X509->altNamesorDecodedCert->altNamesand freesnameonly whennameStoredis set is unaffected; code that frees it unconditionally was already wrong.AltNameNewEx()rejects a negative length, and a NULL name with a positive length, rather than leavinglencovering bytes that were never written - which the old inline code did.SetDNSEntry()screens those arguments before allocating so a NULL return really does mean out of memory, rather than reporting a bad argument asMEMORY_Eand sending a caller that retries onMEMORY_Eround forever.wolfSSL_X509_add_altname_ex()bounds itsword32length againstINT_MAXexplicitly rather than relying on what an out-of-range value narrows to.