Fix x509_get_entries() not validating CRL entry's own SEQUENCE length is fully consumed - #10827
Open
94xhn wants to merge 1 commit into
Open
Fix x509_get_entries() not validating CRL entry's own SEQUENCE length is fully consumed#1082794xhn wants to merge 1 commit into
94xhn wants to merge 1 commit into
Conversation
… is fully consumed x509_get_entries() (used by mbedtls_x509_crl_parse()/_der()) parses each revokedCertificate entry's serial number, revocation date and optional crlEntryExtensions against end2, the bound derived from that entry's own declared SEQUENCE length. After parsing those fields it never checked that *p actually reached end2 -- it only checked *p < end, the bound of the whole revokedCertificates SEQUENCE OF. As a result, a CRL entry whose declared SEQUENCE length is larger than what its serial/time/extensions fields actually consume has its unaccounted trailing bytes silently reinterpreted as the start of the next sibling entry (or, if crafted as a nested SEQUENCE, as an entirely separate phantom entry), instead of being rejected as malformed. This means a single malformed/crafted entry can be silently split into multiple entries with attacker-influenced serial/date/extension contents. This mirrors the same "end of my own declared length" check already used elsewhere in the same file/module for analogous per-item length checks: x509_get_crl_entry_ext()'s own `if (*p != end)`, x509_crt.c's `if (*p != policy_end)` in x509_get_certificate_policies(), and mbedtls_x509_crl_parse_der()'s own final `if (p != end)`. Add the missing `if (*p != end2)` check immediately after x509_get_crl_entry_ext() returns, before the existing `*p < end` check that decides whether to allocate the next sibling entry. Verified with a standalone test harness built against this worktree's freshly built libmbedx509.a/libtfpsacrypto.a, calling the real public mbedtls_x509_crl_parse_der(): - Before this fix: a 44-byte CRL entries block where entry Mbed-TLS#1 declares a SEQUENCE length of 40 bytes but its serial+time+empty-extensions fields only consume 20, with the remaining 20 bytes shaped as a second well-formed RevokedCertificate SEQUENCE, parses successfully (ret == 0) and produces 2 entries instead of being rejected. - After this fix: the same input is rejected with MBEDTLS_ERR_ASN1_LENGTH_MISMATCH (ret == -102). - Regression-checked three legitimate scenarios, all still parse correctly: two well-formed entries (count == 2), one well-formed entry with no extensions field present at all (count == 1), and a CRL with no revokedCertificates field at all (count == 0). ## Disclosure Generative AI (Claude) was used to help investigate this issue and implement this fix. All changes were reviewed by me before submission. Signed-off-by: yi chen <94xhn1@gmail.com>
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.
Description
x509_get_entries()inlibrary/x509_crl.c(used bymbedtls_x509_crl_parse()/mbedtls_x509_crl_parse_der()) parses eachRevokedCertificateentry's serial number, revocation date and optionalcrlEntryExtensionsagainstend2, the bound derived from that entry'sown declared SEQUENCE length. After parsing those fields, the
function never checks that
*pactually reachedend2— it onlychecks
*p < end, which is the bound of the wholerevokedCertificates SEQUENCE OF, not of the individual entry.As a result, a CRL entry whose declared SEQUENCE length is larger than
what its serial/time/extensions fields actually consume has its
unaccounted trailing bytes silently reinterpreted as the start of the
next sibling entry (or, if crafted as a nested SEQUENCE, as an entirely
separate phantom entry), instead of being rejected as malformed. A
single malformed/crafted CRL entry can therefore be silently split into
multiple entries with attacker-influenced serial/date/extension
contents.
This is not a memory-safety issue:
mbedtls_asn1_get_len()alreadyenforces that a declared length cannot exceed the bytes actually
available before the caller-supplied
end, so*pcan never run pastthe real buffer. This is purely an input-validation/parsing-correctness
gap.
Fix
Add the missing
if (*p != end2)check immediately afterx509_get_crl_entry_ext()returns, mirroring the exact idiom alreadyused elsewhere in the same file/module for analogous per-item length
checks:
x509_get_crl_entry_ext()'s ownif (*p != end)check at the end ofthat function,
x509_crt.c'sif (*p != policy_end)inx509_get_certificate_policies(),mbedtls_x509_crl_parse_der()'s own finalif (p != end)check.Testing
Built a standalone test harness (not part of this PR) that links
against this branch's freshly built
libmbedx509.a/libtfpsacrypto.aand calls the real publicmbedtls_x509_crl_parse_der():revokedCertificatesblock whereentry DTLS #1 declares a SEQUENCE content length of 40 bytes, but its
serial (3B) + UTCTime (15B) + empty
crlEntryExtensions(30 00,2B) only consume 20 of those 40 bytes — with the remaining 20 bytes
shaped as a second, well-formed
RevokedCertificateSEQUENCE —parses successfully (
ret == 0) and produces 2 entries insteadof being rejected.
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH(ret == -102).build, all still parse correctly:
count == 2crlEntryExtensionsfield presentat all (it's OPTIONAL) →
count == 1revokedCertificatesfield at all (also OPTIONAL)→
count == 0Searched open/closed PRs and issues in this repo for
x509_get_entries,x509_crl entry,CRL entry length,revokedCertificates— found no existing PR or issue addressing this.Disclosure
Generative AI (Claude) was used to help investigate this issue and implement this fix. All changes were reviewed by me before submission.