Fix non-minimal DER encoding of caller-supplied certificate serials

doc/dox_comments/header_files/asn_public.h

@@ -115,12 +115,25 @@ void  wc_CertFree(Cert* cert);
     either an rsaKey or an eccKey to generate the certificate.  The certificate
     must be initialized with wc_InitCert before this method is called.
 
+    A serial number left at the wc_InitCert default (cert->serialSz of 0) is
+    randomly generated. A caller-supplied serial is taken as a big-endian
+    value of at most CTC_SERIAL_SIZE bytes and is normalized to a minimal DER
+    INTEGER: redundant leading zero bytes are stripped and the sign pad is
+    added back when the high bit is set. Supplying the magnitude alone is
+    enough; a sign pad the caller adds is accepted and is not duplicated.
+    The encoded value, including any sign pad, must not exceed
+    CTC_SERIAL_SIZE octets.
+
     \return Success On successfully making an x509 certificate from the
     specified input cert, returns the size of the cert generated.
     \return MEMORY_E Returned if there is an error allocating memory
     with XMALLOC
     \return BUFFER_E Returned if the provided derBuffer is too small to
     store the generated certificate
+    \return BAD_FUNC_ARG Returned if cert->serialSz is negative, if the
+    encoded serial would exceed CTC_SERIAL_SIZE octets, or if the serial
+    number is zero. RFC 5280 4.1.2.2 requires a positive serial of at most
+    20 octets; define WOLFSSL_ASN_ALLOW_0_SERIAL to permit a zero serial.
     \return Others Additional error messages may be returned if the cert
     generation is not successful.
 
@@ -156,9 +169,13 @@ int  wc_MakeCert(Cert* cert, byte* derBuffer, word32 derSz, RsaKey* rsaKey,
     \ingroup ASN
     \brief Makes certificate with generic key type support.
 
+    The serial number contract is the same as wc_MakeCert().
+
     \return Size of certificate on success
     \return MEMORY_E if memory allocation fails
     \return BUFFER_E if buffer too small
+    \return BAD_FUNC_ARG if the serial number is zero, negative in size, or
+    longer than CTC_SERIAL_SIZE
     \return Other error codes on failure
 
     \param cert Initialized cert structure

tests/api/test_asn.c

@@ -4330,6 +4330,237 @@ int test_wc_MakeCert_generalizedTimeValidity(void)
     return EXPECT_RESULT();
 }
 
