fix: chain static CRL revoked list

Author: MarkAtwood

.github/configs/os-check-linux.json

@@ -601,5 +601,8 @@
  "run": [["./wolfcrypt/test/testwolfcrypt"]]},
 {"name": "postauth-no-compat",
  "comment": "Post-handshake auth without OPENSSL_EXTRA, which otherwise compiles out the handshake-end unload of SSL-owned certs and keys.",
- "configure": ["--enable-postauth", "--enable-sessioncerts"]}
+ "configure": ["--enable-postauth", "--enable-sessioncerts"]},
+{"name": "all-crl-static-revoked-list",
+ "comment": "CRL_STATIC_REVOKED_LIST swaps the revoked cert linked list for a fixed sorted array. Nothing else in this matrix defines it, so the static list tests in tests/api/test_certman.c and the compat layer CRL code on the array (print, get_REVOKED, CRL generation) are built nowhere else.",
+ "configure": ["--enable-all", "CPPFLAGS=-DCRL_STATIC_REVOKED_LIST"]}
 ]

src/crl.c

@@ -130,7 +130,9 @@ static int CompareRevokedCert(const RevokedCert* a, const RevokedCert* b)
 }
 
 /* Sort revoked cert array in-place using insertion sort. The array is bounded
- * by CRL_MAX_REVOKED_CERTS so O(n^2) is fine. */
+ * by CRL_MAX_REVOKED_CERTS so O(n^2) is fine. Then chain the entries through
+ * next, so code that walks the revoked list (wc_MakeCRL_ex,
+ * wolfSSL_X509_CRL_print, wolfSSL_X509_CRL_get_REVOKED) sees every entry. */
 static void SortCRL_CertList(RevokedCert* certs, int totalCerts)
 {
     int i, j;
@@ -145,6 +147,9 @@ static void SortCRL_CertList(RevokedCert* certs, int totalCerts)
         }
         XMEMCPY(&certs[j + 1], &tmp, sizeof(RevokedCert));
     }
+    for (i = 0; i < totalCerts; i++) {
+        certs[i].next = (i + 1 < totalCerts) ? &certs[i + 1] : NULL;
+    }
 }
 #endif /* CRL_STATIC_REVOKED_LIST */
 
@@ -1417,6 +1422,8 @@ static int DupRevokedCertList(CRL_Entry* dupl, const CRL_Entry* ent, void* heap)
         if (DupRevokedCert(&dupl->certs[i], &ent->certs[i], heap) != 0)
             return MEMORY_E;
     }
+    /* DupRevokedCert() cleared next, relink within dupl's own array */
+    SortCRL_CertList(dupl->certs, ent->totalCerts);
 #else
     RevokedCert* current;
     RevokedCert* prev = NULL;
