From 76e4cd37ab5fb1d29faf2f711137059b798cfa48 Mon Sep 17 00:00:00 2001 From: Andrew Gonzalez Date: Sat, 26 Sep 2026 17:04:01 -0400 Subject: [PATCH] buffer: make buf_valid() and buf_defined() NULL-safe buf_len() is written to tolerate an unusable buffer -- it returns 0 when buf_valid() says no. It cannot, because buf_valid() dereferences the pointer before testing anything: return likely(buf->data != NULL) && likely(buf->len >= 0); So buf_len(NULL) crashes inside its own validity check, and so does every other buf_* helper that gates on buf_valid(). buf_defined() has the same shape. This is not hypothetical. Commit bc7f77ea ("ssl: do not trust the peer's request to resend the wrapped client key") fixed a pre-authentication SIGSEGV whose proximate cause was exactly this: a peer-supplied EARLY_NEG_FLAG_RESEND_WKC made control_packet_needs_wkc() true on a client without --tls-crypt-v2, and write_outgoing_tls_ciphertext() then evaluated maxlen -= buf_len(session->tls_wrap.tls_crypt_v2_wkc); with a NULL tls_crypt_v2_wkc. That fix removed the one route to the sink, which was correct and sufficient for the bug at hand, but it left the sharp edge in place for the next caller: a predicate named "valid" that faults on the commonest invalid input. Add the NULL test to both predicates. Eight helpers in buffer.h gate on buf_valid(), so they all become NULL-tolerant, and callers that reasonably expect buf_len(NULL) == 0 get it. The checks are marked likely(), so the hot path is unchanged. Add a regression test covering NULL and freed buffers for buf_defined(), buf_valid() and buf_len(). It fails before this change and passes after. Signed-off-by: Andrew Gonzalez --- src/openvpn/buffer.h | 14 ++++++++------ tests/unit_tests/openvpn/test_buffer.c | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h index 22a8b74..58e10f0 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -392,26 +392,28 @@ clear_buf(void) * A defined buffer has been allocated but may have zero length or negative * len (i.e. it is not necessarily valid). * - * @param buf The buffer to test. + * @param buf The buffer to test. May be NULL, in which case the buffer is + * not defined. */ static inline bool buf_defined(const struct buffer *buf) { - return buf->data != NULL; + return likely(buf != NULL) && buf->data != NULL; } /** * Return true iff \c buf is valid. * - * A buffer is valid when its data pointer is non-NULL and its \c len is - * non-negative. + * A buffer is valid when it is non-NULL, its data pointer is non-NULL and its + * \c len is non-negative. * - * @param buf The buffer to test. + * @param buf The buffer to test. May be NULL, in which case the buffer is + * not valid. */ static inline bool buf_valid(const struct buffer *buf) { - return likely(buf->data != NULL) && likely(buf->len >= 0); + return likely(buf != NULL) && likely(buf->data != NULL) && likely(buf->len >= 0); } /** diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c index 7bd0c45..2945e8a 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -518,6 +518,28 @@ test_buffer_null_terminate(void **state) /* for building long texts */ #define A_TIMES_256 "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAO" +static void +test_buffer_null_predicates(void **state) +{ + struct buffer buf = alloc_buf(16); + + /* The validity predicates must tolerate a NULL buffer pointer rather than + * dereferencing it, so that the whole buf_* family is NULL-safe. */ + assert_false(buf_defined(NULL)); + assert_false(buf_valid(NULL)); + assert_int_equal(buf_len(NULL), 0); + + assert_true(buf_defined(&buf)); + assert_true(buf_valid(&buf)); + + free_buf(&buf); + + /* A freed buffer is neither defined nor valid. */ + assert_false(buf_defined(&buf)); + assert_false(buf_valid(&buf)); + assert_int_equal(buf_len(&buf), 0); +} + void test_buffer_parse(void **state) { @@ -596,6 +618,7 @@ main(void) cmocka_unit_test(test_checked_snprintf), cmocka_unit_test(test_buffer_chomp), cmocka_unit_test(test_buffer_null_terminate), + cmocka_unit_test(test_buffer_null_predicates), cmocka_unit_test(test_buffer_parse), cmocka_unit_test(test_buffer_extract_field) }; -- 2.55.0