Skip to content

Commit 558ae34

Browse files
LinuxJediLinuxJedi
committed
Fix CRL_STATIC_REVOKED_LIST binary search bugs in FindRevokedSerial
The CRL_STATIC_REVOKED_LIST code path stored revoked certificates in a fixed array but never sorted it after parsing, causing binary search to silently miss revoked serials when entries arrived in non-sorted wire order. Additionally, comparisons used rc[0].serialSz instead of rc[mid].serialSz, omitted the length-equality check before XMEMCMP, and ignored the serialHash lookup path entirely (causing a NULL dereference when hash-based lookup was used). Fixes: - Sort the revoked cert array in InitCRL_Entry after populating it - Use rc[mid].serialSz instead of rc->serialSz in binary search - Add serialSz equality check before XMEMCMP, matching linked-list path - Implement serialHash-based linear scan for hash lookup callers Add unit test that loads a CRL with serials in unsorted wire order and verifies that a revoked certificate is correctly detected.
1 parent 9ed2f4b commit 558ae34

3 files changed

Lines changed: 190 additions & 17 deletions

File tree

‎src/crl.c‎

Lines changed: 76 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,35 @@ int InitCRL(WOLFSSL_CRL* crl, WOLFSSL_CERT_MANAGER* cm)
9898
}
9999

100100

