From 7b61836e95a14e96c36b3a16cbdc52e8e4cfdf80 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Mon, 25 Jan 2021 13:31:35 -0800 Subject: [PATCH 1/9] api: add method to append protocol preferences --- api/s2n.h | 26 ++++ tests/unit/s2n_protocol_preferences_test.c | 164 +++++++++++++++++++++ tls/s2n_protocol_preferences.c | 71 ++++++--- 3 files changed, 244 insertions(+), 17 deletions(-) create mode 100644 tests/unit/s2n_protocol_preferences_test.c diff --git a/api/s2n.h b/api/s2n.h index 0dfa21bf26e..a2ebe088eb2 100644 --- a/api/s2n.h +++ b/api/s2n.h @@ -219,6 +219,19 @@ S2N_API extern int s2n_config_add_dhparams(struct s2n_config *config, const char *dhparams_pem); S2N_API extern int s2n_config_set_cipher_preferences(struct s2n_config *config, const char *version); + +/** + * Appends the provided application protocol to the preference list + * + * The data provided in `protocol` parameter will be copied into an internal buffer + * + * @param config The configuration object being updated + * @param protocol A pointer to a slice of bytes + * @param protocol_len The length of bytes that should be read from `protocol`. Note this value cannot exceed `255`. + */ +S2N_API +extern int s2n_config_append_protocol_preference(struct s2n_config *config, const uint8_t *protocol, size_t protocol_len); + S2N_API extern int s2n_config_set_protocol_preferences(struct s2n_config *config, const char * const *protocols, int protocol_count); typedef enum { S2N_STATUS_REQUEST_NONE = 0, S2N_STATUS_REQUEST_OCSP = 1 } s2n_status_request_type; @@ -328,6 +341,19 @@ extern uint64_t s2n_connection_get_delay(struct s2n_connection *conn); S2N_API extern int s2n_connection_set_cipher_preferences(struct s2n_connection *conn, const char *version); + +/** + * Appends the provided application protocol to the preference list + * + * The data provided in `protocol` parameter will be copied into an internal buffer + * + * @param conn The connection object being updated + * @param protocol A pointer to a slice of bytes + * @param protocol_len The length of bytes that should be read from `protocol`. Note this value cannot exceed `255`. + */ +S2N_API +extern int s2n_connection_append_protocol_preference(struct s2n_connection *conn, const uint8_t *protocol, size_t protocol_len); + S2N_API extern int s2n_connection_set_protocol_preferences(struct s2n_connection *conn, const char * const *protocols, int protocol_count); S2N_API diff --git a/tests/unit/s2n_protocol_preferences_test.c b/tests/unit/s2n_protocol_preferences_test.c new file mode 100644 index 00000000000..75235be9ede --- /dev/null +++ b/tests/unit/s2n_protocol_preferences_test.c @@ -0,0 +1,164 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +#include "s2n_test.h" +#include "tls/s2n_connection.h" + +#include + +int main(int argc, char **argv) +{ + BEGIN_TEST(); + + /* Test config append */ + { + struct s2n_config *config; + EXPECT_NOT_NULL(config = s2n_config_new()); + EXPECT_EQUAL(config->application_protocols.size, 0); + size_t prev_size = 0; + + /* should grow the blob with the provided value */ + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)"protocol1", 9)); + EXPECT_EQUAL(config->application_protocols.size, 1 /* len prefix */ + 9); + prev_size = config->application_protocols.size; + + /* should grow the blob even more */ + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)"protocol2", 9)); + EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 9); + prev_size = config->application_protocols.size; + + /* should allow null byte values */ + const uint8_t null_value[9] = { 0 }; + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)null_value, 9)); + EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 9); + prev_size = config->application_protocols.size; + + /* should reallocate the blob */ + const uint8_t large_value[255] = { 0 }; + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)large_value, 255)); + EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 255); + prev_size = config->application_protocols.size; + + /* should limit the length of the protocol value */ + const uint8_t oversized[256] = { 0 }; + EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)oversized, 265)); + EXPECT_EQUAL(config->application_protocols.size, prev_size); + + EXPECT_SUCCESS(s2n_config_free(config)); + } + + /* Test connection append */ + { + struct s2n_connection *conn; + EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); + size_t prev_size = 0; + + /* should grow the blob with the provided value */ + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)"protocol1", 9)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 1 /* len prefix */ + 9); + prev_size = conn->application_protocols_overridden.size; + + /* should grow the blob even more */ + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)"protocol2", 9)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 9); + prev_size = conn->application_protocols_overridden.size; + + /* should allow null byte values */ + const uint8_t null_value[9] = { 0 }; + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)null_value, 9)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 9); + prev_size = conn->application_protocols_overridden.size; + + /* should reallocate the blob */ + const uint8_t large_value[255] = { 0 }; + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)large_value, 255)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 255); + prev_size = conn->application_protocols_overridden.size; + + /* should limit the length of the protocol value */ + const uint8_t oversized[256] = { 0 }; + EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)oversized, 265)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size); + + EXPECT_SUCCESS(s2n_connection_free(conn)); + } + + const char *protocols[] = { "protocol1", "protocol2", "protocol3" }; + const uint8_t protocols_count = s2n_array_len(protocols); + + /* Test config set */ + { + struct s2n_config *config; + EXPECT_NOT_NULL(config = s2n_config_new()); + EXPECT_EQUAL(config->application_protocols.size, 0); + + /* should copy the preference list */ + EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, protocols_count)); + EXPECT_EQUAL(config->application_protocols.size, (1 /* len prefix */ + 9) * protocols_count); + + /* should clear the preference list */ + EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, NULL, protocols_count)); + EXPECT_EQUAL(config->application_protocols.size, 0); + + EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, protocols_count)); + EXPECT_EQUAL(config->application_protocols.size, (1 /* len prefix */ + 9) * protocols_count); + EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, 0)); + EXPECT_EQUAL(config->application_protocols.size, 0); + + /* should limit the length of the protocol value */ + char oversized[257] = { 0 }; + memset(&oversized, 1, 256); + EXPECT_EQUAL(strlen(oversized), 256); + const char *oversized_p[] = { (const char *)&oversized }; + EXPECT_FAILURE(s2n_config_set_protocol_preferences(config, (const char * const *)oversized_p, 1)); + EXPECT_EQUAL(config->application_protocols.size, 0); + + EXPECT_SUCCESS(s2n_config_free(config)); + } + + /* Test connection set */ + { + struct s2n_connection *conn; + EXPECT_NOT_NULL(conn = s2n_connection_new(S2N_CLIENT)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); + + /* should copy the preference list */ + EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, protocols_count)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, (1 /* len prefix */ + 9) * protocols_count); + + /* should clear the preference list */ + EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, NULL, protocols_count)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); + + EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, protocols_count)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, (1 /* len prefix */ + 9) * protocols_count); + EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, 0)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); + + /* should limit the length of the protocol value */ + char oversized[257] = { 0 }; + memset(&oversized, 1, 256); + EXPECT_EQUAL(strlen(oversized), 256); + const char *oversized_p[] = { (const char *)&oversized }; + EXPECT_FAILURE(s2n_connection_set_protocol_preferences(conn, (const char * const *)oversized_p, 1)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); + + EXPECT_SUCCESS(s2n_connection_free(conn)); + } + + END_TEST(); + return 0; +} diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index e0157f4ae4e..f5cb1739787 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -17,39 +17,76 @@ #include "error/s2n_errno.h" #include "utils/s2n_safety.h" -int s2n_blob_set_protocol_preferences(struct s2n_blob *application_protocols, const char *const *protocols, int protocol_count) +S2N_RESULT s2n_protocol_preferences_write(struct s2n_stuffer *protocol_stuffer, const uint8_t *protocol, size_t protocol_len) { - struct s2n_stuffer protocol_stuffer = {0}; + ENSURE(protocol_len <= 255, S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG); + uint32_t extension_len = s2n_stuffer_data_available(protocol_stuffer) + /* len prefix */ 1 + protocol_len; + ENSURE(extension_len <= UINT16_MAX, S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG); + + GUARD_AS_RESULT(s2n_stuffer_write_uint8(protocol_stuffer, protocol_len)); + GUARD_AS_RESULT(s2n_stuffer_write_bytes(protocol_stuffer, protocol, protocol_len)); + + return S2N_RESULT_OK; +} + +S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, const char *const *protocols, int protocol_count) +{ + ENSURE_REF(application_protocols); - GUARD(s2n_free(application_protocols)); + DEFER_CLEANUP(struct s2n_stuffer protocol_stuffer = { 0 }, s2n_stuffer_free); + + GUARD_AS_RESULT(s2n_free(application_protocols)); if (protocols == NULL || protocol_count == 0) { /* NULL value indicates no preference, so nothing to do */ - return 0; + return S2N_RESULT_OK; } - GUARD(s2n_stuffer_growable_alloc(&protocol_stuffer, 256)); - for (int i = 0; i < protocol_count; i++) { + GUARD_AS_RESULT(s2n_stuffer_growable_alloc(&protocol_stuffer, 256)); + for (size_t i = 0; i < protocol_count; i++) { + const uint8_t * protocol = (const uint8_t *)protocols[i]; size_t length = strlen(protocols[i]); - uint8_t protocol[255]; - - S2N_ERROR_IF(length > 255 || (s2n_stuffer_data_available(&protocol_stuffer) + length + 1) > 65535, S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG); - memcpy_check(protocol, protocols[i], length); - GUARD(s2n_stuffer_write_uint8(&protocol_stuffer, length)); - GUARD(s2n_stuffer_write_bytes(&protocol_stuffer, protocol, length)); + GUARD_RESULT(s2n_protocol_preferences_write(&protocol_stuffer, protocol, length)); } - GUARD(s2n_stuffer_extract_blob(&protocol_stuffer, application_protocols)); - GUARD(s2n_stuffer_free(&protocol_stuffer)); - return 0; + GUARD_AS_RESULT(s2n_stuffer_extract_blob(&protocol_stuffer, application_protocols)); + + return S2N_RESULT_OK; +} + +S2N_RESULT s2n_protocol_preferences_append(struct s2n_blob *application_protocols, const uint8_t *protocol, size_t protocol_len) +{ + ENSURE_REF(application_protocols); + ENSURE_REF(protocol); + + struct s2n_stuffer protocol_stuffer = {0}; + GUARD_AS_RESULT(s2n_stuffer_init(&protocol_stuffer, application_protocols)); + protocol_stuffer.growable = true; + GUARD_AS_RESULT(s2n_stuffer_skip_write(&protocol_stuffer, application_protocols->size)); + + GUARD_RESULT(s2n_protocol_preferences_write(&protocol_stuffer, protocol, protocol_len)); + + GUARD_AS_RESULT(s2n_stuffer_extract_blob(&protocol_stuffer, application_protocols)); + + return S2N_RESULT_OK; } int s2n_config_set_protocol_preferences(struct s2n_config *config, const char *const *protocols, int protocol_count) { - return s2n_blob_set_protocol_preferences(&config->application_protocols, protocols, protocol_count); + return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_set(&config->application_protocols, protocols, protocol_count)); +} + +int s2n_config_append_protocol_preference(struct s2n_config *config, const uint8_t *protocol, size_t protocol_len) +{ + return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_append(&config->application_protocols, protocol, protocol_len)); } int s2n_connection_set_protocol_preferences(struct s2n_connection *conn, const char * const *protocols, int protocol_count) { - return s2n_blob_set_protocol_preferences(&conn->application_protocols_overridden, protocols, protocol_count); + return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_set(&conn->application_protocols_overridden, protocols, protocol_count)); +} + +int s2n_connection_append_protocol_preference(struct s2n_connection *conn, const uint8_t *protocol, size_t protocol_len) +{ + return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_append(&conn->application_protocols_overridden, protocol, protocol_len)); } From b3e3517d496fec6a57beabda9dda80cfde9cbbab Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Mon, 25 Jan 2021 13:45:52 -0800 Subject: [PATCH 2/9] add validation for min length --- error/s2n_errno.c | 2 +- error/s2n_errno.h | 2 +- tests/unit/s2n_protocol_preferences_test.c | 14 ++++++++++++-- tls/s2n_protocol_preferences.c | 11 +++++++++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/error/s2n_errno.c b/error/s2n_errno.c index 11ac017428c..37893c2f68a 100755 --- a/error/s2n_errno.c +++ b/error/s2n_errno.c @@ -193,7 +193,7 @@ static const char *no_such_error = "Internal s2n error"; ERR_ENTRY(S2N_ERR_NUM_DEFAULT_CERTIFICATES, "exceeded max default certificates or provided no default") \ ERR_ENTRY(S2N_ERR_MULTIPLE_DEFAULT_CERTIFICATES_PER_AUTH_TYPE, "setting multiple default certificates per auth type is not allowed") \ ERR_ENTRY(S2N_ERR_INVALID_CIPHER_PREFERENCES, "Invalid Cipher Preferences version") \ - ERR_ENTRY(S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG, "Application protocol name is too long") \ + ERR_ENTRY(S2N_ERR_INVALID_APPLICATION_PROTOCOL, "The supplied application protocol name is invalid") \ ERR_ENTRY(S2N_ERR_KEY_MISMATCH, "public and private key do not match") \ ERR_ENTRY(S2N_ERR_SEND_SIZE, "Retried s2n_send() size is invalid") \ ERR_ENTRY(S2N_ERR_CORK_SET_ON_UNMANAGED, "Attempt to set connection cork management on unmanaged IO") \ diff --git a/error/s2n_errno.h b/error/s2n_errno.h index dfb580fa499..83340d40c1b 100755 --- a/error/s2n_errno.h +++ b/error/s2n_errno.h @@ -221,7 +221,7 @@ typedef enum { S2N_ERR_NUM_DEFAULT_CERTIFICATES, S2N_ERR_MULTIPLE_DEFAULT_CERTIFICATES_PER_AUTH_TYPE, S2N_ERR_INVALID_CIPHER_PREFERENCES, - S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG, + S2N_ERR_INVALID_APPLICATION_PROTOCOL, S2N_ERR_KEY_MISMATCH, S2N_ERR_SEND_SIZE, S2N_ERR_CORK_SET_ON_UNMANAGED, diff --git a/tests/unit/s2n_protocol_preferences_test.c b/tests/unit/s2n_protocol_preferences_test.c index 75235be9ede..98f6184e27e 100644 --- a/tests/unit/s2n_protocol_preferences_test.c +++ b/tests/unit/s2n_protocol_preferences_test.c @@ -51,8 +51,13 @@ int main(int argc, char **argv) EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 255); prev_size = config->application_protocols.size; - /* should limit the length of the protocol value */ const uint8_t oversized[256] = { 0 }; + + /* should not allow empty protocol values */ + EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)oversized, 0)); + EXPECT_EQUAL(config->application_protocols.size, prev_size); + + /* should limit the length of the protocol value */ EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)oversized, 265)); EXPECT_EQUAL(config->application_protocols.size, prev_size); @@ -88,8 +93,13 @@ int main(int argc, char **argv) EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 255); prev_size = conn->application_protocols_overridden.size; - /* should limit the length of the protocol value */ const uint8_t oversized[256] = { 0 }; + + /* should not allow empty protocol values */ + EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)oversized, 0)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size); + + /* should limit the length of the protocol value */ EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)oversized, 265)); EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size); diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index f5cb1739787..dd4c4ddb2ae 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -19,9 +19,16 @@ S2N_RESULT s2n_protocol_preferences_write(struct s2n_stuffer *protocol_stuffer, const uint8_t *protocol, size_t protocol_len) { - ENSURE(protocol_len <= 255, S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG); + /** + *= https://tools.ietf.org/rfc/rfc7301#section-3.1 + *# Empty strings + *# MUST NOT be included and byte strings MUST NOT be truncated. + */ + ENSURE(protocol_len != 0, S2N_ERR_INVALID_APPLICATION_PROTOCOL); + ENSURE(protocol_len <= 255, S2N_ERR_INVALID_APPLICATION_PROTOCOL); + uint32_t extension_len = s2n_stuffer_data_available(protocol_stuffer) + /* len prefix */ 1 + protocol_len; - ENSURE(extension_len <= UINT16_MAX, S2N_ERR_APPLICATION_PROTOCOL_TOO_LONG); + ENSURE(extension_len <= UINT16_MAX, S2N_ERR_INVALID_APPLICATION_PROTOCOL); GUARD_AS_RESULT(s2n_stuffer_write_uint8(protocol_stuffer, protocol_len)); GUARD_AS_RESULT(s2n_stuffer_write_bytes(protocol_stuffer, protocol, protocol_len)); From 3a462cf125d80dc89a032f9954b7193aa28ae879 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Mon, 25 Jan 2021 15:51:51 -0800 Subject: [PATCH 3/9] fix blob leak --- tls/s2n_protocol_preferences.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index dd4c4ddb2ae..44f46ff4eb0 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -19,6 +19,9 @@ S2N_RESULT s2n_protocol_preferences_write(struct s2n_stuffer *protocol_stuffer, const uint8_t *protocol, size_t protocol_len) { + ENSURE_MUT(protocol_stuffer); + ENSURE_REF(protocol); + /** *= https://tools.ietf.org/rfc/rfc7301#section-3.1 *# Empty strings @@ -38,17 +41,17 @@ S2N_RESULT s2n_protocol_preferences_write(struct s2n_stuffer *protocol_stuffer, S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, const char *const *protocols, int protocol_count) { - ENSURE_REF(application_protocols); - - DEFER_CLEANUP(struct s2n_stuffer protocol_stuffer = { 0 }, s2n_stuffer_free); - - GUARD_AS_RESULT(s2n_free(application_protocols)); + ENSURE_MUT(application_protocols); + /* NULL value indicates no preference so free the previous blob */ if (protocols == NULL || protocol_count == 0) { - /* NULL value indicates no preference, so nothing to do */ + GUARD_AS_RESULT(s2n_free(application_protocols)); + return S2N_RESULT_OK; } + DEFER_CLEANUP(struct s2n_stuffer protocol_stuffer = { 0 }, s2n_stuffer_free); + GUARD_AS_RESULT(s2n_stuffer_growable_alloc(&protocol_stuffer, 256)); for (size_t i = 0; i < protocol_count; i++) { const uint8_t * protocol = (const uint8_t *)protocols[i]; @@ -63,17 +66,20 @@ S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, S2N_RESULT s2n_protocol_preferences_append(struct s2n_blob *application_protocols, const uint8_t *protocol, size_t protocol_len) { - ENSURE_REF(application_protocols); + ENSURE_MUT(application_protocols); ENSURE_REF(protocol); struct s2n_stuffer protocol_stuffer = {0}; GUARD_AS_RESULT(s2n_stuffer_init(&protocol_stuffer, application_protocols)); + protocol_stuffer.alloced = true; protocol_stuffer.growable = true; GUARD_AS_RESULT(s2n_stuffer_skip_write(&protocol_stuffer, application_protocols->size)); GUARD_RESULT(s2n_protocol_preferences_write(&protocol_stuffer, protocol, protocol_len)); - GUARD_AS_RESULT(s2n_stuffer_extract_blob(&protocol_stuffer, application_protocols)); + // ensure the application_protocols gets updated if we reallocate + *application_protocols = protocol_stuffer.blob; + application_protocols->size = protocol_stuffer.write_cursor; return S2N_RESULT_OK; } From 51abf3ad2433727804c0c0087bf673b51fd8b3c4 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Mon, 25 Jan 2021 15:54:03 -0800 Subject: [PATCH 4/9] helps to use the correct comment style --- tls/s2n_protocol_preferences.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index 44f46ff4eb0..85e5d998388 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -77,7 +77,7 @@ S2N_RESULT s2n_protocol_preferences_append(struct s2n_blob *application_protocol GUARD_RESULT(s2n_protocol_preferences_write(&protocol_stuffer, protocol, protocol_len)); - // ensure the application_protocols gets updated if we reallocate + /* ensure the application_protocols gets updated if we reallocate */ *application_protocols = protocol_stuffer.blob; application_protocols->size = protocol_stuffer.write_cursor; From fa8ddea269c62842e2a15fa046ff3ad40211ca10 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Fri, 29 Jan 2021 14:53:28 -0800 Subject: [PATCH 5/9] use s2n_realloc directly --- api/s2n.h | 10 ++-- tests/unit/s2n_protocol_preferences_test.c | 24 ++++----- tls/s2n_connection.c | 2 + tls/s2n_protocol_preferences.c | 60 ++++++++++------------ 4 files changed, 45 insertions(+), 51 deletions(-) diff --git a/api/s2n.h b/api/s2n.h index a2ebe088eb2..2064973ee77 100644 --- a/api/s2n.h +++ b/api/s2n.h @@ -226,11 +226,11 @@ extern int s2n_config_set_cipher_preferences(struct s2n_config *config, const ch * The data provided in `protocol` parameter will be copied into an internal buffer * * @param config The configuration object being updated - * @param protocol A pointer to a slice of bytes - * @param protocol_len The length of bytes that should be read from `protocol`. Note this value cannot exceed `255`. + * @param protocol A pointer to a byte array value + * @param protocol_len The length of bytes that should be read from `protocol`. Note: this value cannot be 0, otherwise an error will be returned. */ S2N_API -extern int s2n_config_append_protocol_preference(struct s2n_config *config, const uint8_t *protocol, size_t protocol_len); +extern int s2n_config_append_protocol_preference(struct s2n_config *config, const uint8_t *protocol, uint8_t protocol_len); S2N_API extern int s2n_config_set_protocol_preferences(struct s2n_config *config, const char * const *protocols, int protocol_count); @@ -349,10 +349,10 @@ extern int s2n_connection_set_cipher_preferences(struct s2n_connection *conn, co * * @param conn The connection object being updated * @param protocol A pointer to a slice of bytes - * @param protocol_len The length of bytes that should be read from `protocol`. Note this value cannot exceed `255`. + * @param protocol_len The length of bytes that should be read from `protocol`. Note: this value cannot be 0, otherwise an error will be returned. */ S2N_API -extern int s2n_connection_append_protocol_preference(struct s2n_connection *conn, const uint8_t *protocol, size_t protocol_len); +extern int s2n_connection_append_protocol_preference(struct s2n_connection *conn, const uint8_t *protocol, uint8_t protocol_len); S2N_API extern int s2n_connection_set_protocol_preferences(struct s2n_connection *conn, const char * const *protocols, int protocol_count); diff --git a/tests/unit/s2n_protocol_preferences_test.c b/tests/unit/s2n_protocol_preferences_test.c index 98f6184e27e..761a6384806 100644 --- a/tests/unit/s2n_protocol_preferences_test.c +++ b/tests/unit/s2n_protocol_preferences_test.c @@ -51,14 +51,8 @@ int main(int argc, char **argv) EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 255); prev_size = config->application_protocols.size; - const uint8_t oversized[256] = { 0 }; - /* should not allow empty protocol values */ - EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)oversized, 0)); - EXPECT_EQUAL(config->application_protocols.size, prev_size); - - /* should limit the length of the protocol value */ - EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)oversized, 265)); + EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)large_value, 0)); EXPECT_EQUAL(config->application_protocols.size, prev_size); EXPECT_SUCCESS(s2n_config_free(config)); @@ -93,14 +87,8 @@ int main(int argc, char **argv) EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 255); prev_size = conn->application_protocols_overridden.size; - const uint8_t oversized[256] = { 0 }; - /* should not allow empty protocol values */ - EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)oversized, 0)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size); - - /* should limit the length of the protocol value */ - EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)oversized, 265)); + EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)large_value, 0)); EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size); EXPECT_SUCCESS(s2n_connection_free(conn)); @@ -119,6 +107,10 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, protocols_count)); EXPECT_EQUAL(config->application_protocols.size, (1 /* len prefix */ + 9) * protocols_count); + /* should correctly free the old list list */ + EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, 1)); + EXPECT_EQUAL(config->application_protocols.size, 1 /* len prefix */ + 9); + /* should clear the preference list */ EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, NULL, protocols_count)); EXPECT_EQUAL(config->application_protocols.size, 0); @@ -149,6 +141,10 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, protocols_count)); EXPECT_EQUAL(conn->application_protocols_overridden.size, (1 /* len prefix */ + 9) * protocols_count); + /* should correctly free the old list */ + EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, 1)); + EXPECT_EQUAL(conn->application_protocols_overridden.size, 1 /* len prefix */ + 9); + /* should clear the preference list */ EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, NULL, protocols_count)); EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index f2393fd2a1c..c3d97094c2c 100755 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -231,6 +231,8 @@ struct s2n_connection *s2n_connection_new(s2n_mode mode) * is received, the stuffer will be resized according to the cookie length */ GUARD_PTR(s2n_stuffer_growable_alloc(&conn->cookie_stuffer, 0)); + conn->application_protocols_overridden = (struct s2n_blob){ 0 }; + return conn; } diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index 85e5d998388..2dcba3ee7a7 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -17,9 +17,9 @@ #include "error/s2n_errno.h" #include "utils/s2n_safety.h" -S2N_RESULT s2n_protocol_preferences_write(struct s2n_stuffer *protocol_stuffer, const uint8_t *protocol, size_t protocol_len) +S2N_RESULT s2n_protocol_preferences_append(struct s2n_blob *application_protocols, const uint8_t *protocol, uint8_t protocol_len) { - ENSURE_MUT(protocol_stuffer); + ENSURE_MUT(application_protocols); ENSURE_REF(protocol); /** @@ -28,13 +28,18 @@ S2N_RESULT s2n_protocol_preferences_write(struct s2n_stuffer *protocol_stuffer, *# MUST NOT be included and byte strings MUST NOT be truncated. */ ENSURE(protocol_len != 0, S2N_ERR_INVALID_APPLICATION_PROTOCOL); - ENSURE(protocol_len <= 255, S2N_ERR_INVALID_APPLICATION_PROTOCOL); - uint32_t extension_len = s2n_stuffer_data_available(protocol_stuffer) + /* len prefix */ 1 + protocol_len; - ENSURE(extension_len <= UINT16_MAX, S2N_ERR_INVALID_APPLICATION_PROTOCOL); + uint32_t prev_len = application_protocols->size; + uint32_t new_len = prev_len + /* len prefix */ 1 + protocol_len; + ENSURE(new_len <= UINT16_MAX, S2N_ERR_INVALID_APPLICATION_PROTOCOL); + + GUARD_AS_RESULT(s2n_realloc(application_protocols, new_len)); - GUARD_AS_RESULT(s2n_stuffer_write_uint8(protocol_stuffer, protocol_len)); - GUARD_AS_RESULT(s2n_stuffer_write_bytes(protocol_stuffer, protocol, protocol_len)); + struct s2n_stuffer protocol_stuffer = {0}; + GUARD_AS_RESULT(s2n_stuffer_init(&protocol_stuffer, application_protocols)); + GUARD_AS_RESULT(s2n_stuffer_skip_write(&protocol_stuffer, prev_len)); + GUARD_AS_RESULT(s2n_stuffer_write_uint8(&protocol_stuffer, protocol_len)); + GUARD_AS_RESULT(s2n_stuffer_write_bytes(&protocol_stuffer, protocol, protocol_len)); return S2N_RESULT_OK; } @@ -46,40 +51,31 @@ S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, /* NULL value indicates no preference so free the previous blob */ if (protocols == NULL || protocol_count == 0) { GUARD_AS_RESULT(s2n_free(application_protocols)); - return S2N_RESULT_OK; } - DEFER_CLEANUP(struct s2n_stuffer protocol_stuffer = { 0 }, s2n_stuffer_free); + DEFER_CLEANUP(struct s2n_blob new_protocols = { 0 }, s2n_free); - GUARD_AS_RESULT(s2n_stuffer_growable_alloc(&protocol_stuffer, 256)); for (size_t i = 0; i < protocol_count; i++) { const uint8_t * protocol = (const uint8_t *)protocols[i]; size_t length = strlen(protocols[i]); - GUARD_RESULT(s2n_protocol_preferences_write(&protocol_stuffer, protocol, length)); - } - - GUARD_AS_RESULT(s2n_stuffer_extract_blob(&protocol_stuffer, application_protocols)); - - return S2N_RESULT_OK; -} -S2N_RESULT s2n_protocol_preferences_append(struct s2n_blob *application_protocols, const uint8_t *protocol, size_t protocol_len) -{ - ENSURE_MUT(application_protocols); - ENSURE_REF(protocol); + /** + *= https://tools.ietf.org/rfc/rfc7301#section-3.1 + *# Empty strings + *# MUST NOT be included and byte strings MUST NOT be truncated. + */ + ENSURE(length < 256, S2N_ERR_INVALID_APPLICATION_PROTOCOL); - struct s2n_stuffer protocol_stuffer = {0}; - GUARD_AS_RESULT(s2n_stuffer_init(&protocol_stuffer, application_protocols)); - protocol_stuffer.alloced = true; - protocol_stuffer.growable = true; - GUARD_AS_RESULT(s2n_stuffer_skip_write(&protocol_stuffer, application_protocols->size)); + GUARD_RESULT(s2n_protocol_preferences_append(&new_protocols, protocol, (uint8_t)length)); + } - GUARD_RESULT(s2n_protocol_preferences_write(&protocol_stuffer, protocol, protocol_len)); + /* now we can free the previous list since we've validated all new input */ + GUARD_AS_RESULT(s2n_free(application_protocols)); - /* ensure the application_protocols gets updated if we reallocate */ - *application_protocols = protocol_stuffer.blob; - application_protocols->size = protocol_stuffer.write_cursor; + *application_protocols = new_protocols; + /* zero out new_protocols so we don't free what we just allocated */ + new_protocols = (struct s2n_blob){ 0 }; return S2N_RESULT_OK; } @@ -89,7 +85,7 @@ int s2n_config_set_protocol_preferences(struct s2n_config *config, const char *c return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_set(&config->application_protocols, protocols, protocol_count)); } -int s2n_config_append_protocol_preference(struct s2n_config *config, const uint8_t *protocol, size_t protocol_len) +int s2n_config_append_protocol_preference(struct s2n_config *config, const uint8_t *protocol, uint8_t protocol_len) { return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_append(&config->application_protocols, protocol, protocol_len)); } @@ -99,7 +95,7 @@ int s2n_connection_set_protocol_preferences(struct s2n_connection *conn, const c return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_set(&conn->application_protocols_overridden, protocols, protocol_count)); } -int s2n_connection_append_protocol_preference(struct s2n_connection *conn, const uint8_t *protocol, size_t protocol_len) +int s2n_connection_append_protocol_preference(struct s2n_connection *conn, const uint8_t *protocol, uint8_t protocol_len) { return S2N_RESULT_TO_POSIX(s2n_protocol_preferences_append(&conn->application_protocols_overridden, protocol, protocol_len)); } From a93e651f2bd7f99c9d5cc7cbf8ebc20c528e1e0d Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Sun, 31 Jan 2021 10:07:06 -0800 Subject: [PATCH 6/9] make cppcheck happy --- tls/s2n_protocol_preferences.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index 2dcba3ee7a7..40744c5652c 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -73,9 +73,12 @@ S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, /* now we can free the previous list since we've validated all new input */ GUARD_AS_RESULT(s2n_free(application_protocols)); + /* update the connection/config application_protocols with the newly allocated blob */ *application_protocols = new_protocols; - /* zero out new_protocols so we don't free what we just allocated */ + + /* zero out new_protocols so we no longer refer to what we just allocated */ new_protocols = (struct s2n_blob){ 0 }; + GUARD_AS_RESULT(s2n_free(&new_protocols)); return S2N_RESULT_OK; } From cfbac965675e9baf746fa7bcd2411f3f3ed98893 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Mon, 1 Feb 2021 11:45:51 -0800 Subject: [PATCH 7/9] remove magic numbers from tests --- tests/unit/s2n_protocol_preferences_test.c | 89 ++++++++++------------ 1 file changed, 42 insertions(+), 47 deletions(-) diff --git a/tests/unit/s2n_protocol_preferences_test.c b/tests/unit/s2n_protocol_preferences_test.c index 761a6384806..f0f7525c0bb 100644 --- a/tests/unit/s2n_protocol_preferences_test.c +++ b/tests/unit/s2n_protocol_preferences_test.c @@ -18,10 +18,20 @@ #include +#define LEN_PREFIX 1 + int main(int argc, char **argv) { BEGIN_TEST(); + /* input values */ + const uint8_t protocol1[] = "protocol1"; + const size_t protocol1_len = strlen((const char *)protocol1); + const uint8_t protocol2[] = "protocol abc 2"; + const size_t protocol2_len = strlen((const char *)protocol2); + + const uint8_t large_value[255] = { 0 }; + /* Test config append */ { struct s2n_config *config; @@ -30,29 +40,22 @@ int main(int argc, char **argv) size_t prev_size = 0; /* should grow the blob with the provided value */ - EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)"protocol1", 9)); - EXPECT_EQUAL(config->application_protocols.size, 1 /* len prefix */ + 9); + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, protocol1, sizeof(protocol1))); + EXPECT_EQUAL(config->application_protocols.size, LEN_PREFIX + sizeof(protocol1)); prev_size = config->application_protocols.size; /* should grow the blob even more */ - EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)"protocol2", 9)); - EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 9); - prev_size = config->application_protocols.size; - - /* should allow null byte values */ - const uint8_t null_value[9] = { 0 }; - EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)null_value, 9)); - EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 9); + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, protocol2, sizeof(protocol2))); + EXPECT_EQUAL(config->application_protocols.size, prev_size + LEN_PREFIX + sizeof(protocol2)); prev_size = config->application_protocols.size; - /* should reallocate the blob */ - const uint8_t large_value[255] = { 0 }; - EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, (const uint8_t *)large_value, 255)); - EXPECT_EQUAL(config->application_protocols.size, prev_size + 1 /* len prefix */ + 255); + /* should reallocate the blob with large values */ + EXPECT_SUCCESS(s2n_config_append_protocol_preference(config, large_value, sizeof(large_value))); + EXPECT_EQUAL(config->application_protocols.size, prev_size + LEN_PREFIX + sizeof(large_value)); prev_size = config->application_protocols.size; /* should not allow empty protocol values */ - EXPECT_FAILURE(s2n_config_append_protocol_preference(config, (const uint8_t *)large_value, 0)); + EXPECT_FAILURE(s2n_config_append_protocol_preference(config, large_value, 0)); EXPECT_EQUAL(config->application_protocols.size, prev_size); EXPECT_SUCCESS(s2n_config_free(config)); @@ -66,37 +69,36 @@ int main(int argc, char **argv) size_t prev_size = 0; /* should grow the blob with the provided value */ - EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)"protocol1", 9)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, 1 /* len prefix */ + 9); + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, protocol1, sizeof(protocol1))); + EXPECT_EQUAL(conn->application_protocols_overridden.size, LEN_PREFIX + sizeof(protocol1)); prev_size = conn->application_protocols_overridden.size; /* should grow the blob even more */ - EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)"protocol2", 9)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 9); + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, protocol2, sizeof(protocol2))); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + LEN_PREFIX + sizeof(protocol2)); prev_size = conn->application_protocols_overridden.size; - /* should allow null byte values */ - const uint8_t null_value[9] = { 0 }; - EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)null_value, 9)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 9); - prev_size = conn->application_protocols_overridden.size; - - /* should reallocate the blob */ - const uint8_t large_value[255] = { 0 }; - EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, (const uint8_t *)large_value, 255)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + 1 /* len prefix */ + 255); + /* should reallocate the blob with large values */ + EXPECT_SUCCESS(s2n_connection_append_protocol_preference(conn, large_value, sizeof(large_value))); + EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size + LEN_PREFIX + sizeof(large_value)); prev_size = conn->application_protocols_overridden.size; /* should not allow empty protocol values */ - EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, (const uint8_t *)large_value, 0)); + EXPECT_FAILURE(s2n_connection_append_protocol_preference(conn, large_value, 0)); EXPECT_EQUAL(conn->application_protocols_overridden.size, prev_size); EXPECT_SUCCESS(s2n_connection_free(conn)); } - const char *protocols[] = { "protocol1", "protocol2", "protocol3" }; + const char *protocols[] = { (const char *)protocol1, (const char *)protocol2 }; const uint8_t protocols_count = s2n_array_len(protocols); + char oversized_value[257] = { 0 }; + memset(&oversized_value, 1, 256); + EXPECT_EQUAL(strlen(oversized_value), 256); + const char *oversized[] = { oversized_value }; + const uint8_t oversized_count = s2n_array_len(oversized); + /* Test config set */ { struct s2n_config *config; @@ -105,27 +107,23 @@ int main(int argc, char **argv) /* should copy the preference list */ EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, protocols_count)); - EXPECT_EQUAL(config->application_protocols.size, (1 /* len prefix */ + 9) * protocols_count); + EXPECT_EQUAL(config->application_protocols.size, LEN_PREFIX + protocol1_len + LEN_PREFIX + protocol2_len); /* should correctly free the old list list */ EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, 1)); - EXPECT_EQUAL(config->application_protocols.size, 1 /* len prefix */ + 9); + EXPECT_EQUAL(config->application_protocols.size, LEN_PREFIX + protocol1_len); /* should clear the preference list */ EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, NULL, protocols_count)); EXPECT_EQUAL(config->application_protocols.size, 0); EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, protocols_count)); - EXPECT_EQUAL(config->application_protocols.size, (1 /* len prefix */ + 9) * protocols_count); + EXPECT_EQUAL(config->application_protocols.size, LEN_PREFIX + protocol1_len + LEN_PREFIX + protocol2_len); EXPECT_SUCCESS(s2n_config_set_protocol_preferences(config, protocols, 0)); EXPECT_EQUAL(config->application_protocols.size, 0); /* should limit the length of the protocol value */ - char oversized[257] = { 0 }; - memset(&oversized, 1, 256); - EXPECT_EQUAL(strlen(oversized), 256); - const char *oversized_p[] = { (const char *)&oversized }; - EXPECT_FAILURE(s2n_config_set_protocol_preferences(config, (const char * const *)oversized_p, 1)); + EXPECT_FAILURE(s2n_config_set_protocol_preferences(config, oversized, oversized_count)); EXPECT_EQUAL(config->application_protocols.size, 0); EXPECT_SUCCESS(s2n_config_free(config)); @@ -139,27 +137,24 @@ int main(int argc, char **argv) /* should copy the preference list */ EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, protocols_count)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, (1 /* len prefix */ + 9) * protocols_count); + EXPECT_EQUAL(conn->application_protocols_overridden.size, LEN_PREFIX + protocol1_len + LEN_PREFIX + protocol2_len); /* should correctly free the old list */ EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, 1)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, 1 /* len prefix */ + 9); + EXPECT_EQUAL(conn->application_protocols_overridden.size, LEN_PREFIX + protocol1_len); /* should clear the preference list */ EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, NULL, protocols_count)); EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, protocols_count)); - EXPECT_EQUAL(conn->application_protocols_overridden.size, (1 /* len prefix */ + 9) * protocols_count); + EXPECT_EQUAL(conn->application_protocols_overridden.size, LEN_PREFIX + protocol1_len + LEN_PREFIX + protocol2_len); EXPECT_SUCCESS(s2n_connection_set_protocol_preferences(conn, protocols, 0)); EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); /* should limit the length of the protocol value */ - char oversized[257] = { 0 }; - memset(&oversized, 1, 256); - EXPECT_EQUAL(strlen(oversized), 256); - const char *oversized_p[] = { (const char *)&oversized }; - EXPECT_FAILURE(s2n_connection_set_protocol_preferences(conn, (const char * const *)oversized_p, 1)); + + EXPECT_FAILURE(s2n_connection_set_protocol_preferences(conn, oversized, oversized_count)); EXPECT_EQUAL(conn->application_protocols_overridden.size, 0); EXPECT_SUCCESS(s2n_connection_free(conn)); From aac00a724e4a7b548e055b3e00d051e0d0d9e341 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Tue, 2 Feb 2021 09:29:49 -0800 Subject: [PATCH 8/9] reduce the number of reallocations --- tls/s2n_connection.c | 2 -- tls/s2n_protocol_preferences.c | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tls/s2n_connection.c b/tls/s2n_connection.c index c3d97094c2c..f2393fd2a1c 100755 --- a/tls/s2n_connection.c +++ b/tls/s2n_connection.c @@ -231,8 +231,6 @@ struct s2n_connection *s2n_connection_new(s2n_mode mode) * is received, the stuffer will be resized according to the cookie length */ GUARD_PTR(s2n_stuffer_growable_alloc(&conn->cookie_stuffer, 0)); - conn->application_protocols_overridden = (struct s2n_blob){ 0 }; - return conn; } diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index 40744c5652c..a51bc55333d 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -56,6 +56,19 @@ S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, DEFER_CLEANUP(struct s2n_blob new_protocols = { 0 }, s2n_free); + /* Allocate enough space to avoid a reallocation for every entry + * + * We assume that each protocol is most likely 8 bytes or less. + * If it ends up being larger, we will expand the blob automatically + * in the append method. + */ + GUARD_AS_RESULT(s2n_realloc(&new_protocols, protocol_count * 8)); + + /* set the size back to 0 so we start at the beginning. + * s2n_realloc will just update the size field here + */ + GUARD_AS_RESULT(s2n_realloc(&new_protocols, 0)); + for (size_t i = 0; i < protocol_count; i++) { const uint8_t * protocol = (const uint8_t *)protocols[i]; size_t length = strlen(protocols[i]); @@ -78,6 +91,12 @@ S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, /* zero out new_protocols so we no longer refer to what we just allocated */ new_protocols = (struct s2n_blob){ 0 }; + + /* even though we've used DEFER_CLEANUP above, static analysis tools + * like cppcheck don't understand that the new assignment will be consumed + * at the end of the function. Here we free it even though DEFER_CLEANUP will + * also free it. This is OK since s2n_free will be a no-op on empty blobs. + */ GUARD_AS_RESULT(s2n_free(&new_protocols)); return S2N_RESULT_OK; From 9c160205d96d53437c946268bba8a847011c2425 Mon Sep 17 00:00:00 2001 From: Cameron Bytheway Date: Tue, 2 Feb 2021 10:42:24 -0800 Subject: [PATCH 9/9] use cppcheck suppression for unreadVariable --- tls/s2n_protocol_preferences.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tls/s2n_protocol_preferences.c b/tls/s2n_protocol_preferences.c index a51bc55333d..72ba8c659f1 100644 --- a/tls/s2n_protocol_preferences.c +++ b/tls/s2n_protocol_preferences.c @@ -35,7 +35,7 @@ S2N_RESULT s2n_protocol_preferences_append(struct s2n_blob *application_protocol GUARD_AS_RESULT(s2n_realloc(application_protocols, new_len)); - struct s2n_stuffer protocol_stuffer = {0}; + struct s2n_stuffer protocol_stuffer = { 0 }; GUARD_AS_RESULT(s2n_stuffer_init(&protocol_stuffer, application_protocols)); GUARD_AS_RESULT(s2n_stuffer_skip_write(&protocol_stuffer, prev_len)); GUARD_AS_RESULT(s2n_stuffer_write_uint8(&protocol_stuffer, protocol_len)); @@ -89,15 +89,11 @@ S2N_RESULT s2n_protocol_preferences_set(struct s2n_blob *application_protocols, /* update the connection/config application_protocols with the newly allocated blob */ *application_protocols = new_protocols; - /* zero out new_protocols so we no longer refer to what we just allocated */ - new_protocols = (struct s2n_blob){ 0 }; - - /* even though we've used DEFER_CLEANUP above, static analysis tools - * like cppcheck don't understand that the new assignment will be consumed - * at the end of the function. Here we free it even though DEFER_CLEANUP will - * also free it. This is OK since s2n_free will be a no-op on empty blobs. + /* zero out new_protocols so the DEFER_CLEANUP from above doesn't free + * the blob that we created and assigned to application_protocols */ - GUARD_AS_RESULT(s2n_free(&new_protocols)); + /* cppcheck-suppress unreadVariable */ + new_protocols = (struct s2n_blob){ 0 }; return S2N_RESULT_OK; }