Reject zero-length serial number in mbedtls_x509write_crt

[94xhn]

Summary

mbedtls_x509write_crt_set_serial_raw() only checks the upper bound on
serial_len (rejecting anything longer than
MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN), but silently accepts
serial_len == 0:

int mbedtls_x509write_crt_set_serial_raw(mbedtls_x509write_cert *ctx,
                                         const unsigned char *serial, size_t serial_len)
{
    if (serial_len > MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN) {
        return MBEDTLS_ERR_X509_BAD_INPUT_DATA;
    }

    ctx->serial_len = serial_len;
    memcpy(ctx->serial, serial, serial_len);

    return 0;
}

serial_len == 0 is also exactly the state a freshly-initialized
mbedtls_x509write_cert is left in if the setter is never called at all,
since mbedtls_x509write_crt_init() just memsets the whole struct to 0.

The bug

mbedtls_x509write_crt_der() encodes ctx->serial/ctx->serial_len
directly as an ASN.1 INTEGER:

MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_raw_buffer(&c, buf,
                                                        ctx->serial, ctx->serial_len));
if (*c & 0x80) {
    if (c - buf < 1) {
        return MBEDTLS_ERR_X509_BUFFER_TOO_SMALL;
    }
    *(--c) = 0x0;
    len++;
    MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_len(&c, buf,
                                                     ctx->serial_len + 1));
} else {
    MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_len(&c, buf,
                                                     ctx->serial_len));
}
MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_tag(&c, buf,
                                                 MBEDTLS_ASN1_INTEGER));

mbedtls_asn1_write_raw_buffer(&c, buf, ctx->serial, 0) is a no-op for a
zero-length input (tf-psa-crypto/utilities/asn1write.c):

int mbedtls_asn1_write_raw_buffer(unsigned char **p, const unsigned char *start,
                                  const unsigned char *buf, size_t size)
{
    size_t len = 0;
    if (*p < start || (size_t) (*p - start) < size) {
        return MBEDTLS_ERR_ASN1_BUF_TOO_SMALL;
    }
    len = size;
    (*p) -= len;              /* size == 0  ->  no-op, *p unchanged */
    if (len != 0) {
        memcpy(*p, buf, len); /* skipped   */
    }
    return (int) len;         /* returns 0 */
}

So with serial_len == 0, the cursor c never moves, and the following
*c & 0x80 test reads whatever byte happens to already sit at that
buffer position — in practice the tag byte of the Signature
AlgorithmIdentifier SEQUENCE that mbedtls_x509write_crt_der()
always writes immediately before the serial field
(mbedtls_asn1_write_algorithm_identifier_ext() always ends by writing
tag 0x30, whose top bit is clear). The else branch therefore always
runs deterministically: it writes a length octet of 0x00, then the
INTEGER tag 0x02. The CertificateSerialNumber field ends up
encoded as the two bytes 02 00.

Per X.690 §8.3.1, an INTEGER's contents "shall consist of one or more
octets" — 02 00 has zero content octets and is not a valid DER (or
even BER) encoding. I confirmed this independently:

$ printf '\x02\x00' > int_zero.der
$ openssl asn1parse -inform DER -in int_zero.der
    0:d=0  hl=2 l=   0 prim: INTEGER           :BAD INTEGER:[]

mbedtls's own parser side, mbedtls_x509_get_serial() in
library/x509.c, has no length check either and will happily re-parse
this malformed field, so the round-trip inside mbedtls is silent —
but a certificate generated this way can be rejected by other, stricter
X.509/ASN.1 parsers (as demonstrated above with OpenSSL's own decoder).

Fix

• Reject serial_len == 0 in mbedtls_x509write_crt_set_serial_raw(),
  alongside the existing upper-bound check.
• Add the same ctx->serial_len == 0 check directly in
  mbedtls_x509write_crt_der(), as defense-in-depth for the case where
  the setter is never called at all (the struct's zeroed-by-init()
  default state).
• Updated the doc comment for mbedtls_x509write_crt_set_serial_raw().
• Added a test case to x509_set_serial_check in
  tests/suites/test_suite_x509write.function covering the
  zero-length-serial rejection (the existing test there only covered the
  too-long case).
• Added a ChangeLog.d entry.

Testing / verification

I don't have the test suite building locally for this repo right now —
tf-psa-crypto and framework are git submodules that aren't
initialized in a plain checkout, and initializing + configuring the full
CMake build was out of scope for verifying this specific encoding bug.
Instead, I extracted the exact vulnerable logic into a standalone,
dependency-free host-C reproduction: the three real ASN.1-writing
primitives (mbedtls_asn1_write_len, mbedtls_asn1_write_tag,
mbedtls_asn1_write_raw_buffer, copied verbatim from
tf-psa-crypto/utilities/asn1write.c) plus a line-for-line copy of the
vulnerable serial-writing snippet from x509write_crt.c (including the
preceding 0x30 tag write that stands in for the real, always-present
AlgorithmIdentifier SEQUENCE tag byte).

Results (gcc -Wall -Wextra, compiles clean, run on Windows/MinGW):

== Case 1: serial_len = 0 (accepted by set_serial_raw) ==
Serial field bytes               (2 bytes): 02 00
  -> encodes as INTEGER, length octet = 0x00: content-less INTEGER.
  X.690 8.3.1: INTEGER contents 'shall consist of one or more octets'.
  This is REJECTED by strict ASN.1 decoders (verified with
  `openssl asn1parse -inform DER` on the exact 2 bytes `02 00`
  -> prints "BAD INTEGER").

== Case 2: serial_len = 1, serial = {0x00} (for contrast) ==
Serial field bytes               (3 bytes): 02 01 00
  -> valid DER INTEGER encoding of value 0.

== Case 3: serial_len = 4, serial = {01 02 03 04} (normal case) ==
Serial field bytes               (6 bytes): 02 04 01 02 03 04
  -> valid DER INTEGER encoding, unaffected by the bug.

This confirms: (a) the bug is 100% deterministic given the real call
sequence in mbedtls_x509write_crt_der(), not a rare edge case; (b) a
correct zero-valued serial (serial_len == 1, content byte 0x00) is
encoded correctly and is unaffected by rejecting serial_len == 0
specifically; (c) ordinary multi-byte serials are unaffected. I also
manually traced mbedtls_x509_get_serial() (library/x509.c) to confirm
it has no corresponding length check, so the round-trip-within-mbedtls
silence is real and not masked elsewhere.

Happy to add a build-verified test_suite_x509write run if someone can
confirm the preferred way to stand up the tf-psa-crypto/framework
submodules in CI for a one-off check, or if a maintainer can run it
against this branch.

Generative AI

I used generative AI tools (Claude) when researching and drafting this
PR, but a human has reviewed the code and reasoning and is responsible
for the content of this PR.