diff --git a/ChangeLog.d/fix-x509write-crt-zero-length-serial.txt b/ChangeLog.d/fix-x509write-crt-zero-length-serial.txt new file mode 100644 index 0000000000..396ed2e906 --- /dev/null +++ b/ChangeLog.d/fix-x509write-crt-zero-length-serial.txt @@ -0,0 +1,13 @@ +Bugfix + * Fix mbedtls_x509write_crt_set_serial_raw() silently accepting a + zero-length serial number, and mbedtls_x509write_crt_der() silently + encoding it as a syntactically invalid ASN.1 INTEGER (zero content + octets, which X.690 8.3.1 forbids) for the certificate's + CertificateSerialNumber field instead of failing. Certificates + generated this way (either by explicitly passing a zero-length + serial, or by never calling mbedtls_x509write_crt_set_serial_raw() + at all) could be rejected by other, stricter X.509/ASN.1 parsers. + mbedtls_x509write_crt_set_serial_raw() now returns + MBEDTLS_ERR_X509_BAD_INPUT_DATA for a zero-length serial, and + mbedtls_x509write_crt_der() now also rejects a zero-length serial + as a defense-in-depth check. diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 0a7b532404..6d41002e67 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -957,8 +957,8 @@ void mbedtls_x509write_crt_set_version(mbedtls_x509write_cert *ctx, int version) * input buffer * * \return 0 if successful, or - * #MBEDTLS_ERR_X509_BAD_INPUT_DATA if the provided input buffer - * is too big (longer than MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN) + * #MBEDTLS_ERR_X509_BAD_INPUT_DATA if \p serial_len is 0 or + * too big (longer than MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN) */ int mbedtls_x509write_crt_set_serial_raw(mbedtls_x509write_cert *ctx, const unsigned char *serial, size_t serial_len); diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 8c77f10c34..74f27af1cc 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -95,7 +95,7 @@ int mbedtls_x509write_crt_set_issuer_name(mbedtls_x509write_cert *ctx, 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) { + if (serial_len == 0 || serial_len > MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN) { return MBEDTLS_ERR_X509_BAD_INPUT_DATA; } @@ -512,7 +512,18 @@ int mbedtls_x509write_crt_der(mbedtls_x509write_cert *ctx, * - if MSb of "serial" is 1, then prepend an extra 0x00 byte * - 1 byte for the length * - 1 byte for the TAG + * + * ctx->serial_len == 0 (the zeroed state left by + * mbedtls_x509write_crt_init() if mbedtls_x509write_crt_set_serial_raw() + * was never called) must be rejected here: an INTEGER with zero content + * octets is not valid DER (X.690 8.3.1 requires at least one octet), so + * letting it through would silently write a syntactically invalid + * `02 00` CertificateSerialNumber field instead of failing loudly. */ + if (ctx->serial_len == 0) { + return MBEDTLS_ERR_X509_BAD_INPUT_DATA; + } + MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_raw_buffer(&c, buf, ctx->serial, ctx->serial_len)); if (*c & 0x80) { diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index 760ff5fe03..8952817cd6 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -565,6 +565,7 @@ void x509_set_serial_check() { mbedtls_x509write_cert ctx; uint8_t invalid_serial[MBEDTLS_X509_RFC5280_MAX_SERIAL_LEN + 1]; + uint8_t zero_len_serial[1] = { 0x2a }; USE_PSA_INIT(); memset(invalid_serial, 0x01, sizeof(invalid_serial)); @@ -573,6 +574,12 @@ void x509_set_serial_check() sizeof(invalid_serial)), MBEDTLS_ERR_X509_BAD_INPUT_DATA); + /* A zero-length serial must also be rejected: an ASN.1 INTEGER with + * zero content octets is not valid DER (X.690 8.3.1), so + * mbedtls_x509write_crt_der() must never be allowed to encode one. */ + TEST_EQUAL(mbedtls_x509write_crt_set_serial_raw(&ctx, zero_len_serial, 0), + MBEDTLS_ERR_X509_BAD_INPUT_DATA); + exit: ; USE_PSA_DONE();