From cb5bb8916fa0e044e6658c8b3db6d7c672cb25fe Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 11 Apr 2025 14:19:46 +0100 Subject: [PATCH] Fix errors on SSL_accept() and SSL_get_error() MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Calling SSL_accept() was raising two errors on the stack if you passed the wrong object type. Similarly SSL_get_error() was adding an error to the stack if the wrong object type was passed and returning the wrong result. We also ensure SSL_set_accept_state() and SSL_set_connect_state() don't raise spurious errors since these are void functions. Fixes #27347 Fixes #27348 Reviewed-by: Saša Nedvědický Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/27351) --- include/internal/quic_ssl.h | 4 +- ssl/quic/quic_impl.c | 76 +++++++++++++++++++++++++++++-------- ssl/ssl_lib.c | 6 ++- 3 files changed, 66 insertions(+), 20 deletions(-) diff --git a/include/internal/quic_ssl.h b/include/internal/quic_ssl.h index acfeb0d5150..e7fd260ab60 100644 --- a/include/internal/quic_ssl.h +++ b/include/internal/quic_ssl.h @@ -72,8 +72,8 @@ __owur const SSL_CIPHER *ossl_quic_get_cipher(unsigned int u); int ossl_quic_renegotiate_check(SSL *ssl, int initok); int ossl_quic_do_handshake(SSL *s); -void ossl_quic_set_connect_state(SSL *s); -void ossl_quic_set_accept_state(SSL *s); +int ossl_quic_set_connect_state(SSL *s, int raiseerrs); +int ossl_quic_set_accept_state(SSL *s, int raiseerrs); __owur int ossl_quic_has_pending(const SSL *s); __owur int ossl_quic_handle_events(SSL *s); diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index 847639b89e5..64da2be4afc 100644 --- a/ssl/quic/quic_impl.c +++ b/ssl/quic/quic_impl.c @@ -186,6 +186,10 @@ static int quic_raise_non_normal_error(QCTX *ctx, * This determines whether SSL_get_error() is updated; the value it returns * is modified only by an I/O call. * + * QCTX_NO_ERROR + * Don't raise an error if the object type is wrong. Should not be used in + * conjunction with any flags that may raise errors not related to a wrong + * object type. */ #define QCTX_C (1U << 0) #define QCTX_S (1U << 1) @@ -195,6 +199,7 @@ static int quic_raise_non_normal_error(QCTX *ctx, #define QCTX_LOCK (1U << 5) #define QCTX_IO (1U << 6) #define QCTX_D (1U << 7) +#define QCTX_NO_ERROR (1U << 8) /* * Called when expect_quic failed. Used to diagnose why such a call failed and @@ -205,7 +210,9 @@ static int wrong_type(const SSL *s, uint32_t flags) const uint32_t mask = QCTX_C | QCTX_S | QCTX_L | QCTX_D; int code = ERR_R_UNSUPPORTED; - if ((flags & mask) == QCTX_D) + if ((flags & QCTX_NO_ERROR) != 0) + return 1; + else if ((flags & mask) == QCTX_D) code = SSL_R_DOMAIN_USE_ONLY; else if ((flags & mask) == QCTX_L) code = SSL_R_LISTENER_USE_ONLY; @@ -225,7 +232,8 @@ static int wrong_type(const SSL *s, uint32_t flags) * * After this returns 1, all fields of the passed QCTX are initialised. * Returns 0 on failure. This function is intended to be used to provide API - * semantics and as such, it invokes QUIC_RAISE_NON_NORMAL_ERROR() on failure. + * semantics and as such, it invokes QUIC_RAISE_NON_NORMAL_ERROR() on failure + * unless the QCTX_NO_ERROR flag is set. * * The flags argument controls the preconditions and postconditions of this * function. See above for the different flags. @@ -378,6 +386,25 @@ err: return ok; } +static int is_quic_c(const SSL *s, QCTX *ctx, int raiseerrs) +{ + uint32_t flags = QCTX_C; + + if (!raiseerrs) + flags |= QCTX_NO_ERROR; + return expect_quic_as(s, ctx, flags); +} + +/* Same as expect_quic_cs except that errors are not raised if raiseerrs == 0 */ +static int is_quic_cs(const SSL *s, QCTX *ctx, int raiseerrs) +{ + uint32_t flags = QCTX_C | QCTX_S; + + if (!raiseerrs) + flags |= QCTX_NO_ERROR; + return expect_quic_as(s, ctx, flags); +} + static int expect_quic_cs(const SSL *s, QCTX *ctx) { return expect_quic_as(s, ctx, QCTX_C | QCTX_S); @@ -1672,33 +1699,47 @@ long ossl_quic_ctrl(SSL *s, int cmd, long larg, void *parg) } /* SSL_set_connect_state */ -void ossl_quic_set_connect_state(SSL *s) +int ossl_quic_set_connect_state(SSL *s, int raiseerrs) { QCTX ctx; - if (!expect_quic_cs(s, &ctx)) - return; + if (!is_quic_c(s, &ctx, raiseerrs)) + return 0; + + if (ctx.qc->as_server_state == 0) + return 1; /* Cannot be changed after handshake started */ - if (ctx.qc->started || ctx.is_stream) - return; + if (ctx.qc->started) { + if (raiseerrs) + QUIC_RAISE_NON_NORMAL_ERROR(NULL, SSL_R_INVALID_COMMAND, NULL); + return 0; + } ctx.qc->as_server_state = 0; + return 1; } /* SSL_set_accept_state */ -void ossl_quic_set_accept_state(SSL *s) +int ossl_quic_set_accept_state(SSL *s, int raiseerrs) { QCTX ctx; - if (!expect_quic_cs(s, &ctx)) - return; + if (!is_quic_c(s, &ctx, raiseerrs)) + return 0; + + if (ctx.qc->as_server_state == 1) + return 1; /* Cannot be changed after handshake started */ - if (ctx.qc->started || ctx.is_stream) - return; + if (ctx.qc->started) { + if (raiseerrs) + QUIC_RAISE_NON_NORMAL_ERROR(NULL, SSL_R_INVALID_COMMAND, NULL); + return 0; + } ctx.qc->as_server_state = 1; + return 1; } /* SSL_do_handshake */ @@ -1983,7 +2024,8 @@ int ossl_quic_do_handshake(SSL *s) int ossl_quic_connect(SSL *s) { /* Ensure we are in connect state (no-op if non-idle). */ - ossl_quic_set_connect_state(s); + if (!ossl_quic_set_connect_state(s, 1)) + return -1; /* Begin or continue the handshake */ return ossl_quic_do_handshake(s); @@ -1993,7 +2035,8 @@ int ossl_quic_connect(SSL *s) int ossl_quic_accept(SSL *s) { /* Ensure we are in accept state (no-op if non-idle). */ - ossl_quic_set_accept_state(s); + if (!ossl_quic_set_accept_state(s, 1)) + return -1; /* Begin or continue the handshake */ return ossl_quic_do_handshake(s); @@ -2325,8 +2368,9 @@ int ossl_quic_get_error(const SSL *s, int i) QCTX ctx; int net_error, last_error; - if (!expect_quic_cs(s, &ctx)) - return 0; + /* SSL_get_errors() should not raise new errors */ + if (!is_quic_cs(s, &ctx, 0 /* suppress errors */)) + return SSL_ERROR_SSL; qctx_lock(&ctx); net_error = ossl_quic_channel_net_error(ctx.qc->ch); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index a81b16c1097..efc6d66fb9c 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -4980,7 +4980,8 @@ void SSL_set_accept_state(SSL *s) #ifndef OPENSSL_NO_QUIC if (IS_QUIC(s)) { - ossl_quic_set_accept_state(s); + /* We suppress errors because this is a void function */ + (void)ossl_quic_set_accept_state(s, 0 /* suppress errors */); return; } #endif @@ -4999,7 +5000,8 @@ void SSL_set_connect_state(SSL *s) #ifndef OPENSSL_NO_QUIC if (IS_QUIC(s)) { - ossl_quic_set_connect_state(s); + /* We suppress errors because this is a void function */ + (void)ossl_quic_set_connect_state(s, 0 /* suppress errors */); return; } #endif -- 2.47.2