From 7f9fda13e5d650f50e3d0233275d3a10275bd184 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Wed, 21 Aug 2024 18:36:23 +0000 Subject: [PATCH 1/8] fix: resolve UBSAN violations in the codebase * make EXPECT_BYTEARRAY_EQUAL to do memcmp when length input isn't zero * proceed regardless of inputs if length is set to be zero * set results of null arithmetic to null pointer instead of undefine --- stuffer/s2n_stuffer.c | 4 ++-- tests/s2n_test.h | 10 +++++++++- tests/testlib/s2n_test_server_client.c | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index a76a2f41546..e0e41d71f22 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -327,7 +327,7 @@ int s2n_stuffer_write_bytes(struct s2n_stuffer *stuffer, const uint8_t *data, co POSIX_PRECONDITION(s2n_stuffer_validate(stuffer)); POSIX_GUARD(s2n_stuffer_skip_write(stuffer, size)); - void *ptr = stuffer->blob.data + stuffer->write_cursor - size; + void *ptr = (stuffer->blob.data == NULL) ? NULL : stuffer->blob.data + stuffer->write_cursor - size; POSIX_ENSURE(S2N_MEM_IS_READABLE(ptr, size), S2N_ERR_NULL); if (ptr == data) { @@ -437,7 +437,7 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) POSIX_ENSURE_REF(stuffer); struct s2n_stuffer copy = *stuffer; POSIX_GUARD(s2n_stuffer_rewrite(©)); - uint8_t *data = stuffer->blob.data + stuffer->read_cursor; + uint8_t *data = (stuffer->blob.data == NULL) ? NULL : stuffer->blob.data + stuffer->read_cursor; uint32_t data_size = s2n_stuffer_data_available(stuffer); POSIX_GUARD(s2n_stuffer_write_bytes(©, data, data_size)); *stuffer = copy; diff --git a/tests/s2n_test.h b/tests/s2n_test.h index 7b79981355d..a512a155f8d 100644 --- a/tests/s2n_test.h +++ b/tests/s2n_test.h @@ -200,7 +200,15 @@ int test_count; /* for use with S2N_RESULT */ #define EXPECT_OK( function_call ) EXPECT_TRUE( s2n_result_is_ok(function_call) ) -#define EXPECT_BYTEARRAY_EQUAL( p1, p2, l ) EXPECT_EQUAL( memcmp( (p1), (p2), (l) ), 0 ) +#define EXPECT_BYTEARRAY_EQUAL( p1, p2, l ) \ + do { \ + if (l != 0) { \ + EXPECT_EQUAL( memcmp( (p1), (p2), (l) ), 0 ); \ + } else { \ + EXPECT_EQUAL(0, 0); \ + } \ + } while (0) + #define EXPECT_BYTEARRAY_NOT_EQUAL( p1, p2, l ) EXPECT_NOT_EQUAL( memcmp( (p1), (p2), (l) ), 0 ) #define EXPECT_STRING_EQUAL( p1, p2 ) EXPECT_EQUAL( strcmp( (p1), (p2) ), 0 ) diff --git a/tests/testlib/s2n_test_server_client.c b/tests/testlib/s2n_test_server_client.c index ecaec2a752f..4339b42e8ff 100644 --- a/tests/testlib/s2n_test_server_client.c +++ b/tests/testlib/s2n_test_server_client.c @@ -78,7 +78,7 @@ S2N_RESULT s2n_negotiate_test_server_and_client_with_early_data(struct s2n_conne do { if (!client_early_done) { bool success = s2n_send_early_data(client_conn, - early_data_to_send->data + total_data_sent, + (early_data_to_send->data == NULL) ? NULL : early_data_to_send->data + total_data_sent, early_data_to_send->size - total_data_sent, &data_sent, &blocked) == S2N_SUCCESS; From 54ee04fcc57f2254d2f87e344dc28d09229ef3a2 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 22 Aug 2024 16:55:53 +0000 Subject: [PATCH 2/8] fix: presever the original intension of stuffer write and shift function. * Check for s2n_stuffer_write_bytes. Exit if no byte is written. * Check s2n_stuffer_shift's read_cursor. Don't add read_cursor to blob.data if it is zero. * Simplify EXPECT_BYTEARRAY_EQUAL logic. --- stuffer/s2n_stuffer.c | 16 ++++++++++++++-- tests/s2n_test.h | 3 +-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index e0e41d71f22..680799cfa99 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -323,11 +323,17 @@ int s2n_stuffer_write(struct s2n_stuffer *stuffer, const struct s2n_blob *in) int s2n_stuffer_write_bytes(struct s2n_stuffer *stuffer, const uint8_t *data, const uint32_t size) { + + /* Exit when no bytes is written */ + if (size == 0) { + return S2N_SUCCESS; + } + POSIX_ENSURE(S2N_MEM_IS_READABLE(data, size), S2N_ERR_SAFETY); POSIX_PRECONDITION(s2n_stuffer_validate(stuffer)); POSIX_GUARD(s2n_stuffer_skip_write(stuffer, size)); - void *ptr = (stuffer->blob.data == NULL) ? NULL : stuffer->blob.data + stuffer->write_cursor - size; + void *ptr = stuffer->blob.data + stuffer->write_cursor - size; POSIX_ENSURE(S2N_MEM_IS_READABLE(ptr, size), S2N_ERR_NULL); if (ptr == data) { @@ -437,7 +443,13 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) POSIX_ENSURE_REF(stuffer); struct s2n_stuffer copy = *stuffer; POSIX_GUARD(s2n_stuffer_rewrite(©)); - uint8_t *data = (stuffer->blob.data == NULL) ? NULL : stuffer->blob.data + stuffer->read_cursor; + uint8_t *data = stuffer->blob.data; + + /* Prevent null arithmetic if stuffer->read_cursor is zero */ + if (stuffer->read_cursor != 0) { + data += stuffer->read_cursor; + } + uint32_t data_size = s2n_stuffer_data_available(stuffer); POSIX_GUARD(s2n_stuffer_write_bytes(©, data, data_size)); *stuffer = copy; diff --git a/tests/s2n_test.h b/tests/s2n_test.h index a512a155f8d..379f635702e 100644 --- a/tests/s2n_test.h +++ b/tests/s2n_test.h @@ -200,12 +200,11 @@ int test_count; /* for use with S2N_RESULT */ #define EXPECT_OK( function_call ) EXPECT_TRUE( s2n_result_is_ok(function_call) ) +/* if length is zero, then procced, otherwise, do memcmp to check if two memory blocks are the same*/ #define EXPECT_BYTEARRAY_EQUAL( p1, p2, l ) \ do { \ if (l != 0) { \ EXPECT_EQUAL( memcmp( (p1), (p2), (l) ), 0 ); \ - } else { \ - EXPECT_EQUAL(0, 0); \ } \ } while (0) From 25bb07bd546870dead24d62f7ae01513db670b12 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 22 Aug 2024 17:38:15 +0000 Subject: [PATCH 3/8] Run clang-format to properly format the s2n_stuffer.c --- stuffer/s2n_stuffer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 680799cfa99..0752a31927d 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -323,7 +323,6 @@ int s2n_stuffer_write(struct s2n_stuffer *stuffer, const struct s2n_blob *in) int s2n_stuffer_write_bytes(struct s2n_stuffer *stuffer, const uint8_t *data, const uint32_t size) { - /* Exit when no bytes is written */ if (size == 0) { return S2N_SUCCESS; @@ -449,7 +448,7 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) if (stuffer->read_cursor != 0) { data += stuffer->read_cursor; } - + uint32_t data_size = s2n_stuffer_data_available(stuffer); POSIX_GUARD(s2n_stuffer_write_bytes(©, data, data_size)); *stuffer = copy; From 4dc9f8076b1b8b35c56f5bc90e4e0ef0d2c84266 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 22 Aug 2024 18:30:50 +0000 Subject: [PATCH 4/8] Address PR comments: * Update the comments in s2n_stuffer_shift. --- stuffer/s2n_stuffer.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 0752a31927d..989403aef03 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -442,9 +442,13 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) POSIX_ENSURE_REF(stuffer); struct s2n_stuffer copy = *stuffer; POSIX_GUARD(s2n_stuffer_rewrite(©)); + /** + * Adding 0 to a null value is undefine behavior. + * This if statement prevents undefined behavior in the case + * where stuffer->blob.data is NULL and stuffer->read_cursor is 0. + */ uint8_t *data = stuffer->blob.data; - /* Prevent null arithmetic if stuffer->read_cursor is zero */ if (stuffer->read_cursor != 0) { data += stuffer->read_cursor; } From cafb7b4537a1ad1e240192c7ce77b900f10f95c8 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 22 Aug 2024 23:10:45 +0000 Subject: [PATCH 5/8] Address PR comments: * Modifying comments to be more precise. * Deleteling some unnecessary white space. --- stuffer/s2n_stuffer.c | 9 +++------ tests/s2n_test.h | 4 +++- tests/testlib/s2n_test_server_client.c | 9 ++++++++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 989403aef03..7e770878d38 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -323,7 +323,6 @@ int s2n_stuffer_write(struct s2n_stuffer *stuffer, const struct s2n_blob *in) int s2n_stuffer_write_bytes(struct s2n_stuffer *stuffer, const uint8_t *data, const uint32_t size) { - /* Exit when no bytes is written */ if (size == 0) { return S2N_SUCCESS; } @@ -442,13 +441,11 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) POSIX_ENSURE_REF(stuffer); struct s2n_stuffer copy = *stuffer; POSIX_GUARD(s2n_stuffer_rewrite(©)); - /** - * Adding 0 to a null value is undefine behavior. - * This if statement prevents undefined behavior in the case - * where stuffer->blob.data is NULL and stuffer->read_cursor is 0. + + /* Adding 0 to a null value is undefined behavior. + * This prevents undefined behavior in the case where the read cursor is 0. */ uint8_t *data = stuffer->blob.data; - if (stuffer->read_cursor != 0) { data += stuffer->read_cursor; } diff --git a/tests/s2n_test.h b/tests/s2n_test.h index 379f635702e..6281363982a 100644 --- a/tests/s2n_test.h +++ b/tests/s2n_test.h @@ -200,7 +200,9 @@ int test_count; /* for use with S2N_RESULT */ #define EXPECT_OK( function_call ) EXPECT_TRUE( s2n_result_is_ok(function_call) ) -/* if length is zero, then procced, otherwise, do memcmp to check if two memory blocks are the same*/ +/* Memcmp is annotated with a non-null attribute. The if statement prevents + * calling memcmp with NULL arguments when the length is zero. + */ #define EXPECT_BYTEARRAY_EQUAL( p1, p2, l ) \ do { \ if (l != 0) { \ diff --git a/tests/testlib/s2n_test_server_client.c b/tests/testlib/s2n_test_server_client.c index 4339b42e8ff..381f13f2520 100644 --- a/tests/testlib/s2n_test_server_client.c +++ b/tests/testlib/s2n_test_server_client.c @@ -77,8 +77,15 @@ S2N_RESULT s2n_negotiate_test_server_and_client_with_early_data(struct s2n_conne bool server_early_done = false, client_early_done = false; do { if (!client_early_done) { + /* Adding 0 to a null value is undefined behavior. + * This prevents undefined behavior in the case where the offset is 0. + */ + uint8_t *early_data = early_data_to_send->data; + if (total_data_sent != 0) { + early_data += total_data_sent; + } bool success = s2n_send_early_data(client_conn, - (early_data_to_send->data == NULL) ? NULL : early_data_to_send->data + total_data_sent, + early_data, early_data_to_send->size - total_data_sent, &data_sent, &blocked) == S2N_SUCCESS; From ca483f1ef74d16268e850bcee6a7491546277756 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 22 Aug 2024 23:45:49 +0000 Subject: [PATCH 6/8] Address PR comments: * relocate EXPECT_BYTEARRAY_EQUAL comments to within the function * change variable name to make it more specific --- tests/s2n_test.h | 6 +++--- tests/testlib/s2n_test_server_client.c | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/s2n_test.h b/tests/s2n_test.h index 6281363982a..00c6338e69c 100644 --- a/tests/s2n_test.h +++ b/tests/s2n_test.h @@ -200,11 +200,11 @@ int test_count; /* for use with S2N_RESULT */ #define EXPECT_OK( function_call ) EXPECT_TRUE( s2n_result_is_ok(function_call) ) -/* Memcmp is annotated with a non-null attribute. The if statement prevents - * calling memcmp with NULL arguments when the length is zero. - */ #define EXPECT_BYTEARRAY_EQUAL( p1, p2, l ) \ do { \ + /* Memcmp is annotated with a non-null attribute. The if statement prevents \ + * calling memcmp with NULL arguments when the length is zero. \ + */ \ if (l != 0) { \ EXPECT_EQUAL( memcmp( (p1), (p2), (l) ), 0 ); \ } \ diff --git a/tests/testlib/s2n_test_server_client.c b/tests/testlib/s2n_test_server_client.c index 381f13f2520..0e877b0c324 100644 --- a/tests/testlib/s2n_test_server_client.c +++ b/tests/testlib/s2n_test_server_client.c @@ -78,14 +78,14 @@ S2N_RESULT s2n_negotiate_test_server_and_client_with_early_data(struct s2n_conne do { if (!client_early_done) { /* Adding 0 to a null value is undefined behavior. - * This prevents undefined behavior in the case where the offset is 0. - */ - uint8_t *early_data = early_data_to_send->data; + * This prevents undefined behavior in the case where the offset is 0. + */ + uint8_t *next_early_data_to_send = early_data_to_send->data; if (total_data_sent != 0) { - early_data += total_data_sent; + next_early_data_to_send += total_data_sent; } bool success = s2n_send_early_data(client_conn, - early_data, + next_early_data_to_send, early_data_to_send->size - total_data_sent, &data_sent, &blocked) == S2N_SUCCESS; From 9b64d77f46caea391407a2e355e134e60cd5eb28 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Fri, 23 Aug 2024 00:27:19 +0000 Subject: [PATCH 7/8] Address PR comments: * remove comments for MACRO * comdense comments for stuffer and test --- stuffer/s2n_stuffer.c | 4 +--- tests/s2n_test.h | 3 --- tests/testlib/s2n_test_server_client.c | 4 +--- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 7e770878d38..88dda7ca37c 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -442,9 +442,7 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) struct s2n_stuffer copy = *stuffer; POSIX_GUARD(s2n_stuffer_rewrite(©)); - /* Adding 0 to a null value is undefined behavior. - * This prevents undefined behavior in the case where the read cursor is 0. - */ + /* Adding 0 to a NULL value is undefined behavior */ uint8_t *data = stuffer->blob.data; if (stuffer->read_cursor != 0) { data += stuffer->read_cursor; diff --git a/tests/s2n_test.h b/tests/s2n_test.h index 00c6338e69c..696ba68ed0d 100644 --- a/tests/s2n_test.h +++ b/tests/s2n_test.h @@ -202,9 +202,6 @@ int test_count; #define EXPECT_BYTEARRAY_EQUAL( p1, p2, l ) \ do { \ - /* Memcmp is annotated with a non-null attribute. The if statement prevents \ - * calling memcmp with NULL arguments when the length is zero. \ - */ \ if (l != 0) { \ EXPECT_EQUAL( memcmp( (p1), (p2), (l) ), 0 ); \ } \ diff --git a/tests/testlib/s2n_test_server_client.c b/tests/testlib/s2n_test_server_client.c index 0e877b0c324..8ed01012c11 100644 --- a/tests/testlib/s2n_test_server_client.c +++ b/tests/testlib/s2n_test_server_client.c @@ -77,9 +77,7 @@ S2N_RESULT s2n_negotiate_test_server_and_client_with_early_data(struct s2n_conne bool server_early_done = false, client_early_done = false; do { if (!client_early_done) { - /* Adding 0 to a null value is undefined behavior. - * This prevents undefined behavior in the case where the offset is 0. - */ + /* Adding 0 to a NULL value is undefined behavior */ uint8_t *next_early_data_to_send = early_data_to_send->data; if (total_data_sent != 0) { next_early_data_to_send += total_data_sent; From 87af4c8d0902360c044e8e03eff28fb888b02be5 Mon Sep 17 00:00:00 2001 From: Boquan Fang Date: Thu, 22 Aug 2024 20:09:15 -0700 Subject: [PATCH 8/8] Update stuffer/s2n_stuffer.c Change comment location Co-authored-by: Lindsay Stewart --- stuffer/s2n_stuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stuffer/s2n_stuffer.c b/stuffer/s2n_stuffer.c index 88dda7ca37c..134ec0ba9c8 100644 --- a/stuffer/s2n_stuffer.c +++ b/stuffer/s2n_stuffer.c @@ -442,8 +442,8 @@ int s2n_stuffer_shift(struct s2n_stuffer *stuffer) struct s2n_stuffer copy = *stuffer; POSIX_GUARD(s2n_stuffer_rewrite(©)); - /* Adding 0 to a NULL value is undefined behavior */ uint8_t *data = stuffer->blob.data; + /* Adding 0 to a NULL value is undefined behavior */ if (stuffer->read_cursor != 0) { data += stuffer->read_cursor; }