From 072fe8fd6d774231911c5d1b79f68a114533e80a Mon Sep 17 00:00:00 2001 From: kaleb-himes Date: Sat, 7 Dec 2019 03:39:57 -0700 Subject: [PATCH 1/5] More complete fix for removing NO_SKID condition as default with CRL enabled --- src/crl.c | 17 ++++++++--------- src/ssl.c | 4 ++++ wolfcrypt/src/asn.c | 23 ++++++++++++----------- wolfssl/internal.h | 5 ++--- wolfssl/wolfcrypt/asn.h | 2 ++ wolfssl/wolfcrypt/settings.h | 2 +- 6 files changed, 29 insertions(+), 24 deletions(-) diff --git a/src/crl.c b/src/crl.c index 1376a72d8..b94d3aadc 100644 --- a/src/crl.c +++ b/src/crl.c @@ -82,8 +82,7 @@ static int InitCRL_Entry(CRL_Entry* crle, DecodedCRL* dcrl, const byte* buff, WOLFSSL_ENTER("InitCRL_Entry"); XMEMCPY(crle->issuerHash, dcrl->issuerHash, CRL_DIGEST_SIZE); - /* XMEMCPY(crle->crlHash, dcrl->crlHash, CRL_DIGEST_SIZE); - * copy the hash here if needed for optimized comparisons */ + XMEMCPY(crle->crlHash, dcrl->crlHash, CRL_DIGEST_SIZE); XMEMCPY(crle->lastDate, dcrl->lastDate, MAX_DATE_SIZE); XMEMCPY(crle->nextDate, dcrl->nextDate, MAX_DATE_SIZE); crle->lastDateFormat = dcrl->lastDateFormat; @@ -109,7 +108,7 @@ static int InitCRL_Entry(CRL_Entry* crle, DecodedCRL* dcrl, const byte* buff, } XMEMCPY(crle->toBeSigned, buff + dcrl->certBegin, crle->tbsSz); XMEMCPY(crle->signature, dcrl->signature, crle->signatureSz); - #if !defined(NO_SKID) && defined(CRL_SKID_READY) + #ifndef NO_SKID crle->extAuthKeyIdSet = dcrl->extAuthKeyIdSet; if (crle->extAuthKeyIdSet) XMEMCPY(crle->extAuthKeyId, dcrl->extAuthKeyId, KEYID_SIZE); @@ -206,9 +205,9 @@ static int CheckCertCRLList(WOLFSSL_CRL* crl, DecodedCert* cert, int *pFoundEntr WOLFSSL_MSG("Found CRL Entry on list"); if (crle->verified == 0) { - Signer* ca; - #if !defined(NO_SKID) && defined(CRL_SKID_READY) - byte extAuthKeyId[KEYID_SIZE] + Signer* ca = NULL; + #ifndef NO_SKID + byte extAuthKeyId[KEYID_SIZE]; #endif byte issuerHash[CRL_DIGEST_SIZE]; byte* tbs = NULL; @@ -232,15 +231,15 @@ static int CheckCertCRLList(WOLFSSL_CRL* crl, DecodedCert* cert, int *pFoundEntr XMEMCPY(tbs, crle->toBeSigned, tbsSz); XMEMCPY(sig, crle->signature, sigSz); - #if !defined(NO_SKID) && defined(CRL_SKID_READY) - XMEMCMPY(extAuthKeyId, crle->extAuthKeyId, + #ifndef NO_SKID + XMEMCPY(extAuthKeyId, crle->extAuthKeyId, sizeof(extAuthKeyId)); #endif XMEMCPY(issuerHash, crle->issuerHash, sizeof(issuerHash)); wc_UnLockMutex(&crl->crlLock); - #if !defined(NO_SKID) && defined(CRL_SKID_READY) + #ifndef NO_SKID if (crle->extAuthKeyIdSet) ca = GetCA(crl->cm, extAuthKeyId); if (ca == NULL) diff --git a/src/ssl.c b/src/ssl.c index 039aac212..01d8e654b 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -4583,6 +4583,10 @@ Signer* GetCAByName(void* vp, byte* hash) if (XMEMCMP(hash, signers->subjectNameHash, SIGNER_DIGEST_SIZE) == 0) { ret = signers; + } else if (cm->crl != NULL && cm->crl->crlList != NULL && + XMEMCMP(hash, cm->crl->crlList->crlHash, + SIGNER_DIGEST_SIZE) == 0) { + ret = signers; } signers = signers->next; } diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index d9e5956d5..e5773edb2 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -15997,11 +15997,10 @@ int ParseCRL(DecodedCRL* dcrl, const byte* buff, word32 sz, void* cm) WOLFSSL_MSG("ParseCRL"); /* raw crl hash */ - /* hash here if needed for optimized comparisons - * wc_Sha sha; - * wc_InitSha(&sha); - * wc_ShaUpdate(&sha, buff, sz); - * wc_ShaFinal(&sha, dcrl->crlHash); */ + wc_Sha sha; + wc_InitSha(&sha); + wc_ShaUpdate(&sha, buff, sz); + wc_ShaFinal(&sha, dcrl->crlHash); if (GetSequence(buff, &idx, &len, sz) < 0) return ASN_PARSE_E; @@ -16026,15 +16025,17 @@ int ParseCRL(DecodedCRL* dcrl, const byte* buff, word32 sz, void* cm) return ASN_PARSE_E; /* openssl doesn't add skid by default for CRLs cause firefox chokes - we're not assuming it's available yet */ -#if !defined(NO_SKID) && defined(CRL_SKID_READY) - if (dcrl->extAuthKeyIdSet) - ca = GetCA(cm, dcrl->extAuthKeyId); + if experiencing issues uncomment NO_SKID define in CRL section of + wolfssl/wolfcrypt/settings.h */ +#ifndef NO_SKID + ca = GetCAByName(cm, dcrl->crlHash); /* most unique */ + if (ca == NULL && dcrl->extAuthKeyIdSet) + ca = GetCA(cm, dcrl->extAuthKeyId); /* more unique than issuerHash */ if (ca == NULL) - ca = GetCAByName(cm, dcrl->issuerHash); + ca = GetCAByName(cm, dcrl->issuerHash); /* last resort */ #else ca = GetCA(cm, dcrl->issuerHash); -#endif /* !NO_SKID && CRL_SKID_READY */ +#endif /* !NO_SKID */ WOLFSSL_MSG("About to verify CRL signature"); if (ca == NULL) { diff --git a/wolfssl/internal.h b/wolfssl/internal.h index ba1c7a00c..5707f29df 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1856,8 +1856,7 @@ typedef struct CRL_Entry CRL_Entry; struct CRL_Entry { CRL_Entry* next; /* next entry */ byte issuerHash[CRL_DIGEST_SIZE]; /* issuer hash */ - /* byte crlHash[CRL_DIGEST_SIZE]; raw crl data hash */ - /* restore the hash here if needed for optimized comparisons */ + byte crlHash[CRL_DIGEST_SIZE]; /* raw crl data hash */ byte lastDate[MAX_DATE_SIZE]; /* last date updated */ byte nextDate[MAX_DATE_SIZE]; /* next update date */ byte lastDateFormat; /* last date format */ @@ -1870,7 +1869,7 @@ struct CRL_Entry { byte* signature; word32 signatureSz; word32 signatureOID; -#if !defined(NO_SKID) && defined(CRL_SKID_READY) +#ifndef NO_SKID byte extAuthKeyIdSet; byte extAuthKeyId[KEYID_SIZE]; #endif diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 860a7a1d3..311463890 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -1345,11 +1345,13 @@ struct DecodedCRL { byte crlHash[SIGNER_DIGEST_SIZE]; /* raw crl data hash */ byte lastDate[MAX_DATE_SIZE]; /* last date updated */ byte nextDate[MAX_DATE_SIZE]; /* next update date */ + byte extAuthKeyId[KEYID_SIZE]; /* Authority Key ID */ byte lastDateFormat; /* format of last date */ byte nextDateFormat; /* format of next date */ RevokedCert* certs; /* revoked cert list */ int totalCerts; /* number on list */ void* heap; + byte extAuthKeyIdSet : 1; /* Set when the AKID was read from CRL */ }; WOLFSSL_LOCAL void InitDecodedCRL(DecodedCRL*, void* heap); diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index 1f1a672aa..fb2cc2e60 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -1621,7 +1621,7 @@ extern void uITRON4_free(void *p) ; #ifdef HAVE_CRL /* not widely supported yet */ #undef NO_SKID - #define NO_SKID + /* #define NO_SKID */ #endif From bbdf0d101f6f6e104044f6ce506ae79ed09cfa5d Mon Sep 17 00:00:00 2001 From: kaleb-himes Date: Sat, 7 Dec 2019 04:23:02 -0700 Subject: [PATCH 2/5] Improve Decoded CRL initialization --- wolfcrypt/src/asn.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index e5773edb2..363e8dcf0 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -15796,12 +15796,21 @@ void InitDecodedCRL(DecodedCRL* dcrl, void* heap) dcrl->sigIndex = 0; dcrl->sigLength = 0; dcrl->signatureOID = 0; + dcrl->signature = NULL; + XMEMSET(dcrl->issuerHash, 0, SIGNER_DIGEST_SIZE); + XMEMSET(dcrl->crlHash, 0, SIGNER_DIGEST_SIZE); + XMEMSET(dcrl->lastDate, 0, MAX_DATE_SIZE); + XMEMSET(dcrl->nextDate, 0, MAX_DATE_SIZE); + XMEMSET(dcrl->extAuthKeyId, 0, KEYID_SIZE); + dcrl->lastDateFormat = 0; + dcrl->nextDateFormat = 0; dcrl->certs = NULL; dcrl->totalCerts = 0; dcrl->heap = heap; #ifdef WOLFSSL_HEAP_TEST dcrl->heap = (void*)WOLFSSL_HEAP_TEST; #endif + dcrl->extAuthKeyIdSet = 0; } From e8c7d6f81832cdb3f0b14f60bf0681f72f08c494 Mon Sep 17 00:00:00 2001 From: kaleb-himes Date: Sat, 7 Dec 2019 05:06:41 -0700 Subject: [PATCH 3/5] Account for ASN disabled --- wolfssl/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 5707f29df..20b1386a6 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1869,7 +1869,7 @@ struct CRL_Entry { byte* signature; word32 signatureSz; word32 signatureOID; -#ifndef NO_SKID +#if !defined(NO_SKID) && !defined(NO_ASN) byte extAuthKeyIdSet; byte extAuthKeyId[KEYID_SIZE]; #endif From 2b66a9f1ec8db7dddb9ac77c427cd3983d5fcd2e Mon Sep 17 00:00:00 2001 From: kaleb-himes Date: Mon, 9 Dec 2019 14:44:59 -0700 Subject: [PATCH 4/5] Address reviewed items --- src/crl.c | 3 ++- src/ssl.c | 4 ---- wolfcrypt/src/asn.c | 15 ++++++++------- wolfssl/internal.h | 3 ++- wolfssl/wolfcrypt/settings.h | 4 ++-- 5 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/crl.c b/src/crl.c index b94d3aadc..803bd0243 100644 --- a/src/crl.c +++ b/src/crl.c @@ -82,7 +82,8 @@ static int InitCRL_Entry(CRL_Entry* crle, DecodedCRL* dcrl, const byte* buff, WOLFSSL_ENTER("InitCRL_Entry"); XMEMCPY(crle->issuerHash, dcrl->issuerHash, CRL_DIGEST_SIZE); - XMEMCPY(crle->crlHash, dcrl->crlHash, CRL_DIGEST_SIZE); + /* XMEMCPY(crle->crlHash, dcrl->crlHash, CRL_DIGEST_SIZE); + * copy the hash here if needed for optimized comparisons */ XMEMCPY(crle->lastDate, dcrl->lastDate, MAX_DATE_SIZE); XMEMCPY(crle->nextDate, dcrl->nextDate, MAX_DATE_SIZE); crle->lastDateFormat = dcrl->lastDateFormat; diff --git a/src/ssl.c b/src/ssl.c index 01d8e654b..039aac212 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -4583,10 +4583,6 @@ Signer* GetCAByName(void* vp, byte* hash) if (XMEMCMP(hash, signers->subjectNameHash, SIGNER_DIGEST_SIZE) == 0) { ret = signers; - } else if (cm->crl != NULL && cm->crl->crlList != NULL && - XMEMCMP(hash, cm->crl->crlList->crlHash, - SIGNER_DIGEST_SIZE) == 0) { - ret = signers; } signers = signers->next; } diff --git a/wolfcrypt/src/asn.c b/wolfcrypt/src/asn.c index 363e8dcf0..ed698f32d 100644 --- a/wolfcrypt/src/asn.c +++ b/wolfcrypt/src/asn.c @@ -15798,7 +15798,8 @@ void InitDecodedCRL(DecodedCRL* dcrl, void* heap) dcrl->signatureOID = 0; dcrl->signature = NULL; XMEMSET(dcrl->issuerHash, 0, SIGNER_DIGEST_SIZE); - XMEMSET(dcrl->crlHash, 0, SIGNER_DIGEST_SIZE); + /* XMEMSET(dcrl->crlHash, 0, SIGNER_DIGEST_SIZE); + * initialize the hash here if needed for optimized comparisons */ XMEMSET(dcrl->lastDate, 0, MAX_DATE_SIZE); XMEMSET(dcrl->nextDate, 0, MAX_DATE_SIZE); XMEMSET(dcrl->extAuthKeyId, 0, KEYID_SIZE); @@ -16006,10 +16007,11 @@ int ParseCRL(DecodedCRL* dcrl, const byte* buff, word32 sz, void* cm) WOLFSSL_MSG("ParseCRL"); /* raw crl hash */ - wc_Sha sha; - wc_InitSha(&sha); - wc_ShaUpdate(&sha, buff, sz); - wc_ShaFinal(&sha, dcrl->crlHash); + /* hash here if needed for optimized comparisons + * wc_Sha sha; + * wc_InitSha(&sha); + * wc_ShaUpdate(&sha, buff, sz); + * wc_ShaFinal(&sha, dcrl->crlHash); */ if (GetSequence(buff, &idx, &len, sz) < 0) return ASN_PARSE_E; @@ -16037,8 +16039,7 @@ int ParseCRL(DecodedCRL* dcrl, const byte* buff, word32 sz, void* cm) if experiencing issues uncomment NO_SKID define in CRL section of wolfssl/wolfcrypt/settings.h */ #ifndef NO_SKID - ca = GetCAByName(cm, dcrl->crlHash); /* most unique */ - if (ca == NULL && dcrl->extAuthKeyIdSet) + if (dcrl->extAuthKeyIdSet) ca = GetCA(cm, dcrl->extAuthKeyId); /* more unique than issuerHash */ if (ca == NULL) ca = GetCAByName(cm, dcrl->issuerHash); /* last resort */ diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 20b1386a6..694607d50 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -1856,7 +1856,8 @@ typedef struct CRL_Entry CRL_Entry; struct CRL_Entry { CRL_Entry* next; /* next entry */ byte issuerHash[CRL_DIGEST_SIZE]; /* issuer hash */ - byte crlHash[CRL_DIGEST_SIZE]; /* raw crl data hash */ + /* byte crlHash[CRL_DIGEST_SIZE]; raw crl data hash */ + /* restore the hash here if needed for optimized comparisons */ byte lastDate[MAX_DATE_SIZE]; /* last date updated */ byte nextDate[MAX_DATE_SIZE]; /* next update date */ byte lastDateFormat; /* last date format */ diff --git a/wolfssl/wolfcrypt/settings.h b/wolfssl/wolfcrypt/settings.h index fb2cc2e60..df8ced810 100644 --- a/wolfssl/wolfcrypt/settings.h +++ b/wolfssl/wolfcrypt/settings.h @@ -1619,8 +1619,8 @@ extern void uITRON4_free(void *p) ; #endif #ifdef HAVE_CRL - /* not widely supported yet */ - #undef NO_SKID + /* may not be widely supported */ + /* #undef NO_SKID */ /* #define NO_SKID */ #endif From 74e54393abfaa758c37fd5950de4d0a3efa38a15 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 10 Dec 2019 13:17:30 -0800 Subject: [PATCH 5/5] Remove a bitfield indicator from a structure member that didn't require it. --- wolfssl/wolfcrypt/asn.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wolfssl/wolfcrypt/asn.h b/wolfssl/wolfcrypt/asn.h index 311463890..99222638d 100644 --- a/wolfssl/wolfcrypt/asn.h +++ b/wolfssl/wolfcrypt/asn.h @@ -1351,7 +1351,7 @@ struct DecodedCRL { RevokedCert* certs; /* revoked cert list */ int totalCerts; /* number on list */ void* heap; - byte extAuthKeyIdSet : 1; /* Set when the AKID was read from CRL */ + byte extAuthKeyIdSet; /* Set when the AKID was read from CRL */ }; WOLFSSL_LOCAL void InitDecodedCRL(DecodedCRL*, void* heap);