From 53c0003cad29cb2f4f805f5c0d331f8bdba569fa Mon Sep 17 00:00:00 2001 From: David Garske Date: Tue, 14 Aug 2018 16:52:47 -0600 Subject: [PATCH 1/6] Fix to resolve issue with verify callback not causing an error (if one not already present) when returning 0. Test case to follow shortly. --- src/internal.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/internal.c b/src/internal.c index e62a43221..3fca064bd 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8602,6 +8602,12 @@ static int DoVerifyCallback(WOLFSSL* ssl, int ret, ProcPeerCertArgs* args) WOLFSSL_MSG("Verify callback overriding error!"); ret = 0; } + else { + /* induce error if one not present */ + if (ret == 0) { + ret = VERIFY_CERT_ERROR; + } + } #ifdef OPENSSL_EXTRA if (args->certIdx > 0) FreeX509(x509); From 944342b386799f0f6b1da190831ad21d2b2469b5 Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 29 Aug 2018 10:06:46 -0700 Subject: [PATCH 2/6] Fixes for verify callback failure override handling. Fixes the return codes in the failure cases. --- src/internal.c | 57 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/src/internal.c b/src/internal.c index 3fca064bd..4934ff843 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8448,9 +8448,7 @@ typedef struct ProcPeerCertArgs { word32 begin; int totalCerts; /* number of certs in certs buffer */ int count; - int dCertInit; int certIdx; - int fatal; int lastErr; #ifdef WOLFSSL_ALT_CERT_CHAINS int lastCaErr; @@ -8458,11 +8456,14 @@ typedef struct ProcPeerCertArgs { #ifdef WOLFSSL_TLS13 byte ctxSz; #endif -#ifdef WOLFSSL_TRUST_PEER_CERT - byte haveTrustPeer; /* was cert verified by loaded trusted peer cert */ -#endif #if defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) char untrustedDepth; +#endif + word16 fatal:1; + word16 verifyErr:1; + word16 dCertInit:1; +#ifdef WOLFSSL_TRUST_PEER_CERT + word16 haveTrustPeer:1; /* was cert verified by loaded trusted peer cert */ #endif } ProcPeerCertArgs; @@ -8607,6 +8608,9 @@ static int DoVerifyCallback(WOLFSSL* ssl, int ret, ProcPeerCertArgs* args) if (ret == 0) { ret = VERIFY_CERT_ERROR; } + + /* mark as verify error */ + args->verifyErr = 1; } #ifdef OPENSSL_EXTRA if (args->certIdx > 0) @@ -9174,7 +9178,8 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ssl->options.usingAltCertChain = 1; /* clear last CA fail since CA cert was validated */ - args->lastCaErr = 0; + if (!args->verifyErr) + args->lastCaErr = 0; #ifdef SESSION_CERTS AddSessionCertToChain(&ssl->session.altChain, @@ -9185,16 +9190,6 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, } else if (ret != 0) { WOLFSSL_MSG("Failed to verify CA from chain"); - #ifdef WOLFSSL_ALT_CERT_CHAINS - if (args->lastCaErr == 0) { - /* store CA error and proceed to next cert */ - args->lastCaErr = ret; - ret = 0; - } - else { - args->lastErr = args->lastCaErr; - } - #endif #ifdef OPENSSL_EXTRA ssl->peerVerifyRet = X509_V_ERR_INVALID_CA; #endif @@ -9265,6 +9260,16 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, ret = DoVerifyCallback(ssl, ret, args); /* Handle error codes */ + #ifdef WOLFSSL_ALT_CERT_CHAINS + if (args->lastCaErr == 0) { + /* capture CA error and proceed to next cert */ + args->lastCaErr = ret; + ret = 0; + } + else { + args->lastErr = args->lastCaErr; + } + #endif if (ret != 0 && args->lastErr == 0) { args->lastErr = ret; /* save error from last time */ ret = 0; /* reset error */ @@ -9337,7 +9342,22 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, cert->buffer, cert->length); } #endif /* SESSION_CERTS && WOLFSSL_ALT_CERT_CHAINS */ - args->fatal = 0; + + /* check if fatal error */ + if (args->verifyErr) { + args->fatal = 1; + if (ret == 0) { + ret = args->lastErr; + } + #ifdef WOLFSSL_ALT_CERT_CHAINS + if (ret == 0) { + ret = args->lastCaErr; + } + #endif + } + else { + args->fatal = 0; + } } else if (ret == ASN_PARSE_E || ret == BUFFER_E) { WOLFSSL_MSG("Got Peer cert ASN PARSE or BUFFER ERROR"); @@ -9355,7 +9375,8 @@ int ProcessPeerCerts(WOLFSSL* ssl, byte* input, word32* inOutIdx, if (ssl->verifyCallback) { WOLFSSL_MSG( "\tCallback override available, will continue"); - args->fatal = 0; + /* check if fatal error */ + args->fatal = (args->verifyErr) ? 1 : 0; } else { WOLFSSL_MSG("\tNo callback override available, fatal"); From 3d0d10345a615f6a6d941de79b7f5e00074a3eba Mon Sep 17 00:00:00 2001 From: David Garske Date: Wed, 29 Aug 2018 10:55:12 -0700 Subject: [PATCH 3/6] Added test cases for ensuring forced error fails on client and server. Added test cases to ensure bad certificate can be overriden. --- examples/client/client.c | 12 ++++++++---- examples/server/server.c | 8 ++++++-- tests/test-fails.conf | 36 ++++++++++++++++++++++++++++++++++++ tests/test.conf | 11 +++++++++++ wolfssl/test.h | 4 ++++ 5 files changed, 65 insertions(+), 6 deletions(-) diff --git a/examples/client/client.c b/examples/client/client.c index 4635e49d6..40dae0d3b 100644 --- a/examples/client/client.c +++ b/examples/client/client.c @@ -887,7 +887,7 @@ static void Usage(void) #ifdef HAVE_WNR printf("-q Whitewood config file, default %s\n", wnrConfig); #endif - printf("-H Internal tests [defCipherList, exitWithRet]\n"); + printf("-H Internal tests [defCipherList, exitWithRet, verifyFail]\n"); #ifdef WOLFSSL_TLS13 printf("-J Use HelloRetryRequest to choose group for KE\n"); printf("-K Key Exchange for PSK not using (EC)DHE\n"); @@ -1211,10 +1211,14 @@ THREAD_RETURN WOLFSSL_THREAD client_test(void* args) printf("Using default cipher list for testing\n"); useDefCipherList = 1; } - else if (XSTRNCMP(myoptarg, "exitWithRet", 7) == 0) { + else if (XSTRNCMP(myoptarg, "exitWithRet", 11) == 0) { printf("Skip exit() for testing\n"); exitWithRet = 1; } + else if (XSTRNCMP(myoptarg, "verifyFail", 10) == 0) { + printf("Verify should fail\n"); + myVerifyFail = 1; + } else { Usage(); XEXIT_T(MY_EX_USAGE); @@ -1821,9 +1825,9 @@ THREAD_RETURN WOLFSSL_THREAD client_test(void* args) #endif } - if (!usePsk && !useAnon && !useVerifyCb) { + if (!usePsk && !useAnon && (!useVerifyCb || myVerifyFail)) { #if !defined(NO_FILESYSTEM) - if (wolfSSL_CTX_load_verify_locations(ctx, verifyCert,0) + if (wolfSSL_CTX_load_verify_locations(ctx, verifyCert, 0) != WOLFSSL_SUCCESS) { wolfSSL_CTX_free(ctx); ctx = NULL; err_sys("can't load ca file, Please run from wolfSSL home dir"); diff --git a/examples/server/server.c b/examples/server/server.c index 3574dc3e5..46a54986b 100644 --- a/examples/server/server.c +++ b/examples/server/server.c @@ -416,7 +416,7 @@ static void Usage(void) #endif printf("-g Return basic HTML web page\n"); printf("-C The number of connections to accept, default: 1\n"); - printf("-H Internal tests [defCipherList, exitWithRet]\n"); + printf("-H Internal tests [defCipherList, exitWithRet, verifyFail]\n"); #ifdef WOLFSSL_TLS13 printf("-U Update keys and IVs before sending\n"); printf("-K Key Exchange for PSK not using (EC)DHE\n"); @@ -714,10 +714,14 @@ THREAD_RETURN WOLFSSL_THREAD server_test(void* args) printf("Using default cipher list for testing\n"); useDefCipherList = 1; } - else if (XSTRNCMP(myoptarg, "exitWithRet", 7) == 0) { + else if (XSTRNCMP(myoptarg, "exitWithRet", 11) == 0) { printf("Skip exit() for testing\n"); exitWithRet = 1; } + else if (XSTRNCMP(myoptarg, "verifyFail", 10) == 0) { + printf("Verify should fail\n"); + myVerifyFail = 1; + } else { Usage(); XEXIT_T(MY_EX_USAGE); diff --git a/tests/test-fails.conf b/tests/test-fails.conf index e41fcd35e..953eaa738 100644 --- a/tests/test-fails.conf +++ b/tests/test-fails.conf @@ -107,3 +107,39 @@ -A ./certs/test/server-garbage.pem -m +# Verify Callback Failure Tests +# server +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 + +# client verify should fail +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 +-H verifyFail + +# server verify should fail +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 +-H verifyFail + +# client +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 + +# server +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 + +# client verify should fail +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-H verifyFail + +# server verify should fail +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-H verifyFail + +# client +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 diff --git a/tests/test.conf b/tests/test.conf index b1b8fe9fb..6a3505cf7 100644 --- a/tests/test.conf +++ b/tests/test.conf @@ -2202,12 +2202,23 @@ # server TLSv1.2 verify callback override -v 3 -l ECDHE-RSA-AES128-SHA256 +-c ./certs/test/server-cert-rsa-badsig.pem # client TLSv1.2 verify callback override -v 3 -l ECDHE-RSA-AES128-SHA256 -j +# server TLSv1.2 verify callback override +-v 3 +-l ECDHE-ECDSA-AES128-SHA256 +-c ./certs/test/server-cert-ecc-badsig.pem + +# client TLSv1.2 verify callback override +-v 3 +-l ECDHE-ECDSA-AES128-SHA256 +-j + # server TLSv1.2 ECDHE-EDCSA-CHACHA20-POLY1305 -v 3 -l ECDHE-ECDSA-CHACHA20-POLY1305 diff --git a/wolfssl/test.h b/wolfssl/test.h index dc24fb2a6..004c10800 100644 --- a/wolfssl/test.h +++ b/wolfssl/test.h @@ -1452,6 +1452,7 @@ static WC_INLINE void OCSPRespFreeCb(void* ioCtx, unsigned char* response) #endif /* !NO_FILESYSTEM || (NO_FILESYSTEM && FORCE_BUFFER_TEST) */ #endif /* !NO_CERTS */ +static int myVerifyFail = 0; static WC_INLINE int myVerify(int preverify, WOLFSSL_X509_STORE_CTX* store) { char buffer[WOLFSSL_MAX_ERROR_SZ]; @@ -1505,6 +1506,9 @@ static WC_INLINE int myVerify(int preverify, WOLFSSL_X509_STORE_CTX* store) printf("\tAllowing to continue anyway (shouldn't do this)\n"); /* A non-zero return code indicates failure override */ + if (myVerifyFail) + return 0; /* test failure case */ + return 1; } From b369e524d47af6b94c4e9d0439abe3084c200b99 Mon Sep 17 00:00:00 2001 From: David Garske Date: Thu, 30 Aug 2018 11:48:08 -0700 Subject: [PATCH 4/6] Fix for the ECDSA verify callback override test case. Switched to AES128-GCM cipher suite (better cipher suite overall). --- tests/test.conf | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test.conf b/tests/test.conf index 6a3505cf7..b8017c848 100644 --- a/tests/test.conf +++ b/tests/test.conf @@ -2201,22 +2201,24 @@ # server TLSv1.2 verify callback override -v 3 --l ECDHE-RSA-AES128-SHA256 +-l ECDHE-RSA-AES128-GCM-SHA256 -c ./certs/test/server-cert-rsa-badsig.pem # client TLSv1.2 verify callback override -v 3 --l ECDHE-RSA-AES128-SHA256 +-l ECDHE-RSA-AES128-GCM-SHA256 -j # server TLSv1.2 verify callback override -v 3 --l ECDHE-ECDSA-AES128-SHA256 +-l ECDHE-ECDSA-AES128-GCM-SHA256 -c ./certs/test/server-cert-ecc-badsig.pem +-k ./certs/ecc-key.pem # client TLSv1.2 verify callback override -v 3 --l ECDHE-ECDSA-AES128-SHA256 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-A ./certs/ca-ecc-cert.pem -j # server TLSv1.2 ECDHE-EDCSA-CHACHA20-POLY1305 From ffc0f0fb269001f5ddbc1f942a48e1c47af4ff8d Mon Sep 17 00:00:00 2001 From: David Garske Date: Thu, 30 Aug 2018 13:57:21 -0700 Subject: [PATCH 5/6] Fix for building with `SESSION_CERTS` using pointer after free. Documented `store->discardSessionCerts`. --- src/internal.c | 15 +++++++-------- wolfssl/test.h | 2 ++ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/internal.c b/src/internal.c index 4934ff843..5e5b4352e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8620,14 +8620,6 @@ static int DoVerifyCallback(WOLFSSL* ssl, int ret, ProcPeerCertArgs* args) wolfSSL_sk_X509_free(store->chain); store->chain = NULL; #endif - #ifdef WOLFSSL_SMALL_STACK - XFREE(domain, ssl->heap, DYNAMIC_TYPE_STRING); - #ifdef OPENSSL_EXTRA - XFREE(x509, ssl->heap, DYNAMIC_TYPE_X509); - #endif - XFREE(store, ssl->heap, DYNAMIC_TYPE_X509_STORE); - #endif - #ifdef SESSION_CERTS if (store->discardSessionCerts) { WOLFSSL_MSG("Verify callback requested discard sess certs"); @@ -8637,6 +8629,13 @@ static int DoVerifyCallback(WOLFSSL* ssl, int ret, ProcPeerCertArgs* args) #endif } #endif /* SESSION_CERTS */ + #ifdef WOLFSSL_SMALL_STACK + XFREE(domain, ssl->heap, DYNAMIC_TYPE_STRING); + #ifdef OPENSSL_EXTRA + XFREE(x509, ssl->heap, DYNAMIC_TYPE_X509); + #endif + XFREE(store, ssl->heap, DYNAMIC_TYPE_X509_STORE); + #endif } if (ret != 0) { diff --git a/wolfssl/test.h b/wolfssl/test.h index 004c10800..0a8849c6b 100644 --- a/wolfssl/test.h +++ b/wolfssl/test.h @@ -1471,6 +1471,8 @@ static WC_INLINE int myVerify(int preverify, WOLFSSL_X509_STORE_CTX* store) * store->store: WOLFSSL_X509_STORE with CA cert chain * store->store->cm: WOLFSSL_CERT_MANAGER * store->ex_data: The WOLFSSL object pointer + * store->discardSessionCerts: When set to non-zero value session certs + will be discarded (only with SESSION_CERTS) */ printf("In verification callback, error = %d, %s\n", store->error, From d2b9b230a0eb78a96912dbcc8d1edc93003714be Mon Sep 17 00:00:00 2001 From: David Garske Date: Fri, 31 Aug 2018 16:26:51 -0700 Subject: [PATCH 6/6] Added additional verify callback override test cases. --- tests/test-fails.conf | 24 ++++++++++++++++++++++++ tests/test.conf | 24 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/tests/test-fails.conf b/tests/test-fails.conf index 953eaa738..d976b307b 100644 --- a/tests/test-fails.conf +++ b/tests/test-fails.conf @@ -108,6 +108,7 @@ -m # Verify Callback Failure Tests +# no error going into callback, return error # server -v 3 -l ECDHE-RSA-AES128-GCM-SHA256 @@ -143,3 +144,26 @@ # client -v 3 -l ECDHE-ECDSA-AES128-GCM-SHA256 + +# error going into callback, return error +# server +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 +-c ./certs/test/server-cert-rsa-badsig.pem +-k ./certs/server-key.pem + +# client verify should fail +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 +-H verifyFail + +# server +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-c ./certs/test/server-cert-ecc-badsig.pem +-k ./certs/ecc-key.pem + +# client verify should fail +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-H verifyFail diff --git a/tests/test.conf b/tests/test.conf index b8017c848..e6f72bfea 100644 --- a/tests/test.conf +++ b/tests/test.conf @@ -2199,6 +2199,7 @@ -v 3 -l NTRU-AES128-SHA +# error going into callback, return ok # server TLSv1.2 verify callback override -v 3 -l ECDHE-RSA-AES128-GCM-SHA256 @@ -2221,6 +2222,29 @@ -A ./certs/ca-ecc-cert.pem -j +# no error going into callback, return ok +# server TLSv1.2 verify callback override +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 +-c ./certs/server-cert.pem + +# client TLSv1.2 verify callback override +-v 3 +-l ECDHE-RSA-AES128-GCM-SHA256 +-j + +# server TLSv1.2 verify callback override +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-c ./certs/test/server-ecc.pem +-k ./certs/ecc-key.pem + +# client TLSv1.2 verify callback override +-v 3 +-l ECDHE-ECDSA-AES128-GCM-SHA256 +-A ./certs/ca-ecc-cert.pem +-j + # server TLSv1.2 ECDHE-EDCSA-CHACHA20-POLY1305 -v 3 -l ECDHE-ECDSA-CHACHA20-POLY1305