101+
#ifdef CRL_STATIC_REVOKED_LIST
102+
/* Compare two RevokedCert entries by (serialSz, serialNumber) for sorting.
103+
* Returns < 0, 0, or > 0 like memcmp. */
104+
static int CompareRevokedCert(const RevokedCert* a, const RevokedCert* b)
105+
{
106+
if (a->serialSz != b->serialSz)
107+
return a->serialSz - b->serialSz;
108+
return XMEMCMP(a->serialNumber, b->serialNumber, (size_t)a->serialSz);
109+
}
110+
111+
/* Sort revoked cert array in-place using insertion sort. The array is bounded
112+
* by CRL_MAX_REVOKED_CERTS so O(n^2) is fine. */
113+
static void SortCRL_CertList(RevokedCert* certs, int totalCerts)
114+
{
115+
int i, j;
116+
RevokedCert tmp;
117+
118+
for (i = 1; i < totalCerts; i++) {
119+
XMEMCPY(&tmp, &certs[i], sizeof(RevokedCert));
120+
j = i - 1;
121+
while (j >= 0 && CompareRevokedCert(&certs[j], &tmp) > 0) {
122+
XMEMCPY(&certs[j + 1], &certs[j], sizeof(RevokedCert));
123+
j--;
124+
}
125+
XMEMCPY(&certs[j + 1], &tmp, sizeof(RevokedCert));
126+
}
127+
}
128+
#endif /* CRL_STATIC_REVOKED_LIST */
129+
101130
/* Initialize CRL Entry */
102131
static int InitCRL_Entry(CRL_Entry* crle, DecodedCRL* dcrl, const byte* buff,
103132
int verified, void* heap)
@@ -132,12 +161,15 @@ static int InitCRL_Entry(CRL_Entry* crle, DecodedCRL* dcrl, const byte* buff,
132161
#endif
133162
#ifdef CRL_STATIC_REVOKED_LIST
134163
/* ParseCRL_CertList() has already cached the Revoked certs into
135-
the crle->certs array */
164+
the crle->certs array. Sort it so binary search in
165+
FindRevokedSerial works correctly. */
166+
crle->totalCerts = dcrl->totalCerts;
167+
SortCRL_CertList(crle->certs, crle->totalCerts);
136168
#else
137169
crle->certs = dcrl->certs; /* take ownership */
170+
crle->totalCerts = dcrl->totalCerts;
138171
#endif
139172
dcrl->certs = NULL;
140-
crle->totalCerts = dcrl->totalCerts;
141173
crle->crlNumberSet = dcrl->crlNumberSet;
142174
if (crle->crlNumberSet) {
143175
XMEMCPY(crle->crlNumber, dcrl->crlNumber, sizeof(crle->crlNumber));
@@ -313,25 +345,52 @@ static int FindRevokedSerial(RevokedCert* rc, byte* serial, int serialSz,
313345
int ret = 0;
314346
byte hash[SIGNER_DIGEST_SIZE];
315347
#ifdef CRL_STATIC_REVOKED_LIST
316-
/* do binary search */
317-
int low, high, mid;
348+
if (serialHash == NULL) {
349+
/* Binary search by (serialSz, serialNumber). The array was sorted in
350+
* InitCRL_Entry by the same comparison key. */
351+
int low = 0;
352+
int high = totalCerts - 1;
318353

319-
low = 0;
320-
high = totalCerts - 1;
354+
while (low <= high) {
355+
int mid = (low + high) / 2;
356+
int cmp;
321357

322-
while (low <= high) {
323-
mid = (low + high) / 2;
358+
/* Compare by serial size first, then by serial content. Shorter
359+
* serials sort before longer ones. */
360+
if (rc[mid].serialSz != serialSz) {
361+
cmp = rc[mid].serialSz - serialSz;
362+
}
363+
else {
364+
cmp = XMEMCMP(rc[mid].serialNumber, serial,
365+
(size_t)rc[mid].serialSz);
366+
}
324367

325-
if (XMEMCMP(rc[mid].serialNumber, serial, rc->serialSz) < 0) {
326-
low = mid + 1;
327-
}
328-
else if (XMEMCMP(rc[mid].serialNumber, serial, rc->serialSz) > 0) {
329-
high = mid - 1;
368+
if (cmp < 0) {
369+
low = mid + 1;
370+
}
371+
else if (cmp > 0) {
372+
high = mid - 1;
373+
}
374+
else {
375+
WOLFSSL_MSG("Cert revoked");
376+
ret = CRL_CERT_REVOKED;
377+
break;
378+
}
330379
}
331-
else {
332-
WOLFSSL_MSG("Cert revoked");
333-
ret = CRL_CERT_REVOKED;
334-
break;
380+
}
381+
else {
382+
/* Hash-based lookup -- linear scan required since the array is sorted
383+
* by serial number, not by hash. */
384+
int i;
385+
for (i = 0; i < totalCerts; i++) {
386+
ret = CalcHashId(rc[i].serialNumber, (word32)rc[i].serialSz, hash);
387+
if (ret != 0)
388+
break;
389+
if (XMEMCMP(hash, serialHash, SIGNER_DIGEST_SIZE) == 0) {
390+
WOLFSSL_MSG("Cert revoked");
391+
ret = CRL_CERT_REVOKED;
392+
break;
393+
}
335394
}
336395
}
337396
#else

‎tests/api/test_certman.c‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1781,6 +1781,118 @@ int test_wolfSSL_CertManagerCRL(void)
17811781
return EXPECT_RESULT();
17821782
}
17831783

1784+
int test_wolfSSL_CRL_static_revoked_list(void)
1785+
{
1786+
EXPECT_DECLS;
1787+
#if defined(CRL_STATIC_REVOKED_LIST) && defined(HAVE_CRL) && \
1788+
!defined(NO_RSA) && !defined(NO_CERTS)
1789+
/* CRL signed by certs/ca-cert.pem that revokes serials 05, 02, 01 in that
1790+
* (unsorted) wire order. The unsorted order exposes a binary search bug in
1791+
* FindRevokedSerial when CRL_STATIC_REVOKED_LIST is enabled: the revoked
1792+
* cert array is never sorted after parsing, so binary search misses entries
1793+
* that are out of order.
1794+
*
1795+
* Generated with Python cryptography library:
1796+
* builder.add_revoked_certificate(serial=5)
1797+
* builder.add_revoked_certificate(serial=2)
1798+
* builder.add_revoked_certificate(serial=1)
1799+
* crl = builder.sign(ca_key, hashes.SHA256())
1800+
*/
1801+
static const unsigned char crl_multi_revoked[] = {
1802+
0x30, 0x82, 0x02, 0x1D, 0x30, 0x82, 0x01, 0x05,
1803+
0x02, 0x01, 0x01, 0x30, 0x0D, 0x06, 0x09, 0x2A,
1804+
0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01, 0x01, 0x0B,
1805+
0x05, 0x00, 0x30, 0x81, 0x94, 0x31, 0x0B, 0x30,
1806+
0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, 0x02,
1807+
0x55, 0x53, 0x31, 0x10, 0x30, 0x0E, 0x06, 0x03,
1808+
0x55, 0x04, 0x08, 0x0C, 0x07, 0x4D, 0x6F, 0x6E,
1809+
0x74, 0x61, 0x6E, 0x61, 0x31, 0x10, 0x30, 0x0E,
1810+
0x06, 0x03, 0x55, 0x04, 0x07, 0x0C, 0x07, 0x42,
1811+
0x6F, 0x7A, 0x65, 0x6D, 0x61, 0x6E, 0x31, 0x11,
1812+
0x30, 0x0F, 0x06, 0x03, 0x55, 0x04, 0x0A, 0x0C,
1813+
0x08, 0x53, 0x61, 0x77, 0x74, 0x6F, 0x6F, 0x74,
1814+
0x68, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55,
1815+
0x04, 0x0B, 0x0C, 0x0A, 0x43, 0x6F, 0x6E, 0x73,
1816+
0x75, 0x6C, 0x74, 0x69, 0x6E, 0x67, 0x31, 0x18,
1817+
0x30, 0x16, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0C,
1818+
0x0F, 0x77, 0x77, 0x77, 0x2E, 0x77, 0x6F, 0x6C,
1819+
0x66, 0x73, 0x73, 0x6C, 0x2E, 0x63, 0x6F, 0x6D,
1820+
0x31, 0x1F, 0x30, 0x1D, 0x06, 0x09, 0x2A, 0x86,
1821+
0x48, 0x86, 0xF7, 0x0D, 0x01, 0x09, 0x01, 0x16,
1822+
0x10, 0x69, 0x6E, 0x66, 0x6F, 0x40, 0x77, 0x6F,
1823+
0x6C, 0x66, 0x73, 0x73, 0x6C, 0x2E, 0x63, 0x6F,
1824+
0x6D, 0x17, 0x0D, 0x32, 0x36, 0x30, 0x31, 0x30,
1825+
0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5A,
1826+
0x17, 0x0D, 0x33, 0x36, 0x30, 0x31, 0x30, 0x31,
1827+
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5A, 0x30,
1828+
0x3C, 0x30, 0x12, 0x02, 0x01, 0x05, 0x17, 0x0D,
1829+
0x32, 0x33, 0x30, 0x31, 0x30, 0x31, 0x30, 0x30,
1830+
0x30, 0x30, 0x30, 0x30, 0x5A, 0x30, 0x12, 0x02,
1831+
0x01, 0x02, 0x17, 0x0D, 0x32, 0x33, 0x30, 0x32,
1832+
0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
1833+
0x5A, 0x30, 0x12, 0x02, 0x01, 0x01, 0x17, 0x0D,
1834+
0x32, 0x33, 0x30, 0x33, 0x30, 0x31, 0x30, 0x30,
1835+
0x30, 0x30, 0x30, 0x30, 0x5A, 0x30, 0x0D, 0x06,
1836+
0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x01,
1837+
0x01, 0x0B, 0x05, 0x00, 0x03, 0x82, 0x01, 0x01,
1838+
0x00, 0x15, 0x9F, 0xC1, 0x9E, 0x17, 0xB3, 0x5A,
1839+
0xF1, 0x48, 0xA5, 0x87, 0x2A, 0x84, 0xD1, 0x93,
1840+
0x8D, 0x19, 0x24, 0xCB, 0xC5, 0x32, 0x56, 0x10,
1841+
0x6C, 0x4D, 0xF5, 0xD1, 0x9A, 0xC0, 0x1A, 0x8B,
1842+
0x1C, 0x84, 0x6B, 0x4B, 0x20, 0xA7, 0xA4, 0x2C,
1843+
0x11, 0x5C, 0x23, 0xBD, 0x0C, 0xB1, 0x33, 0xBE,
1844+
0x38, 0x1B, 0xCB, 0xDB, 0x8E, 0xD4, 0x0F, 0x62,
1845+
0x0D, 0xB5, 0x18, 0x21, 0x28, 0x0B, 0x77, 0xB9,
1846+
0xB4, 0xA8, 0xE9, 0xA0, 0x25, 0x00, 0x83, 0xED,
1847+
0x64, 0x49, 0x8E, 0x52, 0xD9, 0x8D, 0xAF, 0xC2,
1848+
0x16, 0x3E, 0xD3, 0x93, 0x09, 0xB9, 0x18, 0xBB,
1849+
0x6C, 0x41, 0xDF, 0x59, 0x59, 0x53, 0x8C, 0x64,
1850+
0x8B, 0xD1, 0x9D, 0xBB, 0x92, 0x8F, 0xB2, 0x26,
1851+
0x27, 0x78, 0x41, 0xFB, 0xF8, 0xB1, 0x2F, 0x8F,
1852+
0xA1, 0x85, 0xB6, 0xC7, 0x8E, 0x42, 0x72, 0xEF,
1853+
0xF4, 0x3F, 0xC4, 0xAF, 0x40, 0x95, 0xCA, 0x94,
1854+
0xE5, 0x88, 0x89, 0x18, 0x32, 0x54, 0xC3, 0xC4,
1855+
0xBE, 0x7E, 0x48, 0x1B, 0x3D, 0xB3, 0x6C, 0x11,
1856+
0x54, 0x6F, 0x9E, 0xFE, 0x09, 0x5B, 0x72, 0x3F,
1857+
0xD7, 0xA0, 0x02, 0xFF, 0x43, 0x01, 0xFE, 0x23,
1858+
0xF8, 0x72, 0xCD, 0xA9, 0x76, 0x36, 0x31, 0x78,
1859+
0x21, 0xCB, 0x0E, 0xC2, 0x25, 0x8D, 0x0B, 0x4C,
1860+
0x2C, 0xAA, 0x6A, 0x80, 0x6E, 0xE2, 0x1E, 0xAC,
1861+
0x70, 0x5D, 0x4A, 0xAA, 0x56, 0x17, 0xF0, 0x2D,
1862+
0xA2, 0x2A, 0x4E, 0x2B, 0xC8, 0xC9, 0x87, 0x8E,
1863+
0x07, 0xEB, 0xD8, 0x36, 0x42, 0x39, 0xA0, 0xA4,
1864+
0xF6, 0x34, 0xC2, 0x5F, 0xE1, 0x21, 0x07, 0x50,
1865+
0x4B, 0x37, 0x15, 0x7D, 0xF9, 0x18, 0x54, 0x13,
1866+
0xC0, 0x1D, 0x0A, 0x27, 0x3A, 0x63, 0xD2, 0xC3,
1867+
0xD5, 0x57, 0x5E, 0x67, 0x56, 0x65, 0x9E, 0x2E,
1868+
0x4D, 0xB4, 0x96, 0x54, 0x7A, 0x3D, 0xFD, 0xF9,
1869+
0xCF, 0xCD, 0x10, 0x65, 0x05, 0x97, 0x53, 0x72,
1870+
0x12
1871+
};
1872+
WOLFSSL_CERT_MANAGER* cm = NULL;
1873+
1874+
/* Set up CertManager with the CA and CRL checking enabled */
1875+
ExpectNotNull(cm = wolfSSL_CertManagerNew());
1876+
ExpectIntEQ(wolfSSL_CertManagerLoadCA(cm, "./certs/ca-cert.pem", NULL),
1877+
WOLFSSL_SUCCESS);
1878+
ExpectIntEQ(wolfSSL_CertManagerEnableCRL(cm, WOLFSSL_CRL_CHECKALL),
1879+
WOLFSSL_SUCCESS);
1880+
1881+
/* Load the CRL that revokes serials {05, 02, 01} in unsorted wire order */
1882+
ExpectIntEQ(wolfSSL_CertManagerLoadCRLBuffer(cm, crl_multi_revoked,
1883+
sizeof(crl_multi_revoked), WOLFSSL_FILETYPE_ASN1), WOLFSSL_SUCCESS);
1884+
1885+
/* server-cert.pem has serial 01, which is in the CRL but at the last
1886+
* position in the unsorted array. Binary search on unsorted data misses
1887+
* it, so this assertion fails before the bug fix. */
1888+
ExpectIntEQ(wolfSSL_CertManagerCheckCRL(cm, server_cert_der_2048,
1889+
sizeof_server_cert_der_2048), WC_NO_ERR_TRACE(CRL_CERT_REVOKED));
1890+
1891+
wolfSSL_CertManagerFree(cm);
1892+
#endif
1893+
return EXPECT_RESULT();
1894+
}
1895+
17841896
int test_wolfSSL_CRL_duplicate_extensions(void)
17851897
{
17861898
EXPECT_DECLS;

‎tests/api/test_certman.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ int test_wolfSSL_CertManagerNameConstraint3(void);
3636
int test_wolfSSL_CertManagerNameConstraint4(void);
3737
int test_wolfSSL_CertManagerNameConstraint5(void);
3838
int test_wolfSSL_CertManagerCRL(void);
39+
int test_wolfSSL_CRL_static_revoked_list(void);
3940
int test_wolfSSL_CRL_duplicate_extensions(void);
4041
int test_wolfSSL_CertManagerCheckOCSPResponse(void);
4142
int test_various_pathlen_chains(void);
@@ -53,6 +54,7 @@ int test_various_pathlen_chains(void);
5354
TEST_DECL_GROUP("certman", test_wolfSSL_CertManagerNameConstraint4), \
5455
TEST_DECL_GROUP("certman", test_wolfSSL_CertManagerNameConstraint5), \
5556
TEST_DECL_GROUP("certman", test_wolfSSL_CertManagerCRL), \
57+
TEST_DECL_GROUP("certman", test_wolfSSL_CRL_static_revoked_list), \
5658
TEST_DECL_GROUP("certman", test_wolfSSL_CRL_duplicate_extensions), \
5759
TEST_DECL_GROUP("certman", test_wolfSSL_CertManagerCheckOCSPResponse), \
5860
TEST_DECL_GROUP("certman", test_various_pathlen_chains)

0 commit comments

Comments
 (0)