Skip to content

Commit 4cf3384

Browse files
julek-wolfssljulek-wolfssl
committed
Fix multiple bugs in OCSP implementation
- wolfSSL_i2d_OCSP_REQUEST_bio: save/restore pointer before i2d call that advances it, preventing BIO_write from wrong offset and heap corruption on free - wolfSSL_d2i_OCSP_RESPONSE: remove (unsigned char) cast that truncated pointer advance to 8 bits, breaking responses larger than 255 bytes - wolfSSL_OCSP_CERTID_dup: deep-copy CertStatus to prevent double-free when both original and duplicate are freed - wolfSSL_i2d_OCSP_RESPONSE: add NULL check on response parameter - wolfSSL_i2d_OCSP_REQUEST: advance *data pointer per i2d convention - FreeOCSP: NULL-check ocsp->cm before dereferencing for heap - Fix WOLFSSL_LEAVE strings to match actual function names in wc_CheckCertOcspResponse, GetOcspEntry, GetOcspStatus, CheckOcspResponse, CheckOcspRequest Add test for CERTID dup (double-free confirmed under ASAN without fix) and pointer advancement assertions for d2i_OCSP_RESPONSE callers. Reported in: ZD21469
1 parent 24f9981 commit 4cf3384

4 files changed

Lines changed: 105 additions & 13 deletions

File tree

‎src/ocsp.c‎

