From e6da540fb31aea1c8a280253459b31d9360ef081 Mon Sep 17 00:00:00 2001 From: Hayden Roche Date: Tue, 19 Jul 2022 15:03:46 -0700 Subject: [PATCH] Fix backwards behavior for various wolfSSL_ERR* functions. wolfSSL_ERR_get_error and wolfSSL_ERR_peek_error_line_data should return the earliest error in the queue (i.e. the error at the front), but prior to this commit, they returned the latest/most recent one instead. In DoAlert, we were adding an error to the queue for all alerts. However, a close_notify isn't really an error. This commit makes it so DoAlert only adds errors to the queue for non-close_notify alerts. In ReceiveData, similarly, we were adding an error to the queue when the peer sent a close_notify, as determined by ssl->error == ZERO_RETURN. Now, we don't add an error in this case. --- src/internal.c | 10 ++++++++-- src/ssl.c | 40 +++++++++++++++++++--------------------- tests/api.c | 24 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 23 deletions(-) diff --git a/src/internal.c b/src/internal.c index 57c4fffc9..bbd30a79a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -17854,7 +17854,13 @@ static int DoAlert(WOLFSSL* ssl, byte* input, word32* inOutIdx, int* type) if (*type == close_notify) { ssl->options.closeNotify = 1; } - WOLFSSL_ERROR(*type); + else { + /* + * A close_notify alert doesn't mean there's been an error, so we only + * add other types of alerts to the error queue + */ + WOLFSSL_ERROR(*type); + } if (IsEncryptionOn(ssl, 0)) { *inOutIdx += ssl->keys.padSz; @@ -21486,7 +21492,6 @@ startScr: while (ssl->buffers.clearOutputBuffer.length == 0) { if ( (ssl->error = ProcessReply(ssl)) < 0) { - WOLFSSL_ERROR(ssl->error); if (ssl->error == ZERO_RETURN) { WOLFSSL_MSG("Zero return, no more data coming"); return 0; /* no more data coming */ @@ -21499,6 +21504,7 @@ startScr: return 0; /* peer reset or closed */ } } + WOLFSSL_ERROR(ssl->error); return ssl->error; } diff --git a/src/ssl.c b/src/ssl.c index 91a452132..a966d2f78 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -16159,36 +16159,35 @@ cleanup: unsigned long wolfSSL_ERR_get_error(void) { + int ret; + WOLFSSL_ENTER("wolfSSL_ERR_get_error"); #ifdef WOLFSSL_HAVE_ERROR_QUEUE -#if defined(OPENSSL_ALL) || defined(WOLFSSL_NGINX) || defined(WOLFSSL_HAPROXY) - { - unsigned long ret = wolfSSL_ERR_peek_error_line_data(NULL, NULL, - NULL, NULL); - wc_RemoveErrorNode(-1); - return ret; - } -#else - { - int ret = wc_PullErrorNode(NULL, NULL, NULL); - - if (ret < 0) { - if (ret == BAD_STATE_E) return 0; /* no errors in queue */ + ret = wc_PullErrorNode(NULL, NULL, NULL); + if (ret < 0) { + if (ret == BAD_STATE_E) { + ret = 0; /* no errors in queue */ + } + else { WOLFSSL_MSG("Error with pulling error node!"); WOLFSSL_LEAVE("wolfSSL_ERR_get_error", ret); ret = 0 - ret; /* return absolute value of error */ - /* panic and try to clear out nodes */ wc_ClearErrorNodes(); } - - return (unsigned long)ret; } -#endif + else { + wc_RemoveErrorNode(0); + } + + return ret; #else + + (void)ret; + return (unsigned long)(0 - NOT_COMPILED_IN); -#endif +#endif /* WOLFSSL_HAVE_ERROR_QUEUE */ } #ifdef WOLFSSL_HAVE_ERROR_QUEUE @@ -18545,7 +18544,6 @@ size_t wolfSSL_get_client_random(const WOLFSSL* ssl, unsigned char* out, #endif /* DEBUG_WOLFSSL */ - /* @TODO when having an error queue this needs to push to the queue */ void wolfSSL_ERR_put_error(int lib, int fun, int err, const char* file, int line) { @@ -32345,7 +32343,7 @@ unsigned long wolfSSL_ERR_peek_error_line_data(const char **file, int *line, int ret = 0; while (1) { - ret = wc_PeekErrorNode(-1, file, NULL, line); + ret = wc_PeekErrorNode(0, file, NULL, line); if (ret == BAD_MUTEX_E || ret == BAD_FUNC_ARG || ret == BAD_STATE_E) { WOLFSSL_MSG("Issue peeking at error node in queue"); return 0; @@ -32372,7 +32370,7 @@ unsigned long wolfSSL_ERR_peek_error_line_data(const char **file, int *line, ret != -SOCKET_PEER_CLOSED_E && ret != -SOCKET_ERROR_E) break; - wc_RemoveErrorNode(-1); + wc_RemoveErrorNode(0); } return (unsigned long)ret; diff --git a/tests/api.c b/tests/api.c index d0107a93d..e043b0cea 100644 --- a/tests/api.c +++ b/tests/api.c @@ -38085,6 +38085,29 @@ static void test_wolfSSL_ERR_put_error(void) #endif } +/* + * This is a regression test for a bug where the peek/get error functions were + * drawing from the end of the queue rather than the front. + */ +static void test_wolfSSL_ERR_get_error_order(void) +{ +#ifdef WOLFSSL_HAVE_ERROR_QUEUE + printf(testingFmt, "test_wolfSSL_ERR_get_error_order"); + + /* Empty the queue. */ + wolfSSL_ERR_clear_error(); + + wolfSSL_ERR_put_error(0, 0, ASN_NO_SIGNER_E, "test", 0); + wolfSSL_ERR_put_error(0, 0, ASN_SELF_SIGNED_E, "test", 0); + + AssertIntEQ(wolfSSL_ERR_peek_error(), -ASN_NO_SIGNER_E); + AssertIntEQ(wolfSSL_ERR_get_error(), -ASN_NO_SIGNER_E); + AssertIntEQ(wolfSSL_ERR_peek_error(), -ASN_SELF_SIGNED_E); + AssertIntEQ(wolfSSL_ERR_get_error(), -ASN_SELF_SIGNED_E); + + printf(resultFmt, passed); +#endif /* WOLFSSL_HAVE_ERROR_QUEUE */ +} #ifndef NO_BIO @@ -56016,6 +56039,7 @@ void ApiTest(void) test_wolfSSL_PKCS8_d2i(); test_error_queue_per_thread(); test_wolfSSL_ERR_put_error(); + test_wolfSSL_ERR_get_error_order(); #ifndef NO_BIO test_wolfSSL_ERR_print_errors(); #endif