From: Mounir IDRASSI Date: Wed, 29 Apr 2026 09:39:37 +0000 (+0900) Subject: Fix false success on zero BIO write X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=5f3db0d811329b600045842116a062520437c2a1;p=thirdparty%2Fopenssl.git Fix false success on zero BIO write The TLS record layer treated BIO_write returning 0 as success unless the BIO retry flag was set. For a write with data pending in the write buffer, this could report success to the application while leaving record data pending internally. Capture the pending buffer length before the transport write. If a positive length write returns zero, return retry when the BIO retry flag is set and fatal otherwise. Do not queue an SSL reason for the fatal zero-without-retry case: a custom BIO can return zero without retry without this implying an SSL library or protocol error, so SSL_get_error() reports SSL_ERROR_SYSCALL with an empty error queue. Preserve the KTLS empty fragment case where the pending length is already zero and a zero byte write is expected to succeed. For DTLS, the same zero return without retry condition is now treated as fatal; the failed buffer is still dropped in the existing DTLS write error path. This intentionally tightens the previous behavior, which could treat the zero return as success. Add regression coverage with a custom BIO that returns 0 without setting retry for a positive length write. Fixes #31009 Reviewed-by: Jakub Zelenka Reviewed-by: Matt Caswell Reviewed-by: Andrew Dinh MergeDate: Mon Aug 3 12:37:56 2026 (Merged from https://github.com/openssl/openssl/pull/31021) --- diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 0363bf23bf2..36304496e83 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -1849,13 +1849,14 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) { int i, ret; TLS_BUFFER *thiswb; - size_t tmpwrit = 0; + size_t tmpwrit = 0, left; if (rl->nextwbuf >= rl->numwpipes) return OSSL_RECORD_RETURN_SUCCESS; for (;;) { thiswb = &rl->wbuf[rl->nextwbuf]; + left = TLS_BUFFER_get_left(thiswb); clear_sys_error(); if (rl->bio != NULL) { @@ -1865,13 +1866,24 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) return ret; } i = BIO_write(rl->bio, (char *)&(TLS_BUFFER_get_buf(thiswb)[TLS_BUFFER_get_offset(thiswb)]), - (unsigned int)TLS_BUFFER_get_left(thiswb)); + (unsigned int)left); if (i >= 0) { tmpwrit = i; - if (i == 0 && BIO_should_retry(rl->bio)) - ret = OSSL_RECORD_RETURN_RETRY; - else + if (i == 0 && left != 0) { + if (BIO_should_retry(rl->bio)) { + ret = OSSL_RECORD_RETURN_RETRY; + } else { + /* + * Treat this as a fatal I/O condition. Do not queue an + * SSL reason: a zero return with no retry flag may come + * from a custom BIO and does not imply an SSL library + * or protocol error. + */ + ret = OSSL_RECORD_RETURN_FATAL; + } + } else { ret = OSSL_RECORD_RETURN_SUCCESS; + } } else { if (BIO_should_retry(rl->bio)) { ret = OSSL_RECORD_RETURN_RETRY; @@ -1894,7 +1906,7 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) * Treat i == 0 as success rather than an error for zero byte * writes to permit this case. */ - if (i >= 0 && tmpwrit == TLS_BUFFER_get_left(thiswb)) { + if (i >= 0 && tmpwrit == left) { TLS_BUFFER_set_left(thiswb, 0); TLS_BUFFER_add_offset(thiswb, tmpwrit); if (++(rl->nextwbuf) < rl->numwpipes) diff --git a/test/helpers/ssltestlib.c b/test/helpers/ssltestlib.c index 57b8c3e0403..0d056d4ea88 100644 --- a/test/helpers/ssltestlib.c +++ b/test/helpers/ssltestlib.c @@ -34,11 +34,13 @@ static int tls_dump_puts(BIO *bp, const char *str); #define BIO_TYPE_MEMPACKET_TEST 0x81 #define BIO_TYPE_ALWAYS_RETRY 0x82 #define BIO_TYPE_MAYBE_RETRY (0x83 | BIO_TYPE_FILTER) +#define BIO_TYPE_NO_RETRY_ZERO (0x84 | BIO_TYPE_FILTER) static BIO_METHOD *method_tls_dump = NULL; static BIO_METHOD *meth_mem = NULL; static BIO_METHOD *meth_always_retry = NULL; static BIO_METHOD *meth_maybe_retry = NULL; +static BIO_METHOD *meth_no_retry_zero = NULL; static int retry_err = -1; /* Note: Not thread safe! */ @@ -1011,6 +1013,10 @@ static int maybe_retry_new(BIO *bi); static int maybe_retry_free(BIO *a); static int maybe_retry_write(BIO *b, const char *in, int inl); static long maybe_retry_ctrl(BIO *b, int cmd, long num, void *ptr); +static int no_retry_zero_new(BIO *bi); +static int no_retry_zero_free(BIO *a); +static int no_retry_zero_write(BIO *b, const char *in, int inl); +static long no_retry_zero_ctrl(BIO *b, int cmd, long num, void *ptr); const BIO_METHOD *bio_s_maybe_retry(void) { @@ -1097,6 +1103,61 @@ static long maybe_retry_ctrl(BIO *bio, int cmd, long num, void *ptr) } } +const BIO_METHOD *bio_s_no_retry_zero(void) +{ + if (meth_no_retry_zero == NULL) { + if (!TEST_ptr(meth_no_retry_zero = BIO_meth_new(BIO_TYPE_NO_RETRY_ZERO, + "No Retry Zero")) + || !TEST_true(BIO_meth_set_write(meth_no_retry_zero, + no_retry_zero_write)) + || !TEST_true(BIO_meth_set_ctrl(meth_no_retry_zero, + no_retry_zero_ctrl)) + || !TEST_true(BIO_meth_set_create(meth_no_retry_zero, + no_retry_zero_new)) + || !TEST_true(BIO_meth_set_destroy(meth_no_retry_zero, + no_retry_zero_free))) + return NULL; + } + return meth_no_retry_zero; +} + +void bio_s_no_retry_zero_free(void) +{ + BIO_meth_free(meth_no_retry_zero); +} + +static int no_retry_zero_new(BIO *bio) +{ + BIO_set_init(bio, 1); + return 1; +} + +static int no_retry_zero_free(BIO *bio) +{ + BIO_set_data(bio, NULL); + BIO_set_init(bio, 0); + return 1; +} + +static int no_retry_zero_write(BIO *bio, const char *in, int inl) +{ + BIO_clear_retry_flags(bio); + return 0; +} + +static long no_retry_zero_ctrl(BIO *bio, int cmd, long num, void *ptr) +{ + BIO *next = BIO_next(bio); + + switch (cmd) { + case BIO_CTRL_FLUSH: + return next == NULL ? 1 : BIO_ctrl(next, cmd, num, ptr); + + default: + return next == NULL ? 0 : BIO_ctrl(next, cmd, num, ptr); + } +} + int create_ssl_ctx_pair(OSSL_LIB_CTX *libctx, const SSL_METHOD *sm, const SSL_METHOD *cm, int min_proto_version, int max_proto_version, SSL_CTX **sctx, SSL_CTX **cctx, diff --git a/test/helpers/ssltestlib.h b/test/helpers/ssltestlib.h index 68a10b24bd1..37063b5018c 100644 --- a/test/helpers/ssltestlib.h +++ b/test/helpers/ssltestlib.h @@ -59,6 +59,9 @@ void set_always_retry_err_val(int err); const BIO_METHOD *bio_s_maybe_retry(void); void bio_s_maybe_retry_free(void); +const BIO_METHOD *bio_s_no_retry_zero(void); +void bio_s_no_retry_zero_free(void); + /* Packet types - value 0 is reserved */ #define INJECT_PACKET 1 #define INJECT_PACKET_IGNORE_REC_SEQ 2 diff --git a/test/sslapitest.c b/test/sslapitest.c index 6fc18a0dc04..1c762016540 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -13556,6 +13556,70 @@ end: return testresult; } +/* + * Test that a BIO returning 0 without a retry flag for a write with a positive + * length is not treated as a successful write. + */ +static int test_data_write_zero_no_retry(int tst) +{ + SSL_CTX *cctx = NULL, *sctx = NULL; + SSL *clientssl = NULL, *serverssl = NULL; + BIO *bzero = BIO_new(bio_s_no_retry_zero()); + const SSL_METHOD *smeth = TLS_server_method(); + const SSL_METHOD *cmeth = TLS_client_method(); + unsigned char inbuf[1] = { 0 }; + size_t written; + unsigned long errcode; + int err, min_version = 0, max_version = 0, testresult = 0; + + if (tst == 1) { +#if !defined(OPENSSL_NO_DTLS) && !defined(OPENSSL_NO_DTLS1_2) + smeth = DTLS_server_method(); + cmeth = DTLS_client_method(); + min_version = max_version = DTLS1_2_VERSION; +#else + BIO_free(bzero); + return TEST_skip("DTLS 1.2 not supported"); +#endif + } + + if (!TEST_ptr(bzero)) + goto end; + + if (!TEST_true(create_ssl_ctx_pair(libctx, smeth, cmeth, min_version, + max_version, &sctx, &cctx, cert, privkey))) + goto end; + + if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, NULL, + NULL))) + goto end; + + if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE))) + goto end; + + SSL_set0_wbio(clientssl, bzero); + bzero = NULL; + + ERR_clear_error(); + if (!TEST_false(SSL_write_ex(clientssl, inbuf, sizeof(inbuf), &written))) + goto end; + + err = SSL_get_error(clientssl, 0); + errcode = ERR_get_error(); + if (!TEST_int_eq(err, SSL_ERROR_SYSCALL) + || !TEST_ulong_eq(errcode, 0)) + goto end; + + testresult = 1; +end: + SSL_free(serverssl); + SSL_free(clientssl); + SSL_CTX_free(sctx); + SSL_CTX_free(cctx); + BIO_free_all(bzero); + return testresult; +} + struct resume_servername_cb_data { int i; SSL_CTX *cctx; @@ -15598,6 +15662,7 @@ int setup_tests(void) ADD_TEST(test_rstate_string); ADD_ALL_TESTS(test_handshake_retry, 16); ADD_TEST(test_data_retry); + ADD_ALL_TESTS(test_data_write_zero_no_retry, 2); ADD_ALL_TESTS(test_multi_resume, 5); ADD_ALL_TESTS(test_select_next_proto, OSSL_NELEM(next_proto_tests)); #if !defined(OPENSSL_NO_TLS1_2) && !defined(OPENSSL_NO_NEXTPROTONEG) @@ -15649,6 +15714,7 @@ void cleanup_tests(void) bio_s_mempacket_test_free(); bio_s_always_retry_free(); bio_s_maybe_retry_free(); + bio_s_no_retry_zero_free(); OSSL_PROVIDER_unload(defctxnull); OSSL_LIB_CTX_free(libctx); }