@@ -2594,7 +2601,9 @@ int wolfSSL_X509_CRL_add_revoked(WOLFSSL_X509_CRL* crl,
 {
     CRL_Entry* entry;
     RevokedCert* rc;
+#ifndef CRL_STATIC_REVOKED_LIST
     RevokedCert* curr;
+#endif
 
     WOLFSSL_ENTER("wolfSSL_X509_CRL_add_revoked");
 
@@ -2611,6 +2620,12 @@ int wolfSSL_X509_CRL_add_revoked(WOLFSSL_X509_CRL* crl,
     if (entry == NULL) {
         return BAD_FUNC_ARG;
     }
+#ifdef CRL_STATIC_REVOKED_LIST
+    /* Same limit and error as the parser, GetRevoked() */
+    if (entry->totalCerts >= CRL_MAX_REVOKED_CERTS) {
+        return MEMORY_E;
+    }
+#endif
 
     {
         const byte* serial = rev->serialNumber->data;
@@ -2669,6 +2684,13 @@ int wolfSSL_X509_CRL_add_revoked(WOLFSSL_X509_CRL* crl,
     rc->reasonCode = rev->reason;
     rc->next = NULL;
 
+#ifdef CRL_STATIC_REVOKED_LIST
+    /* Copy into the fixed array and keep it sorted for FindRevokedSerial */
+    XMEMCPY(&entry->certs[entry->totalCerts], rc, sizeof(RevokedCert));
+    XFREE(rc, crl->heap, DYNAMIC_TYPE_REVOKED);
+    entry->totalCerts++;
+    SortCRL_CertList(entry->certs, entry->totalCerts);
+#else
     /* Add to end of list */
     if (entry->certs == NULL) {
         entry->certs = rc;
@@ -2679,6 +2701,7 @@ int wolfSSL_X509_CRL_add_revoked(WOLFSSL_X509_CRL* crl,
         curr->next = rc;
     }
     entry->totalCerts++;
+#endif
 
     /* Invalidate cached STACK_OF(X509_REVOKED) since list changed */
     if (crl->revokedStack != NULL) {

tests/api.c

@@ -30412,6 +30412,30 @@ static int test_wolfSSL_X509_CRL_reason_critical_boolean(void)
     defined(HAVE_CRL) && !defined(NO_FILESYSTEM) && \
     !defined(NO_STDIO_FILESYSTEM) && defined(WOLFSSL_CERT_GEN) && \
     !defined(NO_ASN_TIME)
+#ifdef CRL_STATIC_REVOKED_LIST
+/* The static revoked list is kept sorted, not in add order, so match a decoded
+ * entry against the serials not matched yet, want[n] being the cert serial.
+ * Returns 1 when it matched one. */
+static int crl_match_revoked(const RevokedCert* rev, byte want[][1], int n,
+                             const byte* certSerial, int certSerialSz,
+                             int* used)
+{
+    int k;
+
+    for (k = 0; k <= n; k++) {
+        const byte* s = (k < n) ? want[k] : certSerial;
+        int sz = (k < n) ? (int)sizeof(want[0]) : certSerialSz;
+
+        if ((*used & (1 << k)) == 0 && rev->serialSz == sz &&
+                XMEMCMP(rev->serialNumber, s, (size_t)sz) == 0) {
+            *used |= 1 << k;
+            return 1;
+        }
+    }
+    return 0;
+}
+#endif
+
 /* Helper function to create, sign, and write a CRL */
 static int generate_crl_test(const char* keyFile, const char* certFile,
                              const char* derFile, const char* pemFile,
@@ -30580,7 +30604,14 @@ static int generate_crl_test(const char* keyFile, const char* certFile,
     revokedCount = 0;
     if (EXPECT_SUCCESS() && decodedCrl != NULL && decodedCrl->crlList != NULL) {
         RevokedCert* rev = decodedCrl->crlList->certs;
+#ifdef CRL_STATIC_REVOKED_LIST
+        int used = 0;
+#endif
         while (rev != NULL) {
+#ifdef CRL_STATIC_REVOKED_LIST
+            ExpectIntEQ(crl_match_revoked(rev, serialsToRevokeBytes,
+                numSerials, certSerial, certSerialSz, &used), 1);
+#else
             if (revokedCount == 0) {
                 /* Last added (cert serial) appears first in decoded list */
                 ExpectIntEQ(rev->serialSz, certSerialSz);
@@ -30595,6 +30626,7 @@ static int generate_crl_test(const char* keyFile, const char* certFile,
                 ExpectIntEQ(XMEMCMP(rev->serialNumber,
                     serialsToRevokeBytes[idx], rev->serialSz), 0);
             }
+#endif
             revokedCount++;
             rev = rev->next;
         }
@@ -30625,7 +30657,14 @@ static int generate_crl_test(const char* keyFile, const char* certFile,
     revokedCount = 0;
     if (EXPECT_SUCCESS() && decodedCrl != NULL && decodedCrl->crlList != NULL) {
         RevokedCert* rev = decodedCrl->crlList->certs;
+#ifdef CRL_STATIC_REVOKED_LIST
+        int used = 0;
+#endif
         while (rev != NULL) {
+#ifdef CRL_STATIC_REVOKED_LIST
+            ExpectIntEQ(crl_match_revoked(rev, serialsToRevokeBytes,
+                numSerials, certSerial, certSerialSz, &used), 1);
+#else
             if (revokedCount == 0) {
                 ExpectIntEQ(rev->serialSz, certSerialSz);
                 ExpectIntEQ(XMEMCMP(rev->serialNumber, certSerial,
@@ -30638,6 +30677,7 @@ static int generate_crl_test(const char* keyFile, const char* certFile,
                 ExpectIntEQ(XMEMCMP(rev->serialNumber,
                     serialsToRevokeBytes[idx], rev->serialSz), 0);
             }
+#endif
             revokedCount++;
             rev = rev->next;
         }
@@ -30713,6 +30753,12 @@ static int test_wolfSSL_X509_CRL_sign_large(void)
     XFILE fp = XBADFILE;
     int i;
     byte serial[4];
+#if defined(CRL_STATIC_REVOKED_LIST) && CRL_MAX_REVOKED_CERTS < 1024
+    /* The static list holds at most CRL_MAX_REVOKED_CERTS entries */
+    const int numRevoked = CRL_MAX_REVOKED_CERTS;
+#else
+    const int numRevoked = 1024;
+#endif
 
     ExpectTrue((fp = XFOPEN(testRsaCertFile, "rb")) != XBADFILE);
     if (fp != XBADFILE) {
@@ -30743,14 +30789,22 @@ static int test_wolfSSL_X509_CRL_sign_large(void)
         revoked.serialNumber->length = (int)sizeof(serial);
     }
 
-    for (i = 1; i <= 1024; i++) {
+    for (i = 1; i <= numRevoked; i++) {
         serial[0] = (byte)(i & 0xff);
         serial[1] = (byte)((i >> 8) & 0xff);
         serial[2] = (byte)((i >> 16) & 0xff);
         serial[3] = (byte)((i >> 24) & 0xff);
         ExpectIntEQ(wolfSSL_X509_CRL_add_revoked(crl, &revoked),
             WOLFSSL_SUCCESS);
     }
+#ifdef CRL_STATIC_REVOKED_LIST
+    if (numRevoked == CRL_MAX_REVOKED_CERTS) {
+        /* Full, so the next one is refused the same way the parser does */
+        serial[0] = (byte)(i & 0xff);
+        ExpectIntEQ(wolfSSL_X509_CRL_add_revoked(crl, &revoked),
+            WC_NO_ERR_TRACE(MEMORY_E));
+    }
+#endif
 
     ExpectTrue((fp = XFOPEN(testRsaKeyFile, "rb")) != XBADFILE);
     if (fp != XBADFILE) {

tests/api/test_certman.c

@@ -2648,6 +2648,34 @@ int test_wolfSSL_CRL_static_revoked_list(void)
         sizeof_server_cert_der_2048), WC_NO_ERR_TRACE(CRL_CERT_REVOKED));
 
     wolfSSL_CertManagerFree(cm);
+
+#if defined(OPENSSL_EXTRA) && !defined(NO_BIO) && defined(XSNPRINTF) && \
+    !defined(NO_FILESYSTEM) && !defined(NO_STDIO_FILESYSTEM)
+    {
+        /* The compat layer walks the revoked list through next, so the static
+         * array has to be chained for every entry, not just the first. Before
+         * the fix get_REVOKED returned 1 entry and X509_CRL_print crashed. */
+        WOLFSSL_X509_CRL* crl = NULL;
+        WOLFSSL_BIO* bio = NULL;
+        char* out = NULL;
+
+        ExpectNotNull(crl = wolfSSL_d2i_X509_CRL(NULL, crl_multi_revoked,
+            (int)sizeof(crl_multi_revoked)));
+        ExpectIntEQ(wolfSSL_sk_X509_REVOKED_num(
+            wolfSSL_X509_CRL_get_REVOKED(crl)), 3);
+
+        ExpectNotNull(bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem()));
+        ExpectIntEQ(wolfSSL_X509_CRL_print(bio, crl), WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_BIO_write(bio, "", 1), 1);
+        ExpectIntGT(wolfSSL_BIO_get_mem_data(bio, &out), 0);
+        ExpectNotNull(out ? XSTRSTR(out, "Serial Number: 1 (0x1)") : NULL);
+        ExpectNotNull(out ? XSTRSTR(out, "Serial Number: 2 (0x2)") : NULL);
+        ExpectNotNull(out ? XSTRSTR(out, "Serial Number: 5 (0x5)") : NULL);
+
+        wolfSSL_BIO_free(bio);
+        wolfSSL_X509_CRL_free(crl);
+    }
+#endif
 #endif
     return EXPECT_RESULT();
 }