+#if defined(TEST_SIGN_CERT_BOUNDS_RSA) || defined(TEST_SIGN_CERT_BOUNDS_ECC)
+#define TEST_MAKECERT_SERIAL
+#endif
+
+#ifdef TEST_MAKECERT_SERIAL
+/* Longest serialNumber TLV the cases below expect. */
+#define TEST_SERIAL_TLV_MAX 8
+
+/* Find the serialNumber TLV in a certificate body. wc_MakeCert() writes a
+ * TBSCertificate, so the [0] EXPLICIT version precedes the serial. */
+static const byte* test_cert_serial_tlv(const byte* body, int bodySz)
+{
+    int i;
+
+    for (i = 0; ((i + 5 + TEST_SERIAL_TLV_MAX) <= bodySz) && (i < 16); i++) {
+        if ((body[i] == 0xA0) && (body[i + 1] == 0x03) &&
+            (body[i + 2] == ASN_INTEGER) && (body[i + 3] == 0x01) &&
+            (body[i + 5] == ASN_INTEGER)) {
+            return &body[i + 5];
+        }
+    }
+
+    return NULL;
+}
+
+/* Build a certificate body carrying the given serial. */
+static int test_cert_make_serial(Cert* cert, WC_RNG* rng, void* key,
+    byte* out, word32 outSz, const byte* serial, int serialSz)
+{
+    int ret;
+    int copySz = serialSz;
+
+    /* The caller scans this buffer once the call returns, so leave it in a
+     * known state on the paths that bail out before the generator writes. */
+    XMEMSET(out, 0, outSz);
+
+    ret = wc_InitCert(cert);
+    if (ret != 0)
+        return ret;
+
+    /* Keep the copy inside cert->serial so an over-long size can still be
+     * handed to the generator to exercise its bound check. */
+    if (copySz > CTC_SERIAL_SIZE)
+        copySz = CTC_SERIAL_SIZE;
+    if (copySz < 0)
+        copySz = 0;
+
+    cert->isCA = 0;
+    XMEMCPY(cert->serial, serial, (size_t)copySz);
+    cert->serialSz = serialSz;
+    XSTRNCPY(cert->subject.country, "US", CTC_NAME_SIZE);
+    XSTRNCPY(cert->subject.org, "wolfSSL", CTC_NAME_SIZE);
+    XSTRNCPY(cert->subject.commonName, "serial-encoding", CTC_NAME_SIZE);
+
+#ifdef TEST_SIGN_CERT_BOUNDS_RSA
+    cert->sigType = CTC_SHA256wRSA;
+    return wc_MakeCert(cert, out, outSz, (RsaKey*)key, NULL, rng);
+#else
+    cert->sigType = CTC_SHA256wECDSA;
+    return wc_MakeCert(cert, out, outSz, NULL, (ecc_key*)key, rng);
+#endif
+}
+#endif /* TEST_MAKECERT_SERIAL */
+
+/*
+ * A caller-supplied serial number must be encoded as a minimal DER INTEGER.
+ *
+ * A fixed-width device serial carries leading zero bytes that DER does not
+ * allow, and the encoder only ever adds the sign pad, so those bytes used to
+ * reach the certificate untouched. The result failed to parse, here and in
+ * every other strict decoder.
+ */
+int test_wc_MakeCert_serial_encoding(void)
+{
+    EXPECT_DECLS;
+#ifdef TEST_MAKECERT_SERIAL
+    static const struct {
+        byte serial[CTC_SERIAL_SIZE];
+        int  serialSz;
+        byte expect[TEST_SERIAL_TLV_MAX];
+        int  expectSz;
+    } cases[] = {
+        /* Redundant leading zeros are dropped. */
+        { { 0x00, 0x00, 0x00, 0x01 }, 4, { 0x02, 0x01, 0x01 }, 3 },
+        { { 0x00, 0x00, 0x12, 0x34 }, 4, { 0x02, 0x02, 0x12, 0x34 }, 4 },
+        { { 0x00, 0x12, 0x34, 0x56 }, 4, { 0x02, 0x03, 0x12, 0x34, 0x56 }, 5 },
+        /* Already minimal, so left alone. */
+        { { 0x12, 0x34, 0x56, 0x78 }, 4,
+          { 0x02, 0x04, 0x12, 0x34, 0x56, 0x78 }, 6 },
+        /* A set MSB still gets the sign pad it needs. */
+        { { 0x80, 0x00, 0x00, 0x01 }, 4,
+          { 0x02, 0x05, 0x00, 0x80, 0x00, 0x00, 0x01 }, 7 },
+        /* A sign pad the caller already supplied is not doubled. */
+        { { 0x00, 0x80, 0x01 }, 3, { 0x02, 0x03, 0x00, 0x80, 0x01 }, 5 }
+    };
+    static const byte zeroSerial[4] = { 0x00, 0x00, 0x00, 0x00 };
+    static const byte zeroTlv[3] = { 0x02, 0x01, 0x00 };
+    byte   maxSerial[CTC_SERIAL_SIZE];
+    WC_RNG rng;
+    Cert   cert;
+    DecodedCert decoded;
+    byte*  body = NULL;
+    const byte* tlv = NULL;
+    int    rngInit = 0;
+    int    bodySz = 0;
+    int    signedSz = 0;
+    int    i;
+#ifdef TEST_SIGN_CERT_BOUNDS_RSA
+    RsaKey key;
+    word32 idx = 0;
+#else
+    ecc_key key;
+    word32 idx = 0;
+#endif
+    int    keyInit = 0;
+
+    XMEMSET(&rng, 0, sizeof(rng));
+    XMEMSET(&cert, 0, sizeof(cert));
+    XMEMSET(&key, 0, sizeof(key));
+
+    ExpectIntEQ(wc_InitRng(&rng), 0);
+    if (EXPECT_SUCCESS()) rngInit = 1;
+
+    ExpectNotNull(body = (byte*)XMALLOC(SIGN_CERT_SCRATCH_SZ, HEAP_HINT,
+        DYNAMIC_TYPE_TMP_BUFFER));
+
+#ifdef TEST_SIGN_CERT_BOUNDS_RSA
+    ExpectIntEQ(wc_InitRsaKey_ex(&key, HEAP_HINT, testDevId), 0);
+    if (EXPECT_SUCCESS()) keyInit = 1;
+    ExpectIntEQ(wc_RsaPrivateKeyDecode(server_key_der_2048, &idx, &key,
+        sizeof_server_key_der_2048), 0);
+#else
+    ExpectIntEQ(wc_ecc_init_ex(&key, HEAP_HINT, testDevId), 0);
+    if (EXPECT_SUCCESS()) keyInit = 1;
+    ExpectIntEQ(wc_EccPrivateKeyDecode(ecc_key_der_256, &idx, &key,
+        sizeof_ecc_key_der_256), 0);
+#endif
+
+    for (i = 0; i < (int)(sizeof(cases) / sizeof(cases[0])); i++) {
+        ExpectIntGT(bodySz = test_cert_make_serial(&cert, &rng, &key, body,
+            SIGN_CERT_SCRATCH_SZ, cases[i].serial, cases[i].serialSz), 0);
+        ExpectNotNull(tlv = test_cert_serial_tlv(body, bodySz));
+        if (EXPECT_SUCCESS() && (tlv != NULL)) {
+            ExpectIntEQ(tlv[1] + 2, cases[i].expectSz);
+            ExpectIntEQ(XMEMCMP(tlv, cases[i].expect,
+                (size_t)cases[i].expectSz), 0);
+        }
+
+        /* What wolfSSL emits, wolfSSL has to be able to parse back. */
+#ifdef TEST_SIGN_CERT_BOUNDS_RSA
+        ExpectIntGT(signedSz = wc_SignCert(bodySz, cert.sigType, body,
+            SIGN_CERT_SCRATCH_SZ, &key, NULL, &rng), 0);
+#else
+        ExpectIntGT(signedSz = wc_SignCert(bodySz, cert.sigType, body,
+            SIGN_CERT_SCRATCH_SZ, NULL, &key, &rng), 0);
+#endif
+        if (EXPECT_SUCCESS()) {
+            wc_InitDecodedCert(&decoded, body, (word32)signedSz, HEAP_HINT);
+            ExpectIntEQ(wc_ParseCert(&decoded, CERT_TYPE, NO_VERIFY, NULL), 0);
+            wc_FreeDecodedCert(&decoded);
+        }
+    }
+
+#if !defined(WOLFSSL_NO_ASN_STRICT) && !defined(WOLFSSL_PYTHON) && \
+    !defined(WOLFSSL_ASN_ALLOW_0_SERIAL)
+    /* RFC 5280 4.1.2.2 needs a positive serial, so blank silicon has to fail
+     * at generation rather than mint a certificate nothing will accept. */
+    ExpectIntEQ(test_cert_make_serial(&cert, &rng, &key, body,
+        SIGN_CERT_SCRATCH_SZ, zeroSerial, (int)sizeof(zeroSerial)),
+        WC_NO_ERR_TRACE(BAD_FUNC_ARG));
+    (void)zeroTlv;
+#elif defined(WOLFSSL_ASN_TEMPLATE)
+    /* The permissive build still has to emit the canonical zero encoding.
+     * The original back end rejects zero regardless of this macro. */
+    ExpectIntGT(bodySz = test_cert_make_serial(&cert, &rng, &key, body,
+        SIGN_CERT_SCRATCH_SZ, zeroSerial, (int)sizeof(zeroSerial)), 0);
+    ExpectNotNull(tlv = test_cert_serial_tlv(body, bodySz));
+    if (EXPECT_SUCCESS() && (tlv != NULL)) {
+        ExpectIntEQ(XMEMCMP(tlv, zeroTlv, sizeof(zeroTlv)), 0);
+    }
+#else
+    (void)zeroSerial;
+    (void)zeroTlv;
+#endif
+
+    /* Exactly CTC_SERIAL_SIZE is the largest serial that must be accepted. */
+    XMEMSET(maxSerial, 0x11, sizeof(maxSerial));
+    ExpectIntGT(bodySz = test_cert_make_serial(&cert, &rng, &key, body,
+        SIGN_CERT_SCRATCH_SZ, maxSerial, CTC_SERIAL_SIZE), 0);
+    ExpectNotNull(tlv = test_cert_serial_tlv(body, bodySz));
+    if (EXPECT_SUCCESS() && (tlv != NULL)) {
+        ExpectIntEQ(tlv[1], CTC_SERIAL_SIZE);
+    }
+
+#ifdef WOLFSSL_ASN_TEMPLATE
+    /* A 20 byte serial with the high bit set needs a sign pad, which would
+     * push the encoded value to 21 octets. */
+    XMEMSET(maxSerial, 0x11, sizeof(maxSerial));
+    maxSerial[0] = 0x80;
+    ExpectIntEQ(test_cert_make_serial(&cert, &rng, &key, body,
+        SIGN_CERT_SCRATCH_SZ, maxSerial, CTC_SERIAL_SIZE),
+        WC_NO_ERR_TRACE(BAD_FUNC_ARG));
+#endif
+
+    /* A negative size is rejected by both ASN back ends. */
+    ExpectIntEQ(test_cert_make_serial(&cert, &rng, &key, body,
+        SIGN_CERT_SCRATCH_SZ, cases[0].serial, -1),
+        WC_NO_ERR_TRACE(BAD_FUNC_ARG));
+
+#ifdef WOLFSSL_ASN_TEMPLATE
+    /* Over the 20 octet ceiling RFC 5280 4.1.2.2 sets. The original ASN
+     * back end truncates through SetSerialNumber() instead of erroring. */
+    ExpectIntEQ(test_cert_make_serial(&cert, &rng, &key, body,
+        SIGN_CERT_SCRATCH_SZ, cases[0].serial, CTC_SERIAL_SIZE + 1),
+        WC_NO_ERR_TRACE(BAD_FUNC_ARG));
+#endif
+
+    XFREE(body, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);
+    if (keyInit) {
+#ifdef TEST_SIGN_CERT_BOUNDS_RSA
+        wc_FreeRsaKey(&key);
+#else
+        wc_ecc_free(&key);
+#endif
+    }
+    if (rngInit)
+        wc_FreeRng(&rng);
+#endif /* TEST_MAKECERT_SERIAL */
+    return EXPECT_RESULT();
+}
+
 /*
  * MC/DC wave 2 - decision-targeted negative paths for PKCS#8 wrap/parse
  * and RSA key decode. Targets argument-check, short-buffer, and

tests/api/test_asn.h

@@ -50,6 +50,7 @@ int test_ToTraditional_ex_negative(void);
 int test_ToTraditional_ex_mldsa_bad_params(void);
 int test_wc_SignCert_buffer_bounds(void);
 int test_wc_MakeCert_generalizedTimeValidity(void);
+int test_wc_MakeCert_serial_encoding(void);
 int test_wc_DecodeKeyUsage_decipherOnly(void);
 int test_wc_DecodeExtKeyUsage_ssh(void);
 int test_wc_DecodeExtKeyUsage_ssh_oid_collision(void);
@@ -83,6 +84,7 @@ int test_wc_AsnFeatureCoverage(void);
     TEST_DECL_GROUP("asn", test_ToTraditional_ex_mldsa_bad_params), \
     TEST_DECL_GROUP("asn", test_wc_SignCert_buffer_bounds),         \
     TEST_DECL_GROUP("asn", test_wc_MakeCert_generalizedTimeValidity), \
+    TEST_DECL_GROUP("asn", test_wc_MakeCert_serial_encoding),       \
     TEST_DECL_GROUP("asn", test_wc_DecodeKeyUsage_decipherOnly),    \
     TEST_DECL_GROUP("asn", test_wc_DecodeExtKeyUsage_ssh),          \
     TEST_DECL_GROUP("asn", test_wc_DecodeExtKeyUsage_ssh_oid_collision), \

tests/unit-mcdc/test_asn_cert_whitebox.c

@@ -2158,6 +2158,31 @@ static void wb_fill_name(CertName* name, const char* cn)
 /* Build one fixture. Returns 0 on success. Never asserts on the *content*
  * of the result beyond "the generator accepted it": the whole point of some
  * of these shapes is that a strict parser will later reject them. */
