From 3646051434dac02ecc9f59577868e417efd64c4e Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 4 Dec 2019 06:56:36 -0800 Subject: [PATCH 1/5] Fix for alternate chain logic where presented peer's CA could be marked as trusted. When building with `WOLFSSL_ALT_CERT_CHAINS` a peer's presented CA could be incorrectly added to the certificate manager, marking it as trusted. Began in PR #1934 ZD 9626 --- src/internal.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index e230283c7..c3d212fe9 100644 --- a/src/internal.c +++ b/src/internal.c @@ -10250,12 +10250,15 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, } #endif /* HAVE_OCSP || HAVE_CRL */ + /* Do verify callback */ + ret = DoVerifyCallback(ssl, ret, args); + #ifdef WOLFSSL_ALT_CERT_CHAINS /* For alternate cert chain, its okay for a CA cert to fail with ASN_NO_SIGNER_E here. The "alternate" certificate chain mode only requires that the peer certificate validate to a trusted CA */ - if (ret != 0) { + if (ret != 0 && args->dCert->isCA) { if (ret == ASN_NO_SIGNER_E) { if (!ssl->options.usingAltCertChain) { WOLFSSL_MSG("Trying alternate cert chain"); @@ -10265,11 +10268,9 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret = 0; /* clear error and continue */ } } + else /* do not add to certificate manager */ #endif /* WOLFSSL_ALT_CERT_CHAINS */ - /* Do verify callback */ - ret = DoVerifyCallback(ssl, ret, args); - /* If valid CA then add to Certificate Manager */ if (ret == 0 && args->dCert->isCA && !ssl->options.verifyNone) { buffer* cert = &args->certs[args->certIdx]; From acd4bc3305841e7e9f32d1df55eff0857fadb1e9 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 4 Dec 2019 11:02:22 -0800 Subject: [PATCH 2/5] Added logging for SendAlert call. --- src/internal.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index c3d212fe9..7f82efd56 100644 --- a/src/internal.c +++ b/src/internal.c @@ -16746,6 +16746,8 @@ int SendAlert(WOLFSSL* ssl, int severity, int type) int outputSz; int dtlsExtra = 0; + WOLFSSL_ENTER("SendAlert"); + #ifdef HAVE_WRITE_DUP if (ssl->dupWrite && ssl->dupSide == READ_DUP_SIDE) { int notifyErr = 0; @@ -16842,7 +16844,11 @@ int SendAlert(WOLFSSL* ssl, int severity, int type) ssl->buffers.outputBuffer.length += sendSz; ssl->options.sendAlertState = 1; - return SendBuffered(ssl); + ret = SendBuffered(ssl); + + WOLFSSL_LEAVE("SendAlert", ret); + + return ret; } const char* wolfSSL_ERR_reason_error_string(unsigned long e) From b01c558adbec51f963e9e8e2c0b5541ff7172510 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 4 Dec 2019 12:41:23 -0800 Subject: [PATCH 3/5] Fix to not send alert until after the verify cert callback and alternate chain logic has been evaluated. --- src/internal.c | 71 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/src/internal.c b/src/internal.c index 7f82efd56..f004ce9ea 100644 --- a/src/internal.c +++ b/src/internal.c @@ -9368,6 +9368,29 @@ typedef struct ProcPeerCertArgs { #endif } ProcPeerCertArgs; +static void DoCertFatalAlert(WOLFSSL* ssl, int ret) +{ + int alertWhy; + if (ssl == NULL || ret == 0) { + return; + } + + /* Determine alert reason */ + alertWhy = bad_certificate; + if (ret == ASN_AFTER_DATE_E || ret == ASN_BEFORE_DATE_E) { + alertWhy = certificate_expired; + } +#if (defined(OPENSSL_ALL) || defined(WOLFSSL_APACHE_HTTPD)) + else if (ret == CRL_CERT_REVOKED) { + alertWhy = certificate_revoked; + } +#endif + + /* send fatal alert and mark connection closed */ + SendAlert(ssl, alert_fatal, alertWhy); /* try to send */ + ssl->options.isClosed = 1; +} + /* WOLFSSL_ALWAYS_VERIFY_CB: Use verify callback for success or failure cases */ /* WOLFSSL_VERIFY_CB_ALL_CERTS: Issue callback for all intermediate certificates */ @@ -9378,21 +9401,10 @@ typedef struct ProcPeerCertArgs { static int DoVerifyCallback(WOLFSSL* ssl, int ret, ProcPeerCertArgs* args) { - int verify_ok = 0, alertWhy = 0, use_cb = 0; + int verify_ok = 0, use_cb = 0; - /* Determine return code and alert reason */ - if (ret != 0) { - alertWhy = bad_certificate; - if (ret == ASN_AFTER_DATE_E || - ret == ASN_BEFORE_DATE_E) { - alertWhy = certificate_expired; - } -#if (defined(OPENSSL_ALL) || defined(WOLFSSL_APACHE_HTTPD)) - else if (ret == CRL_CERT_REVOKED) - alertWhy = certificate_revoked; -#endif - } - else { + /* Determine if verify was okay */ + if (ret == 0) { verify_ok = 1; } @@ -9618,17 +9630,6 @@ static int DoVerifyCallback(WOLFSSL* ssl, int ret, ProcPeerCertArgs* args) #endif } - if (ret != 0) { - if (!ssl->options.verifyNone) { - /* handle failure */ - SendAlert(ssl, alert_fatal, alertWhy); /* try to send */ - ssl->options.isClosed = 1; - } - - /* Report SSL error */ - ssl->error = ret; - } - return ret; } @@ -10302,9 +10303,16 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, } /* Handle error codes */ - if (ret != 0 && args->lastErr == 0) { - args->lastErr = ret; /* save error from last time */ - ret = 0; /* reset error */ + if (ret != 0) { + if (!ssl->options.verifyNone) { + DoCertFatalAlert(ssl, ret); + } + ssl->error = ret; /* Report SSL error */ + + if (args->lastErr == 0) { + args->lastErr = ret; /* save error from last time */ + ret = 0; /* reset error */ + } } FreeDecodedCert(args->dCert); @@ -10876,6 +10884,13 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret = ssl->error = 0; } + if (ret != 0) { + if (!ssl->options.verifyNone) { + DoCertFatalAlert(ssl, ret); + } + ssl->error = ret; /* Report SSL error */ + } + if (ret == 0 && ssl->options.side == WOLFSSL_CLIENT_END) { ssl->options.serverState = SERVER_CERT_COMPLETE; } From 9b437384de0201a3c822fbdb11564b0c16231f70 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 4 Dec 2019 14:14:37 -0800 Subject: [PATCH 4/5] Allow `AddCA` for root CA's over the wire that do not have the extended key usage cert_sign set. --- src/ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ssl.c b/src/ssl.c index 090561b1f..6123a2ac9 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -4807,7 +4807,7 @@ int AddCA(WOLFSSL_CERT_MANAGER* cm, DerBuffer** pDer, int type, int verify) } #ifndef ALLOW_INVALID_CERTSIGN else if (ret == 0 && cert->isCA == 1 && type != WOLFSSL_USER_CA && - (cert->extKeyUsage & KEYUSE_KEY_CERT_SIGN) == 0) { + !cert->selfSigned && (cert->extKeyUsage & KEYUSE_KEY_CERT_SIGN) == 0) { /* Intermediate CA certs are required to have the keyCertSign * extension set. User loaded root certs are not. */ WOLFSSL_MSG("\tDoesn't have key usage certificate signing"); From b126802c36d286a4192d6366771c6d6dad5fbf6d Mon Sep 17 00:00:00 2001 From: David Garske Date: Tue, 17 Dec 2019 14:50:36 -0800 Subject: [PATCH 5/5] Clarify logic for skipping call to AddCA. --- src/internal.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index f004ce9ea..8aa541f00 100644 --- a/src/internal.c +++ b/src/internal.c @@ -10161,6 +10161,8 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, && !args->haveTrustPeer #endif /* WOLFSSL_TRUST_PEER_CERT */ ) { + int skipAddCA = 0; + /* select last certificate */ args->certIdx = args->count - 1; @@ -10268,12 +10270,15 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret = 0; /* clear error and continue */ } + + /* do not add to certificate manager */ + skipAddCA = 1; } - else /* do not add to certificate manager */ #endif /* WOLFSSL_ALT_CERT_CHAINS */ /* If valid CA then add to Certificate Manager */ - if (ret == 0 && args->dCert->isCA && !ssl->options.verifyNone) { + if (ret == 0 && args->dCert->isCA && + !ssl->options.verifyNone && !skipAddCA) { buffer* cert = &args->certs[args->certIdx]; /* Is valid CA */