Lines changed: 49 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ int wc_CheckCertOcspResponse(WOLFSSL_OCSP *ocsp, DecodedCert *cert,
8989
ocspRequest = (OcspRequest*)XMALLOC(sizeof(OcspRequest), NULL,
9090
DYNAMIC_TYPE_TMP_BUFFER);
9191
if (ocspRequest == NULL) {
92-
WOLFSSL_LEAVE("CheckCertOCSP", MEMORY_ERROR);
92+
WOLFSSL_LEAVE("wc_CheckCertOcspResponse", MEMORY_ERROR);
9393
return MEMORY_E;
9494
}
9595
#endif
@@ -105,7 +105,7 @@ int wc_CheckCertOcspResponse(WOLFSSL_OCSP *ocsp, DecodedCert *cert,
105105
XFREE(ocspRequest, NULL, DYNAMIC_TYPE_TMP_BUFFER);
106106
#endif
107107

108-
WOLFSSL_LEAVE("CheckCertOCSP", ret);
108+
WOLFSSL_LEAVE("wc_CheckCertOcspResponse", ret);
109109
return ret;
110110
}
111111

@@ -171,19 +171,20 @@ static void FreeOcspEntry(OcspEntry* entry, void* heap)
171171
void FreeOCSP(WOLFSSL_OCSP* ocsp, int dynamic)
172172
{
173173
OcspEntry *entry, *next;
174+
void* heap = (ocsp->cm != NULL) ? ocsp->cm->heap : NULL;
174175

175176
WOLFSSL_ENTER("FreeOCSP");
176177

177178
for (entry = ocsp->ocspList; entry; entry = next) {
178179
next = entry->next;
179-
FreeOcspEntry(entry, ocsp->cm->heap);
180-
XFREE(entry, ocsp->cm->heap, DYNAMIC_TYPE_OCSP_ENTRY);
180+
FreeOcspEntry(entry, heap);
181+
XFREE(entry, heap, DYNAMIC_TYPE_OCSP_ENTRY);
181182
}
182183

183184
wc_FreeMutex(&ocsp->ocspLock);
184185

185186
if (dynamic)
186-
XFREE(ocsp, ocsp->cm->heap, DYNAMIC_TYPE_OCSP);
187+
XFREE(ocsp, heap, DYNAMIC_TYPE_OCSP);
187188

188189
}
189190

@@ -244,7 +245,7 @@ static int GetOcspEntry(WOLFSSL_OCSP* ocsp, OcspRequest* request,
244245
*entry = NULL;
245246

246247
if (wc_LockMutex(&ocsp->ocspLock) != 0) {
247-
WOLFSSL_LEAVE("CheckCertOCSP", BAD_MUTEX_E);
248+
WOLFSSL_LEAVE("GetOcspEntry", BAD_MUTEX_E);
248249
return BAD_MUTEX_E;
249250
}
250251

@@ -287,7 +288,7 @@ static int GetOcspStatus(WOLFSSL_OCSP* ocsp, OcspRequest* request,
287288
*status = NULL;
288289

289290
if (wc_LockMutex(&ocsp->ocspLock) != 0) {
290-
WOLFSSL_LEAVE("CheckCertOCSP", BAD_MUTEX_E);
291+
WOLFSSL_LEAVE("GetOcspStatus", BAD_MUTEX_E);
291292
return BAD_MUTEX_E;
292293
}
293294

@@ -374,7 +375,7 @@ int CheckOcspResponse(WOLFSSL_OCSP *ocsp, byte *response, int responseSz,
374375
XFREE(newSingle, NULL, DYNAMIC_TYPE_OCSP_ENTRY);
375376
XFREE(ocspResponse, NULL, DYNAMIC_TYPE_OCSP_REQUEST);
376377

377-
WOLFSSL_LEAVE("CheckCertOCSP", MEMORY_ERROR);
378+
WOLFSSL_LEAVE("CheckOcspResponse", MEMORY_ERROR);
378379
return MEMORY_E;
379380
}
380381
#endif
@@ -550,7 +551,7 @@ int CheckOcspRequest(WOLFSSL_OCSP* ocsp, OcspRequest* ocspRequest,
550551

551552
request = (byte*)XMALLOC((size_t)requestSz, ocsp->cm->heap, DYNAMIC_TYPE_OCSP);
552553
if (request == NULL) {
553-
WOLFSSL_LEAVE("CheckCertOCSP", MEMORY_ERROR);
554+
WOLFSSL_LEAVE("CheckOcspRequest", MEMORY_ERROR);
554555
return MEMORY_ERROR;
555556
}
556557

@@ -1285,7 +1286,7 @@ OcspResponse* wolfSSL_d2i_OCSP_RESPONSE(OcspResponse** response,
12851286
}
12861287

12871288
if (GetSequence(*data, &idx, &length, (word32)len) >= 0)
1288-
(*data) += (unsigned char) ((int)idx + length);
1289+
(*data) += idx + length;
12891290

12901291
if (response != NULL && *response == NULL)
12911292
*response = resp;
@@ -1296,6 +1297,9 @@ OcspResponse* wolfSSL_d2i_OCSP_RESPONSE(OcspResponse** response,
12961297
int wolfSSL_i2d_OCSP_RESPONSE(OcspResponse* response,
12971298
unsigned char** data)
12981299
{
1300+
if (response == NULL)
1301+
return BAD_FUNC_ARG;
1302+
12991303
if (data == NULL)
13001304
return (int)response->maxIdx;
13011305

@@ -1366,7 +1370,11 @@ int wolfSSL_i2d_OCSP_REQUEST(OcspRequest* request, unsigned char** data)
13661370
if (size <= 0 || data == NULL)
13671371
return size;
13681372

1369-
return EncodeOcspRequest(request, *data, (word32) size);
1373+
size = EncodeOcspRequest(request, *data, (word32) size);
1374+
if (size > 0)
1375+
*data += size;
1376+
1377+
return size;
13701378
}
13711379

13721380
WOLFSSL_OCSP_ONEREQ* wolfSSL_OCSP_request_add0_id(OcspRequest *req,
@@ -1405,9 +1413,35 @@ WOLFSSL_OCSP_CERTID* wolfSSL_OCSP_CERTID_dup(WOLFSSL_OCSP_CERTID* id)
14051413

14061414
certId = (WOLFSSL_OCSP_CERTID*)XMALLOC(sizeof(WOLFSSL_OCSP_CERTID),
14071415
NULL, DYNAMIC_TYPE_OPENSSL);
1408-
if (certId) {
1409-
XMEMCPY(certId, id, sizeof(WOLFSSL_OCSP_CERTID));
1416+
if (certId == NULL)
1417+
return NULL;
1418+
1419+
XMEMCPY(certId, id, sizeof(WOLFSSL_OCSP_CERTID));
1420+
certId->next = NULL;
1421+
certId->rawCertId = NULL;
1422+
certId->rawCertIdSize = 0;
1423+
1424+
/* Deep-copy the status to avoid double-free */
1425+
if (id->status != NULL) {
1426+
certId->status = (CertStatus*)XMALLOC(sizeof(CertStatus),
1427+
NULL, DYNAMIC_TYPE_OCSP_STATUS);
1428+
if (certId->status == NULL) {
1429+
XFREE(certId, NULL, DYNAMIC_TYPE_OPENSSL);
1430+
return NULL;
1431+
}
1432+
XMEMCPY(certId->status, id->status, sizeof(CertStatus));
1433+
certId->status->next = NULL;
1434+
/* Don't share dynamically allocated fields */
1435+
certId->status->rawOcspResponse = NULL;
1436+
certId->status->rawOcspResponseSz = 0;
1437+
certId->status->serialInt = NULL;
1438+
#ifdef WOLFSSL_OCSP_PARSE_STATUS
1439+
certId->status->thisDateAsn = NULL;
1440+
certId->status->nextDateAsn = NULL;
1441+
#endif
14101442
}
1443+
certId->ownStatus = 1;
1444+
14111445
return certId;
14121446
}
14131447

@@ -1429,7 +1463,9 @@ int wolfSSL_i2d_OCSP_REQUEST_bio(WOLFSSL_BIO* out,
14291463
}
14301464

14311465
if (data != NULL) {
1466+
unsigned char* dataOrig = data;
14321467
size = wolfSSL_i2d_OCSP_REQUEST(req, &data);
1468+
data = dataOrig;
14331469
}
14341470

14351471
if (size <= 0) {

‎tests/api.c‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2989,6 +2989,7 @@ static int test_wolfSSL_CheckOCSPResponse(void)
29892989

29902990
pt = data;
29912991
ExpectNotNull(res = wolfSSL_d2i_OCSP_RESPONSE(NULL, &pt, dataSz));
2992+
ExpectPtrEq(pt, data + dataSz);
29922993
ExpectNotNull(issuer = wolfSSL_X509_load_certificate_file(caFile,
29932994
SSL_FILETYPE_PEM));
29942995
ExpectNotNull(st = wolfSSL_X509_STORE_new());
@@ -3013,6 +3014,7 @@ static int test_wolfSSL_CheckOCSPResponse(void)
30133014

30143015
pt = data;
30153016
ExpectNotNull(res = wolfSSL_d2i_OCSP_RESPONSE(NULL, &pt, dataSz));
3017+
ExpectPtrEq(pt, data + dataSz);
30163018
wolfSSL_OCSP_RESPONSE_free(res);
30173019
res = NULL;
30183020

@@ -3124,6 +3126,7 @@ static int test_wolfSSL_CheckOCSPResponse(void)
31243126

31253127
pt = data;
31263128
ExpectNotNull(res = wolfSSL_d2i_OCSP_RESPONSE(NULL, &pt, dataSz));
3129+
ExpectPtrEq(pt, data + dataSz);
31273130

31283131
/* try to verify the response */
31293132
ExpectNotNull(issuer = wolfSSL_X509_load_certificate_file(caFile,
@@ -35448,6 +35451,7 @@ TEST_CASE testCases[] = {
3544835451
TEST_DECL(test_ocsp_basic_verify),
3544935452
TEST_DECL(test_ocsp_response_parsing),
3545035453
TEST_DECL(test_ocsp_certid_enc_dec),
35454+
TEST_DECL(test_ocsp_certid_dup),
3545135455
TEST_DECL(test_ocsp_tls_cert_cb),
3545235456
TEST_DECL(test_ocsp_cert_unknown_crl_fallback),
3545335457
TEST_DECL(test_ocsp_cert_unknown_crl_fallback_nonleaf),

‎tests/api/test_ocsp.c‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@ int test_ocsp_basic_verify(void)
215215
ptr = (const unsigned char*)resp;
216216
ExpectNotNull(
217217
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp)));
218+
ExpectPtrEq(ptr, (const unsigned char*)resp + sizeof(resp));
218219
ExpectIntEQ(response->responseStatus, 0);
219220
ExpectIntEQ(response->responderIdType, OCSP_RESPONDER_ID_NAME);
220221
ExpectBufEQ(response->responderId.nameHash, cert.subjectHash,
@@ -225,6 +226,8 @@ int test_ocsp_basic_verify(void)
225226
ptr = (const unsigned char*)resp_rid_bykey;
226227
ExpectNotNull(response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr,
227228
sizeof(resp_rid_bykey)));
229+
ExpectPtrEq(ptr, (const unsigned char*)resp_rid_bykey +
230+
sizeof(resp_rid_bykey));
228231
ExpectIntEQ(response->responseStatus, 0);
229232
ExpectIntEQ(response->responderIdType, OCSP_RESPONDER_ID_KEY);
230233
ExpectBufEQ(response->responderId.keyHash, cert.subjectKeyHash,
@@ -235,6 +238,7 @@ int test_ocsp_basic_verify(void)
235238
ptr = (const unsigned char*)resp_nocert;
236239
ExpectNotNull(
237240
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp_nocert)));
241+
ExpectPtrEq(ptr, (const unsigned char*)resp_nocert + sizeof(resp_nocert));
238242
ExpectIntEQ(response->responseStatus, 0);
239243
wolfSSL_OCSP_RESPONSE_free(response);
240244

@@ -246,6 +250,7 @@ int test_ocsp_basic_verify(void)
246250
ptr = (const unsigned char*)resp;
247251
ExpectNotNull(
248252
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp)));
253+
ExpectPtrEq(ptr, (const unsigned char*)resp + sizeof(resp));
249254
/* no verify signer certificate */
250255
ExpectIntEQ(wolfSSL_OCSP_basic_verify(response, NULL, NULL, OCSP_NOVERIFY),
251256
WOLFSSL_SUCCESS);
@@ -272,6 +277,7 @@ int test_ocsp_basic_verify(void)
272277
ptr = (const unsigned char*)resp_nocert;
273278
ExpectNotNull(
274279
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp_nocert)));
280+
ExpectPtrEq(ptr, (const unsigned char*)resp_nocert + sizeof(resp_nocert));
275281
ExpectIntEQ(wolfSSL_OCSP_basic_verify(response, certs, store, 0),
276282
WOLFSSL_SUCCESS);
277283
wolfSSL_OCSP_RESPONSE_free(response);
@@ -281,6 +287,7 @@ int test_ocsp_basic_verify(void)
281287
ptr = (const unsigned char*)resp;
282288
ExpectNotNull(
283289
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp)));
290+
ExpectPtrEq(ptr, (const unsigned char*)resp + sizeof(resp));
284291
ExpectIntEQ(wolfSSL_OCSP_basic_verify(response, NULL, store, 0),
285292
WOLFSSL_SUCCESS);
286293
/* make invalid signature */
@@ -311,6 +318,7 @@ int test_ocsp_basic_verify(void)
311318
ptr = (const unsigned char*)resp_nocert;
312319
ExpectNotNull(
313320
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp_nocert)));
321+
ExpectPtrEq(ptr, (const unsigned char*)resp_nocert + sizeof(resp_nocert));
314322
ExpectIntNE(wolfSSL_OCSP_basic_verify(response, NULL, store, 0),
315323
WOLFSSL_SUCCESS);
316324
wolfSSL_OCSP_RESPONSE_free(response);
@@ -332,6 +340,7 @@ int test_ocsp_basic_verify(void)
332340
ptr = (const unsigned char*)resp_multi;
333341
ExpectNotNull(
334342
response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr, sizeof(resp_multi)));
343+
ExpectPtrEq(ptr, (const unsigned char*)resp_multi + sizeof(resp_multi));
335344
ExpectIntEQ(wolfSSL_OCSP_basic_verify(response, certs, store, 0),
336345
WOLFSSL_SUCCESS);
337346
wolfSSL_OCSP_RESPONSE_free(response);
@@ -342,6 +351,8 @@ int test_ocsp_basic_verify(void)
342351
ptr = (const unsigned char*)resp_bad_noauth;
343352
ExpectNotNull(response = wolfSSL_d2i_OCSP_RESPONSE(NULL, &ptr,
344353
sizeof(resp_bad_noauth)));
354+
ExpectPtrEq(ptr, (const unsigned char*)resp_bad_noauth +
355+
sizeof(resp_bad_noauth));
345356

