Skip to content

Commit b53db94

Browse files
kareem-wolfsslkareem-wolfssl
committed
x509_verify_cert: Code review feedback.
1 parent aa6f1b2 commit b53db94

File tree

5 files changed

+99
-110
lines changed

5 files changed

+99
-110
lines changed

‎src/ssl.c‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6147,23 +6147,23 @@ int AddCA(WOLFSSL_CERT_MANAGER* cm, DerBuffer** pDer, int type, int verify)
61476147
int RemoveCA(WOLFSSL_CERT_MANAGER* cm, byte* hash, int type)
61486148
{
61496149
Signer* current;
6150-
Signer* prev;
6150+
Signer** prev;
61516151
int ret = WC_NO_ERR_TRACE(WOLFSSL_FAILURE);
61526152
word32 row;
61536153

61546154
WOLFSSL_MSG("Removing a CA");
61556155

61566156
if (cm == NULL || hash == NULL) {
6157-
return ret;
6157+
return BAD_FUNC_ARG;
61586158
}
61596159

61606160
row = HashSigner(hash);
61616161

61626162
if (wc_LockMutex(&cm->caLock) != 0) {
6163-
return ret;
6163+
return BAD_MUTEX_E;
61646164
}
61656165
current = cm->caTable[row];
6166-
prev = current;
6166+
prev = &cm->caTable[row];
61676167
while (current) {
61686168
byte* subjectHash;
61696169

@@ -6175,17 +6175,12 @@ int RemoveCA(WOLFSSL_CERT_MANAGER* cm, byte* hash, int type)
61756175

61766176
if ((current->type == type) &&
61776177
(XMEMCMP(hash, subjectHash, SIGNER_DIGEST_SIZE) == 0)) {
6178-
if (current == cm->caTable[row]) {
6179-
cm->caTable[row] = cm->caTable[row]->next;
6180-
}
6181-
else {
6182-
prev->next = current->next;
6183-
}
6178+
*prev = current->next;
61846179
FreeSigner(current, cm->heap);
61856180
ret = WOLFSSL_SUCCESS;
61866181
break;
61876182
}
6188-
prev = current;
6183+
prev = &current->next;
61896184
current = current->next;
61906185
}
61916186
wc_UnLockMutex(&cm->caLock);

‎src/x509_str.c‎

Lines changed: 84 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,8 @@
3434
#ifdef OPENSSL_EXTRA
3535
static int X509StoreGetIssuerEx(WOLFSSL_X509 **issuer,
3636
WOLFSSL_STACK *certs, WOLFSSL_X509 *x);
37-
static int X509StorePopCert(WOLFSSL_STACK *certs_stack,
38-
WOLFSSL_STACK *dest_stack,
39-
WOLFSSL_X509 *cert);
4037
static int X509StoreAddCa(WOLFSSL_X509_STORE* store,
4138
WOLFSSL_X509* x509, int type);
42-
static int X509StoreRemoveCa(WOLFSSL_X509_STORE* store,
43-
WOLFSSL_X509* x509, int type);
4439
#endif
4540

4641
/* Based on OpenSSL default max depth */
@@ -409,6 +404,64 @@ static int addAllButSelfSigned(WOLF_STACK_OF(WOLFSSL_X509)*to,
409404
return ret;
410405
}
411406

407+
static int X509StoreRemoveCa(WOLFSSL_X509_STORE* store,
408+
WOLFSSL_X509* x509, int type) {
409+
int result = WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR);
410+
byte hash[KEYID_SIZE];
411+
412+
if (store != NULL && x509 != NULL && x509->derCert != NULL) {
413+
result = GetHashId(x509->subjKeyId, (int)x509->subjKeyIdSz,
414+
hash, HashIdAlg(x509->sigOID));
415+
if (result) {
416+
result = WOLFSSL_FATAL_ERROR;
417+
} else {
418+
result = RemoveCA(store->cm, hash, type);
419+
}
420+
}
421+
422+
return result;
423+
}
424+
425+
static int X509StoreMoveCert(WOLFSSL_STACK *certs_stack,
426+
WOLFSSL_STACK *dest_stack,
427+
WOLFSSL_X509 *cert) {
428+
int i;
429+
430+
if (certs_stack == NULL || dest_stack == NULL || cert == NULL)
431+
return WOLFSSL_FATAL_ERROR;
432+
433+
for (i = 0; i < wolfSSL_sk_X509_num(certs_stack); i++) {
434+
if (wolfSSL_sk_X509_value(certs_stack, i) == cert) {
435+
wolfSSL_sk_X509_push(dest_stack,
436+
(WOLFSSL_X509*)wolfSSL_sk_pop_node(certs_stack, i));
437+
return WOLFSSL_SUCCESS;
438+
}
439+
}
440+
441+
return WOLFSSL_FAILURE;
442+
}
443+
444+
445+
/* Current certificate failed, but it is possible there is an
446+
* alternative cert with the same subject key which will work.
447+
* Retry until all possible candidate certs are exhausted. */
448+
static int X509VerifyCertSetupRetry(WOLFSSL_X509_STORE_CTX* ctx,
449+
WOLF_STACK_OF(WOLFSSL_X509)* certs, WOLF_STACK_OF(WOLFSSL_X509)* failed,
450+
int* depth, int origDepth) {
451+
int ret = WC_NO_ERR_TRACE(WOLFSSL_FAILURE);
452+
453+
WOLFSSL_MSG("X509_verify_cert current cert failed, "
454+
"retrying with other certs.");
455+
ret = X509StoreRemoveCa(ctx->store, ctx->current_cert,
456+
WOLFSSL_TEMP_CA);
457+
X509StoreMoveCert(certs, failed, ctx->current_cert);
458+
ctx->current_cert = wolfSSL_sk_X509_pop(ctx->chain);
459+
if (*depth < origDepth)
460+
*depth += 1;
461+
462+
return ret;
463+
}
464+
412465
/* Verifies certificate chain using WOLFSSL_X509_STORE_CTX
413466
* returns 1 on success or <= 0 on failure.
414467
*/
@@ -499,25 +552,28 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
499552
goto exit;
500553
}
501554
}
502-
} else {
555+
} else
503556
#endif
504-
ret = X509StoreAddCa(ctx->store, issuer,
505-
WOLFSSL_TEMP_CA);
506-
if (ret != WOLFSSL_SUCCESS) {
507-
goto retry;
508-
}
509-
added = 1;
510-
ret = X509StoreVerifyCert(ctx);
511-
if (ret != WOLFSSL_SUCCESS) {
512-
if ((origDepth - depth) <= 1)
513-
added = 0;
514-
goto retry;
515-
}
516-
/* Add it to the current chain and look at the issuer cert next */
517-
wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
518-
#if defined(OPENSSL_ALL) || defined(WOLFSSL_QT)
557+
{
558+
ret = X509StoreAddCa(ctx->store, issuer,
559+
WOLFSSL_TEMP_CA);
560+
if (ret != WOLFSSL_SUCCESS) {
561+
X509VerifyCertSetupRetry(ctx, certs, failedCerts,
562+
&depth, origDepth);
563+
continue;
564+
}
565+
added = 1;
566+
ret = X509StoreVerifyCert(ctx);
567+
if (ret != WOLFSSL_SUCCESS) {
568+
if ((origDepth - depth) <= 1)
569+
added = 0;
570+
X509VerifyCertSetupRetry(ctx, certs, failedCerts,
571+
&depth, origDepth);
572+
continue;
573+
}
574+
/* Add it to the current chain and look at the issuer cert next */
575+
wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
519576
}
520-
#endif
521577
ctx->current_cert = issuer;
522578
}
523579
else if (ret == WC_NO_ERR_TRACE(WOLFSSL_FAILURE)) {
@@ -531,7 +587,9 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
531587
wolfSSL_sk_X509_push(ctx->chain, ctx->current_cert);
532588
ret = WOLFSSL_SUCCESS;
533589
} else {
534-
goto retry;
590+
X509VerifyCertSetupRetry(ctx, certs, failedCerts,
591+
&depth, origDepth);
592+
continue;
535593
}
536594
}
537595

