Fix bug in ParseCRL_Extensions #8587

lealem47 · 2025-03-24T17:47:14Z

Description

Fix a bug where the idx would get incremented where we don't expect it to be, in the else-if CRL_NUMBER_OID case in ParseCRL_Extensions()
Store crl->crlNumber as a byte array to handle CRL numbers as long as 20 octets or 49 digits (https://datatracker.ietf.org/doc/html/rfc5280#section-5.2.3). This conversion unfortunately makes it so that crlNumber isn't stored when NO_BIG_INT is defined

Fixes #8574 and wolfSSL/wolfCLU#174

Also fixes zd#19611

Testing

Tested using wolfCLU with the CRL files provided in #8574 and wolfSSL/wolfCLU#174

Build wolfSSL with
./configure --enable-wolfclu --enable-crl

Then run wolfCLU with
./wolfssl crl -in crl_extention_test.pem -text

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

SparkiDev · 2025-03-24T21:56:16Z

Using a mp_int is heavy weight for a 20 byte number - can allocate up to 1KB for storage of 20 bytes.
You can store the array of bytes and do a XMEMCMP and add a simple big-endian number larger comparison.

lealem47 · 2025-03-25T14:21:59Z

Jenkins retest this please

wolfssl/wolfcrypt/asn.h

wolfssl/ssl.h

src/crl.c

dgarske

Looks good. Still on fence about backwards compatibility, but okay for now. Over to @SparkiDev

wolfcrypt/src/asn.c

src/crl.c

src/x509.c

lealem47 · 2025-05-13T15:56:19Z

Jenkins retest this please

PRB-generic-config-parser error:

Found unhandled org.jenkinsci.plugins.workflow.support.steps.AgentOfflineException exception:
Unable to create live FilePath for wolf-linux-cloud-node-cps0d5; wolf-linux-cloud-node-cps0d5 was marked offline: Connection was broken

lealem47 · 2025-05-14T23:22:17Z

Jenkins Retest this please

dgarske · 2025-05-15T18:04:36Z

src/crl.c

@@ -645,12 +703,17 @@ static int AddCRL(WOLFSSL_CRL* crl, DecodedCRL* dcrl, const byte* buff,

    for (curr = crl->crlList; curr != NULL; curr = curr->next) {
        if (XMEMCMP(curr->issuerHash, crle->issuerHash, CRL_DIGEST_SIZE) == 0) {
-            if (crle->crlNumber <= curr->crlNumber) {
+            ret = CompareCRLnumber(crle, curr);
+            if (ret == -1 || ret == 0) {


Can you please use the MP_ defines for the checks? Perhaps also update the code comments on CompareCRLnumber. Why is MP_LT allowed? can you please add a code comment?

/** Result of comparison is that the first number is greater than second. */ #define MP_GT 1 /** Result of comparison is they are equal. */ #define MP_EQ 0 /** Result of comparison is that the first number is less than second. */ #define MP_LT (-1)

dgarske · 2025-05-15T18:05:02Z

src/x509.c

@@ -6429,7 +6429,7 @@ static int X509PrintSerial_ex(WOLFSSL_BIO* bio, byte* serial, int sz,
    /* if serial can fit into byte then print on the same line */
    else  {
        if ((scratchLen = XSNPRINTF(
-                 scratch, MAX_WIDTH, " %d (0x%x)\n", serial[0], serial[0]))
+                 scratch, MAX_WIDTH, " %d (0x%x)\n", (char)serial[0], serial[0]))


This line is over 80 chars now.

dgarske · 2025-05-15T18:05:38Z

wolfcrypt/src/asn.c

-                    else if (length == 1) {
-                        dcrl->crlNumber = buf[idx];
-                    }
+                    mp_free(m);


Maybe not call this if ret == MP_INIT_E?

dgarske · 2025-05-15T18:22:40Z

wolfssl/wolfcrypt/asn.h

 #endif
-    int          crlNumber;          /* CRL number extension  */
+    byte    crlNumberSet:1;          /* CRL number set indicator */


Multi-test reports this bit-field issue, however there must be a way to make this work. Perhaps it needs to be unsigned int. Not sure (see other uses to solve)

[allcryptonly-gcc-c89] [14 of 39] [e7860a7e85] configure... real 0m5.702s user 0m3.401s sys 0m2.671s In file included from ./wolfssl/ssl.h:45, from ./wolfssl/openssl/ssl.h:39, from ./wolfssl/openssl/aes.h:44, from conftest.c:73: ./wolfssl/wolfcrypt/asn.h:2705:13: error: type of bit-field 'extAuthKeyIdSet' is a GCC extension [-Werror=pedantic] 2705 | byte extAuthKeyIdSet:1; /* Auth key identifier set indicator */ | ^~~~~~~~~~~~~~~ ./wolfssl/wolfcrypt/asn.h:2707:13: error: type of bit-field 'crlNumberSet' is a GCC extension [-Werror=pedantic] 2707 | byte crlNumberSet:1; /* CRL number set indicator */ | ^~~~~~~~~~~~ cc1: all warnings being treated as errors

douzzer · 2025-05-16T17:57:05Z

src/crl.c

+    mp_int* curr_num = (mp_int*)XMALLOC(sizeof(*curr_num), NULL,
+                        DYNAMIC_TYPE_BIGINT);
+    if (prev_num == NULL || curr_num == NULL) {
+        return MEMORY_E;


This will leak if prev_num was allocated but curr_num failed.

douzzer · 2025-05-16T17:59:11Z

src/crl.c

+#ifdef WOLFSSL_SMALL_STACK
+        XFREE(prev_num, NULL, DYNAMIC_TYPE_BIGINT);
+        XFREE(curr_num, NULL, DYNAMIC_TYPE_BIGINT);
+#endif
+        return BAD_FUNC_ARG;


It would be nice to refactor this func to use an if (ret == 0) cascade, with smallstack cleanup once at the end.

douzzer · 2025-05-16T18:24:03Z

src/crl.c

+{
+    int result = 0;
+#ifdef WOLFSSL_SMALL_STACK
+    mp_int* prev_num = (mp_int*)XMALLOC(sizeof(*prev_num), NULL,


Regarding @SparkiDev 's concern about allocing gobs of memory just to process 20 octets of MPI: It's possible to right-size-allocate the MPI containers with existing macros. You'd use DECL_MP_INT_SIZE(), NEW_MP_INT_SIZE(). INIT_MP_INT_SIZE(), and FREE_MP_INT_SIZE(). These exist with all MPI back ends, though they only right-size on SP and heapmath, not TFM (doesn't matter).

lealem47 self-assigned this Mar 24, 2025

This was referenced Mar 24, 2025

[Bug]: wolfSSL cannot correctly process CRL files with extensions. #8574

Closed

[Bug]: wolfSSL is unable to correctly process the CRL serial number field. wolfSSL/wolfCLU#174

Open

lealem47 assigned wolfSSL-Bot and unassigned lealem47 Mar 24, 2025

lealem47 force-pushed the gh8574 branch from a6894fc to 3372701 Compare March 25, 2025 00:13

dgarske requested a review from SparkiDev March 25, 2025 20:47

dgarske assigned SparkiDev Mar 25, 2025

dgarske requested changes Mar 26, 2025

View reviewed changes

wolfssl/wolfcrypt/asn.h Outdated Show resolved Hide resolved

dgarske assigned lealem47 Mar 26, 2025

dgarske requested changes Mar 26, 2025

View reviewed changes

wolfssl/wolfcrypt/asn.h Outdated Show resolved Hide resolved

wolfssl/ssl.h Outdated Show resolved Hide resolved

lealem47 force-pushed the gh8574 branch from bddf97f to 20147ba Compare March 26, 2025 22:13

dgarske requested changes Mar 26, 2025

View reviewed changes

src/crl.c Outdated Show resolved Hide resolved

lealem47 removed their assignment Mar 26, 2025

dgarske previously approved these changes Mar 26, 2025

View reviewed changes

SparkiDev requested changes Mar 27, 2025

View reviewed changes

wolfcrypt/src/asn.c Outdated Show resolved Hide resolved

SparkiDev assigned lealem47 and unassigned SparkiDev Mar 27, 2025

dgarske unassigned wolfSSL-Bot Mar 28, 2025

lealem47 dismissed dgarske’s stale review via 70a54ff April 1, 2025 23:05

lealem47 force-pushed the gh8574 branch 2 times, most recently from 550ca10 to 70a54ff Compare April 2, 2025 00:28

lealem47 requested a review from SparkiDev April 2, 2025 01:00

lealem47 assigned SparkiDev and unassigned lealem47 Apr 2, 2025

lealem47 force-pushed the gh8574 branch from 70a54ff to 2a68506 Compare April 2, 2025 13:56

SparkiDev assigned lealem47 and unassigned SparkiDev Apr 3, 2025

lealem47 assigned SparkiDev May 6, 2025

dgarske requested changes May 6, 2025

View reviewed changes

src/crl.c Outdated Show resolved Hide resolved

src/x509.c Outdated Show resolved Hide resolved

dgarske assigned lealem47 and unassigned SparkiDev May 6, 2025

lealem47 requested a review from dgarske May 9, 2025 23:05

lealem47 removed their assignment May 9, 2025

SparkiDev assigned lealem47 May 11, 2025

dgarske approved these changes May 12, 2025

View reviewed changes

lealem47 removed their assignment May 13, 2025

SparkiDev assigned lealem47 May 13, 2025

lealem47 added 10 commits May 14, 2025 13:39

Fix bug in ParseCRL_Extensions

876c08d

Store crlNumber as a byte array

de06c9a

Add appropriate HAVE_CRL guards

319094d

Move bit-fields to the end

e6acc02

Adhere to coding standard

34132f8

Store crlNumber as hex

e82fe4a

Print serial number as signed char

93eb6cc

Only copy crl num if set

dc85016

Address review feedback

3a8fa59

Properly address feedback

e7860a7

lealem47 force-pushed the gh8574 branch from 5a8106d to e7860a7 Compare May 14, 2025 19:39

lealem47 removed their assignment May 15, 2025

dgarske requested changes May 15, 2025

View reviewed changes

dgarske assigned lealem47 May 15, 2025

dgarske requested changes May 15, 2025

View reviewed changes

douzzer requested changes May 16, 2025

View reviewed changes

douzzer assigned douzzer and unassigned wolfSSL-Bot May 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in ParseCRL_Extensions #8587

Fix bug in ParseCRL_Extensions #8587

lealem47 commented Mar 24, 2025 •

edited

Loading

SparkiDev commented Mar 24, 2025

lealem47 commented Mar 25, 2025

dgarske left a comment

lealem47 commented May 13, 2025

lealem47 commented May 14, 2025 •

edited

Loading

dgarske May 15, 2025

dgarske May 15, 2025

dgarske May 15, 2025

dgarske May 15, 2025

douzzer May 16, 2025

douzzer May 16, 2025

douzzer May 16, 2025

Fix bug in ParseCRL_Extensions #8587

Are you sure you want to change the base?

Fix bug in ParseCRL_Extensions #8587

Conversation

lealem47 commented Mar 24, 2025 • edited Loading

Description

Testing

Checklist

SparkiDev commented Mar 24, 2025

lealem47 commented Mar 25, 2025

dgarske left a comment

Choose a reason for hiding this comment

lealem47 commented May 13, 2025

lealem47 commented May 14, 2025 • edited Loading

dgarske May 15, 2025

Choose a reason for hiding this comment

dgarske May 15, 2025

Choose a reason for hiding this comment

dgarske May 15, 2025

Choose a reason for hiding this comment

dgarske May 15, 2025

Choose a reason for hiding this comment

douzzer May 16, 2025

Choose a reason for hiding this comment

douzzer May 16, 2025

Choose a reason for hiding this comment

douzzer May 16, 2025

Choose a reason for hiding this comment

lealem47 commented Mar 24, 2025 •

edited

Loading

lealem47 commented May 14, 2025 •

edited

Loading