346357
expectedRet = WOLFSSL_FAILURE;
347358
#ifdef WOLFSSL_NO_OCSP_ISSUER_CHECK
@@ -665,11 +676,51 @@ int test_ocsp_certid_enc_dec(void)
665676
wolfSSL_X509_free(issuer);
666677
return EXPECT_SUCCESS();
667678
}
679+
int test_ocsp_certid_dup(void)
680+
{
681+
EXPECT_DECLS;
682+
WOLFSSL_OCSP_CERTID* certId = NULL;
683+
WOLFSSL_OCSP_CERTID* certIdDup = NULL;
684+
WOLFSSL_X509* subject = NULL;
685+
WOLFSSL_X509* issuer = NULL;
686+
687+
/* Load test certificates */
688+
ExpectNotNull(
689+
subject = wolfSSL_X509_load_certificate_file(
690+
"./certs/ocsp/intermediate1-ca-cert.pem", WOLFSSL_FILETYPE_PEM));
691+
ExpectNotNull(issuer = wolfSSL_X509_load_certificate_file(
692+
"./certs/ocsp/root-ca-cert.pem", WOLFSSL_FILETYPE_PEM));
693+
694+
/* Create CERTID from certificates */
695+
ExpectNotNull(certId = wolfSSL_OCSP_cert_to_id(NULL, subject, issuer));
696+
697+
/* Dup */
698+
ExpectNotNull(certIdDup = wolfSSL_OCSP_CERTID_dup(certId));
699+
700+
/* Verify the dup compares equal */
701+
ExpectIntEQ(wolfSSL_OCSP_id_cmp(certId, certIdDup), 0);
702+
703+
/* Verify status is a distinct allocation (deep copy) */
704+
ExpectPtrNE(certId->status, certIdDup->status);
705+
706+
/* Freeing both must not double-free (ASAN will catch it) */
707+
wolfSSL_OCSP_CERTID_free(certId);
708+
wolfSSL_OCSP_CERTID_free(certIdDup);
709+
710+
wolfSSL_X509_free(subject);
711+
wolfSSL_X509_free(issuer);
712+
return EXPECT_SUCCESS();
713+
}
714+
668715
#else /* !NO_SHA && OPENSSL_ALL && HAVE_OCSP && !WOLFSSL_SM3 && !WOLFSSL_SM2 */
669716
int test_ocsp_certid_enc_dec(void)
670717
{
671718
return TEST_SKIPPED;
672719
}
720+
int test_ocsp_certid_dup(void)
721+
{
722+
return TEST_SKIPPED;
723+
}
673724
#endif
674725

675726
#if defined(HAVE_OCSP) && defined(WOLFSSL_CERT_SETUP_CB) && \

‎tests/api/test_ocsp.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#define WOLFSSL_TEST_OCSP_H
2424

2525
int test_ocsp_certid_enc_dec(void);
26+
int test_ocsp_certid_dup(void);
2627
int test_ocsp_status_callback(void);
2728
int test_ocsp_basic_verify(void);
2829
int test_ocsp_response_parsing(void);

0 commit comments

Comments
 (0)