@@ -564,33 +622,18 @@ int wolfSSL_X509_verify_cert(WOLFSSL_X509_STORE_CTX* ctx)
564622
}
565623

566624
depth--;
567-
continue;
568-
569-
retry:
570-
/* Current certificate failed, but it is possible there is an
571-
* alternative cert with the same subject key which will work.
572-
* Retry until all possible candidate certs are exhausted. */
573-
WOLFSSL_MSG("X509_verify_cert current cert failed,"
574-
"retrying with other certs.");
575-
ret = X509StoreRemoveCa(ctx->store, ctx->current_cert,
576-
WOLFSSL_TEMP_CA);
577-
X509StorePopCert(certs, failedCerts, ctx->current_cert);
578-
ctx->current_cert = wolfSSL_sk_X509_pop(ctx->chain);
579-
if (depth < origDepth)
580-
depth++;
581625
}
582626

583627
exit:
584628
/* Copy back failed certs if verification failed. */
585629
if (ret != WOLFSSL_SUCCESS) {
586-
while (wolfSSL_sk_X509_num(failedCerts) > 0)
630+
for (int cnt = 0, total = wolfSSL_sk_X509_num(failedCerts);
631+
cnt < total; cnt++)
587632
{
588633
wolfSSL_sk_X509_push(certs, wolfSSL_sk_X509_pop(failedCerts));
589634
}
590635
}
591-
if (failedCerts) {
592-
wolfSSL_sk_X509_pop_free(failedCerts, NULL);
593-
}
636+
wolfSSL_sk_X509_pop_free(failedCerts, NULL);
594637

