Fix x509_get_entries() not validating CRL entry's own SEQUENCE length 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 #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 commit is contained in:
yi chen 2026-07-13 06:57:21 +08:00
parent 9e9eb069d6
commit e125fbde02
2 changed files with 14 additions and 0 deletions

View File

@ -0,0 +1,9 @@
Bugfix
* Fix x509_get_entries() (used by mbedtls_x509_crl_parse()) not
checking that a revoked-certificate entry's serial number,
revocation date and optional crlEntryExtensions fully consume
the entry's own declared SEQUENCE length. A CRL entry with a
declared length longer than the sum of its actual fields could
have the extra trailing bytes silently reinterpreted as one or
more additional, attacker-influenced CRL entries, instead of
being rejected as malformed.

View File

@ -262,6 +262,11 @@ static int x509_get_entries(unsigned char **p,
return ret;
}
if (*p != end2) {
return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_FORMAT,
MBEDTLS_ERR_ASN1_LENGTH_MISMATCH);
}
if (*p < end) {
cur_entry->next = mbedtls_calloc(1, sizeof(mbedtls_x509_crl_entry));