+/* Rewrite a freshly built body's serial number to zero.
+ *
+ * wc_MakeCert() refuses to emit a zero serial (RFC 5280 4.1.2.2), so a
+ * zero-serial fixture is built with 0x01 and the single content octet is
+ * patched here before signing. The encoded length does not change, so the
+ * signature still covers the whole body. */
+static int wb_patch_zero_serial(byte* der, int derSz)
+{
+    int i;
+
+    for (i = 0; ((i + 8) <= derSz) && (i < 16); i++) {
+        /* [0] EXPLICIT { INTEGER 2 } then the serial INTEGER holding 1. */
+        if ((der[i] == 0xA0) && (der[i + 1] == 0x03) &&
+            (der[i + 2] == ASN_INTEGER) && (der[i + 3] == 0x01) &&
+            (der[i + 4] == 0x02) &&
+            (der[i + 5] == ASN_INTEGER) && (der[i + 6] == 0x01) &&
+            (der[i + 7] == 0x01)) {
+            der[i + 7] = 0x00;
+            return 0;
+        }
+    }
+
+    return -1;
+}
+
 static int wb_make_fixture(WbFix* out, const WbSpec* spec)
 {
     Cert* cert;
@@ -2220,7 +2245,9 @@ static int wb_make_fixture(WbFix* out, const WbSpec* spec)
         }
 
         if (spec->zeroSerial) {
+            /* Patched down to 0 after the body is built. */
             XMEMSET(cert->serial, 0, sizeof(cert->serial));
+            cert->serial[0] = 0x01;
             cert->serialSz = 1;
         }
         else {
@@ -2274,6 +2301,9 @@ static int wb_make_fixture(WbFix* out, const WbSpec* spec)
             ret = 0;
         }
     }
+    if (ret == 0 && spec->zeroSerial) {
+        ret = wb_patch_zero_serial(out->der, cert->bodySz);
+    }
     if (ret == 0) {
         bodySz = cert->bodySz;
         ret = wc_SignCert(bodySz, cert->sigType, out->der, WB_FIX_DER_SZ,