595638
/* Remove additional intermediates from init from the store */
596639
if (ctx != NULL && numInterAdd > 0) {
@@ -1098,25 +1141,6 @@ static int X509StoreGetIssuerEx(WOLFSSL_X509 **issuer,
10981141
return WOLFSSL_FAILURE;
10991142
}
11001143

1101-
static int X509StorePopCert(WOLFSSL_STACK *certs_stack,
1102-
WOLFSSL_STACK *dest_stack,
1103-
WOLFSSL_X509 *cert) {
1104-
int i;
1105-
1106-
if (certs_stack == NULL || dest_stack == NULL || cert == NULL)
1107-
return WOLFSSL_FATAL_ERROR;
1108-
1109-
for (i = 0; i < wolfSSL_sk_X509_num(certs_stack); i++) {
1110-
if (wolfSSL_sk_X509_value(certs_stack, i) == cert) {
1111-
wolfSSL_sk_X509_push(dest_stack,
1112-
(WOLFSSL_X509*)wolfSSL_sk_pop_node(certs_stack, i));
1113-
return WOLFSSL_SUCCESS;
1114-
}
1115-
}
1116-
1117-
return WOLFSSL_FAILURE;
1118-
}
1119-
11201144
#endif
11211145

11221146
/******************************************************************************
@@ -1450,40 +1474,6 @@ static int X509StoreAddCa(WOLFSSL_X509_STORE* store,
14501474
return result;
14511475
}
14521476

1453-
static int X509StoreRemoveCa(WOLFSSL_X509_STORE* store,
1454-
WOLFSSL_X509* x509, int type) {
1455-
int result = WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR);
1456-
DecodedCert* dCert = NULL;
1457-
1458-
if (store != NULL && x509 != NULL && x509->derCert != NULL) {
1459-
dCert = (DecodedCert*)XMALLOC(sizeof(DecodedCert), NULL,
1460-
DYNAMIC_TYPE_DCERT);
1461-
1462-
if (dCert == NULL) {
1463-
return result;
1464-
}
1465-
XMEMSET(dCert, 0, sizeof(DecodedCert));
1466-
wc_InitDecodedCert(dCert, x509->derCert->buffer,
1467-
x509->derCert->length, NULL);
1468-
result = wc_ParseCert(dCert, CA_TYPE, NO_VERIFY, store->cm);
1469-
if (result) {
1470-
wc_FreeDecodedCert(dCert);
1471-
XFREE(dCert, NULL, DYNAMIC_TYPE_DCERT);
1472-
return WOLFSSL_FATAL_ERROR;
1473-
}
1474-
1475-
result = RemoveCA(store->cm, dCert->extSubjKeyId, type);
1476-
}
1477-
1478-
if (dCert) {
1479-
wc_FreeDecodedCert(dCert);
1480-
XFREE(dCert, NULL, DYNAMIC_TYPE_DCERT);
1481-
}
1482-
1483-
return result;
1484-
}
1485-
1486-
14871477
int wolfSSL_X509_STORE_add_cert(WOLFSSL_X509_STORE* store, WOLFSSL_X509* x509)
14881478
{
14891479
int result = WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR);

‎tests/api.c‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20363,10 +20363,10 @@ static int test_wolfSSL_X509_STORE_CTX_ex11(X509_STORE_test_data *testData)
2036320363
return EXPECT_RESULT();
2036420364
}
2036520365

20366-
#ifdef HAVE_ECC
2036720366
static int test_wolfSSL_X509_STORE_CTX_ex12(void)
2036820367
{
2036920368
EXPECT_DECLS;
20369+
#ifdef HAVE_ECC
2037020370
X509_STORE* store = NULL;
2037120371
X509_STORE_CTX* ctx = NULL;
2037220372
STACK_OF(X509)* chain = NULL;
@@ -20387,6 +20387,10 @@ static int test_wolfSSL_X509_STORE_CTX_ex12(void)
2038720387
ExpectNotNull(rootEccX509 = test_wolfSSL_X509_STORE_CTX_ex_helper(intCARootECCFile));
2038820388
ExpectIntEQ(X509_STORE_add_cert(store, rootEccX509), 1);
2038920389
ExpectNotNull(badAkiX509 = test_wolfSSL_X509_STORE_CTX_ex_helper(intCABadAKIECCFile));
20390+
ExpectNotNull(ctx = X509_STORE_CTX_new());
20391+
ExpectIntEQ(X509_STORE_CTX_init(ctx, store, badAkiX509, NULL), 1);
20392+
ExpectIntEQ(X509_verify_cert(ctx), 0);
20393+
2039020394
ExpectIntEQ(X509_STORE_add_cert(store, badAkiX509), 1);
2039120395
ExpectNotNull(ctx = X509_STORE_CTX_new());
2039220396
ExpectNotNull(ca1X509 = test_wolfSSL_X509_STORE_CTX_ex_helper(intCA1ECCFile));
@@ -20399,10 +20403,10 @@ static int test_wolfSSL_X509_STORE_CTX_ex12(void)
2039920403
X509_free(rootEccX509);
2040020404
X509_free(badAkiX509);
2040120405
X509_free(ca1X509);
20406+
#endif
2040220407
return EXPECT_RESULT();
2040320408
}
2040420409
#endif
20405-
#endif
2040620410

2040720411
static int test_wolfSSL_X509_STORE_CTX_ex(void)
2040820412
{
@@ -20441,9 +20445,7 @@ static int test_wolfSSL_X509_STORE_CTX_ex(void)
2044120445
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex9(&testData), 1);
2044220446
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex10(&testData), 1);
2044320447
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex11(&testData), 1);
20444-
#ifdef HAVE_ECC
2044520448
ExpectIntEQ(test_wolfSSL_X509_STORE_CTX_ex12(), 1);
20446-
#endif
2044720449

2044820450
if(testData.x509Ca) {
2044920451
X509_free(testData.x509Ca);

‎wolfcrypt/src/asn.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14351,7 +14351,7 @@ int CalcHashId_ex(const byte* data, word32 len, byte* hash, int hashAlg)
1435114351
* @return 0 on success.
1435214352
* @return MEMORY_E when dynamic memory allocation fails.
1435314353
*/
14354-
static int GetHashId(const byte* id, int length, byte* hash, int hashAlg)
14354+
int GetHashId(const byte* id, int length, byte* hash, int hashAlg)
1435514355
{
1435614356
int ret;
1435714357

‎wolfssl/wolfcrypt/asn.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2063,6 +2063,8 @@ WOLFSSL_LOCAL int HashIdAlg(word32 oidSum);
20632063
WOLFSSL_LOCAL int CalcHashId(const byte* data, word32 len, byte* hash);
20642064
WOLFSSL_LOCAL int CalcHashId_ex(const byte* data, word32 len, byte* hash,
20652065
int hashAlg);
2066+
WOLFSSL_LOCAL int GetHashId(const byte* id, int length, byte* hash,
2067+
int hashAlg);
20662068
WOLFSSL_LOCAL int GetName(DecodedCert* cert, int nameType, int maxIdx);
20672069

20682070
#ifdef ASN_BER_TO_DER

0 commit comments

Comments
 (0)