Conversation
It is not guaranteed to be null terminated. Use memchr() instead. Reported-by: Max Dymond
There was a problem hiding this comment.
🟢 Approval recommended
Pull request overview
Updates the OpenSSL backend’s hostname verification to avoid calling strlen() on certificate name data returned by OpenSSL APIs, since it is not guaranteed to be NUL-terminated.
Changes:
- Replaces
altlen == strlen(altptr)with an in-boundsmemchr(altptr, '\0', altlen)check when verifyingsubjectAltNameDNS entries. - Keeps the embedded-NUL rejection behavior while avoiding out-of-bounds reads on non-terminated ASN.1 string data.
File summaries
| File | Description |
|---|---|
| lib/vtls/openssl.c | Avoids strlen() on SAN data from OpenSSL by using memchr() within the provided length. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bagder
marked this pull request as ready for review
August 31, 2026 20:05
This was referenced Sep 2, 2026
bagder
pushed a commit
that referenced
this pull request
Sep 2, 2026
#22767 removed some of the strlen calls, but it wasn't quite sufficient. Curl_cert_hostcheck has a number of places where it implicitly assumes the buffer is zero-terminated: - The check for empty strings assume that, even if the buffer is zero-length, it can be dereferenced. This only works for C strings where there's always a zero byte in there. Replace this with a length check. - The strncmp("*.") call will read past the buffer if the pattern equals "*". This works for C strings because it will stop at the zero byte first. For general buffers, this needs a length check first. The various checks for the first and last character are actually OK, because the caller rejects empty strings first, but I similarly guarded them for completeness. Likewise, I guarded the accesses of hostname in hopes it can eventually be de-C-string-ified, but the call to Curl_host_is_ipnum requires it to be a C string. Update unit1397 to cover these cases. Run each test with a separate-allocated buffer. The two fixed invalid accesses are then caught by ASan and valgrind. Closes #22795
bagder
pushed a commit
that referenced
this pull request
Sep 2, 2026
The code removed from #22767 was part of a mess from when cURL once that strings in certificates were C strings and did not have embedded zeros. See these historical commits: * c0e8bed * 0b66efa * 781b82b Since it seems there was never a test for any of this, fill in some of these missing tests now. (I was working on a fix for the same strlen issue but never got around to uploading it. Since #22767 has since landed, I figure I may as well contribute the tests.) Closes #22794
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.
It is not guaranteed to be null terminated. Use memchr() instead.
Reported-by: Max Dymond