From 7e403da823410be1b7f5d91559bc309f9e3e08d3 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Sun, 19 Nov 2023 16:46:35 -0800 Subject: [PATCH 1/5] Add basic "security rules" --- stuffer/s2n_stuffer.h | 14 + stuffer/s2n_stuffer_text.c | 78 +++++ tests/unit/s2n_security_rules_test.c | 409 +++++++++++++++++++++++++++ tests/unit/s2n_stuffer_text_test.c | 153 ++++++++++ tls/s2n_security_policies.c | 13 + tls/s2n_security_policies.h | 3 + tls/s2n_security_rules.c | 229 +++++++++++++++ tls/s2n_security_rules.h | 53 ++++ 8 files changed, 952 insertions(+) create mode 100644 tests/unit/s2n_security_rules_test.c create mode 100644 tls/s2n_security_rules.c create mode 100644 tls/s2n_security_rules.h diff --git a/stuffer/s2n_stuffer.h b/stuffer/s2n_stuffer.h index ee9eda12873..51f3eff47d7 100644 --- a/stuffer/s2n_stuffer.h +++ b/stuffer/s2n_stuffer.h @@ -16,6 +16,7 @@ #pragma once #include +#include #include #include #include @@ -165,6 +166,19 @@ int s2n_stuffer_skip_read_until(struct s2n_stuffer *stuffer, const char *target) int s2n_stuffer_alloc_ro_from_string(struct s2n_stuffer *stuffer, const char *str); int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data, uint32_t length); +/* Stuffer versions of sprintf methods, except: + * - They write bytes, not strings. They do not write a final '\0'. Unfortunately, + * they do still require enough space for a final '\0'-- we'd have to reimplement + * sprintf to avoid that. + * - vprintf/vnprintf do not consume their vargs. They call va_copy before using + * the varg argument, so they can be called repeatedly with the same vargs. + * - nprintf/vnprintf do not return the actual size of the formatted string. + */ +int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...); +int s2n_stuffer_nprintf(struct s2n_stuffer *stuffer, uint32_t max, const char *format, ...); +int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs); +int s2n_stuffer_vnprintf(struct s2n_stuffer *stuffer, uint32_t max, const char *format, va_list vargs); + /* Read a private key from a PEM encoded stuffer to an ASN1/DER encoded one */ int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1, int *type); diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index e78663e91c2..de9ff62b96b 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -14,6 +14,7 @@ */ #include +#include #include "stuffer/s2n_stuffer.h" #include "utils/s2n_mem.h" @@ -206,3 +207,80 @@ int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data, return S2N_SUCCESS; } + +/* If we call va_start or va_copy there MUST be a matching call to va_end, + * so we should use DEFER_CLEANUP with our va_lists. + * Unfortunately, some environments implement va_list in unexpected ways + * (for example, as an array). + * To avoid any surprises, just wrap the va_list in our own struct. + */ +struct s2n_va_list { + va_list va_list; +}; + +static void s2n_va_list_cleanup(struct s2n_va_list *list) +{ + if (list) { + va_end(list->va_list); + } +} + +int s2n_stuffer_vnprintf(struct s2n_stuffer *stuffer, uint32_t max_size, + const char *format, va_list vargs_in) +{ + POSIX_PRECONDITION(s2n_stuffer_validate(stuffer)); + POSIX_ENSURE_REF(format); + + /* vsnprintf consumes the va_list, so copy it first */ + DEFER_CLEANUP(struct s2n_va_list vargs_1 = { 0 }, s2n_va_list_cleanup); + va_copy(vargs_1.va_list, vargs_in); + + /* The first call to vsnprintf calculates the size of the formatted string. + * str_len does not include the one byte vsnprintf requires for a trailing '\0', + * so we need one more byte. + */ + int str_len = vsnprintf(NULL, 0, format, vargs_1.va_list); + POSIX_ENSURE_GTE(str_len, 0); + POSIX_ENSURE_LT((unsigned int) str_len, UINT32_MAX); + uint32_t mem_size = MIN((uint32_t) str_len + 1, max_size); + + bool previously_tainted = stuffer->tainted; + char *str = s2n_stuffer_raw_write(stuffer, mem_size); + stuffer->tainted = previously_tainted; + POSIX_GUARD_PTR(str); + + /* vsnprintf again consumes the va_list, so copy it first */ + DEFER_CLEANUP(struct s2n_va_list vargs_2 = { 0 }, s2n_va_list_cleanup); + va_copy(vargs_2.va_list, vargs_in); + + /* This time, vsnprintf actually writes the formatted string */ + int written = vsnprintf(str, mem_size, format, vargs_2.va_list); + POSIX_ENSURE_GTE(written, 0); + + /* We don't actually use c-strings, so erase the final '\0' */ + POSIX_GUARD(s2n_stuffer_wipe_n(stuffer, 1)); + + return S2N_SUCCESS; +} + +int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs) +{ + POSIX_GUARD(s2n_stuffer_vnprintf(stuffer, UINT32_MAX, format, vargs)); + return S2N_SUCCESS; +} + +int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...) +{ + DEFER_CLEANUP(struct s2n_va_list vargs = { 0 }, s2n_va_list_cleanup); + va_start(vargs.va_list, format); + POSIX_GUARD(s2n_stuffer_vprintf(stuffer, format, vargs.va_list)); + return S2N_SUCCESS; +} + +int s2n_stuffer_nprintf(struct s2n_stuffer *stuffer, uint32_t max, const char *format, ...) +{ + DEFER_CLEANUP(struct s2n_va_list vargs = { 0 }, s2n_va_list_cleanup); + va_start(vargs.va_list, format); + POSIX_GUARD(s2n_stuffer_vnprintf(stuffer, max, format, vargs.va_list)); + return S2N_SUCCESS; +} diff --git a/tests/unit/s2n_security_rules_test.c b/tests/unit/s2n_security_rules_test.c new file mode 100644 index 00000000000..9d7c8e43229 --- /dev/null +++ b/tests/unit/s2n_security_rules_test.c @@ -0,0 +1,409 @@ +/* + * 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 "tls/s2n_security_rules.h" + +#include "s2n_test.h" +#include "tls/s2n_cipher_preferences.h" + +S2N_RESULT s2n_security_rule_validate_policy( + const struct s2n_security_rule *rule, + const struct s2n_security_policy *policy, + struct s2n_security_rule_result *result); + +struct s2n_cipher_suite *VALID_CIPHER_SUITE = &s2n_tls13_aes_256_gcm_sha384; +struct s2n_cipher_suite *EXAMPLE_INVALID_CIPHER_SUITE = &s2n_tls13_aes_128_gcm_sha256; +struct s2n_cipher_suite *EXAMPLE_INVALID_CIPHER_SUITE_2 = &s2n_tls13_chacha20_poly1305_sha256; +static S2N_RESULT s2n_test_cipher_suite_rule(const struct s2n_cipher_suite *cipher_suite, bool *valid) +{ + RESULT_ENSURE_REF(valid); + if (cipher_suite == VALID_CIPHER_SUITE) { + *valid = true; + } else { + *valid = false; + } + return S2N_RESULT_OK; +} + +const struct s2n_signature_scheme *VALID_SIG_SCHEME = &s2n_ecdsa_sha256; +const struct s2n_signature_scheme *EXAMPLE_INVALID_SIG_SCHEME = &s2n_ecdsa_sha384; +static S2N_RESULT s2n_test_sig_scheme_rule(const struct s2n_signature_scheme *sig_scheme, bool *valid) +{ + RESULT_ENSURE_REF(valid); + if (sig_scheme == VALID_SIG_SCHEME) { + *valid = true; + } else { + *valid = false; + } + return S2N_RESULT_OK; +} + +const struct s2n_ecc_named_curve *VALID_CURVE = &s2n_ecc_curve_secp256r1; +const struct s2n_ecc_named_curve *EXAMPLE_INVALID_CURVE = &s2n_ecc_curve_secp384r1; +static S2N_RESULT s2n_test_curve_rule(const struct s2n_ecc_named_curve *curve, bool *valid) +{ + RESULT_ENSURE_REF(valid); + if (curve == VALID_CURVE) { + *valid = true; + } else { + *valid = false; + } + return S2N_RESULT_OK; +} + +int main(int argc, char **argv) +{ + BEGIN_TEST(); + + const struct s2n_security_rule test_rule = { + .name = "Test Rule", + .validate_cipher_suite = s2n_test_cipher_suite_rule, + .validate_sig_scheme = s2n_test_sig_scheme_rule, + .validate_cert_sig_scheme = s2n_test_sig_scheme_rule, + .validate_curve = s2n_test_curve_rule, + }; + + const struct s2n_cipher_preferences valid_cipher_prefs = { + .suites = &VALID_CIPHER_SUITE, + .count = 1, + }; + struct s2n_cipher_suite *invalid_cipher_suites[] = { + EXAMPLE_INVALID_CIPHER_SUITE, + VALID_CIPHER_SUITE, + EXAMPLE_INVALID_CIPHER_SUITE_2, + }; + const struct s2n_cipher_preferences invalid_cipher_prefs = { + .suites = invalid_cipher_suites, + .count = s2n_array_len(invalid_cipher_suites), + }; + + const struct s2n_signature_preferences valid_sig_prefs = { + .signature_schemes = &VALID_SIG_SCHEME, + .count = 1, + }; + const struct s2n_signature_preferences invalid_sig_prefs = { + .signature_schemes = &EXAMPLE_INVALID_SIG_SCHEME, + .count = 1, + }; + + const struct s2n_ecc_preferences valid_ecc_prefs = { + .ecc_curves = &VALID_CURVE, + .count = 1, + }; + const struct s2n_ecc_preferences invalid_ecc_prefs = { + .ecc_curves = &EXAMPLE_INVALID_CURVE, + .count = 1, + }; + + const struct s2n_security_policy valid_policy = { + .cipher_preferences = &valid_cipher_prefs, + .signature_preferences = &valid_sig_prefs, + .certificate_signature_preferences = &valid_sig_prefs, + .ecc_preferences = &valid_ecc_prefs, + }; + const struct s2n_security_policy invalid_policy = { + .cipher_preferences = &invalid_cipher_prefs, + .signature_preferences = &invalid_sig_prefs, + .certificate_signature_preferences = &invalid_sig_prefs, + .ecc_preferences = &invalid_ecc_prefs, + }; + + /* Test s2n_security_rule_validate_policy */ + { + /* Test: Marks valid policy as valid */ + { + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &valid_policy, &result)); + EXPECT_FALSE(result.found_error); + }; + + /* Test: Marks invalid policy as invalid */ + { + /* Test: Entire policy invalid */ + { + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &invalid_policy, &result)); + EXPECT_TRUE(result.found_error); + }; + + /* Test: only cipher suite invalid */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.cipher_preferences = &invalid_cipher_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + }; + + /* Test: only sig scheme invalid */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.signature_preferences = &invalid_sig_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + }; + + /* Test: only cert sig scheme invalid */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.certificate_signature_preferences = &invalid_sig_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + }; + + /* Test: only curve invalid */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.ecc_preferences = &invalid_ecc_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + }; + }; + + /* Test: skips certificate signature preferences if not present */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.certificate_signature_preferences = NULL; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_FALSE(result.found_error); + }; + + /* Test: writes output for errors + * + * These tests are little brittle because they include hard-coded outputs. + * I think it's worthwhile to clearly verify the output. + */ + { + /* Test: Writes single line of output */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.ecc_preferences = &invalid_ecc_prefs; + const char expected_output[] = + "Test Rule: policy unnamed: curve: secp384r1 (#1)\n"; + + DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, + s2n_security_rule_result_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + + size_t output_size = s2n_stuffer_data_available(&result.output); + EXPECT_EQUAL(output_size, strlen(expected_output)); + uint8_t *output_bytes = s2n_stuffer_raw_read(&result.output, output_size); + EXPECT_NOT_NULL(output_bytes); + EXPECT_BYTEARRAY_EQUAL(expected_output, output_bytes, output_size); + }; + + /* Test: Writes multiple lines of output for same field */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.cipher_preferences = &invalid_cipher_prefs; + const char expected_output[] = + "Test Rule: policy unnamed: cipher suite: TLS_AES_128_GCM_SHA256 (#1)\n" + "Test Rule: policy unnamed: cipher suite: TLS_CHACHA20_POLY1305_SHA256 (#3)\n"; + + DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, + s2n_security_rule_result_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + + size_t output_size = s2n_stuffer_data_available(&result.output); + EXPECT_EQUAL(output_size, strlen(expected_output)); + uint8_t *output_bytes = s2n_stuffer_raw_read(&result.output, output_size); + EXPECT_NOT_NULL(output_bytes); + EXPECT_BYTEARRAY_EQUAL(expected_output, output_bytes, output_size); + }; + + /* Test: Writes multiple lines of output for different fields */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.cipher_preferences = &invalid_cipher_prefs; + test_policy.ecc_preferences = &invalid_ecc_prefs; + const char expected_output[] = + "Test Rule: policy unnamed: cipher suite: TLS_AES_128_GCM_SHA256 (#1)\n" + "Test Rule: policy unnamed: cipher suite: TLS_CHACHA20_POLY1305_SHA256 (#3)\n" + "Test Rule: policy unnamed: curve: secp384r1 (#1)\n"; + + DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, + s2n_security_rule_result_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + + size_t output_size = s2n_stuffer_data_available(&result.output); + EXPECT_EQUAL(output_size, strlen(expected_output)); + uint8_t *output_bytes = s2n_stuffer_raw_read(&result.output, output_size); + EXPECT_NOT_NULL(output_bytes); + EXPECT_BYTEARRAY_EQUAL(expected_output, output_bytes, output_size); + }; + + /* Writes correct name for versioned policy */ + { + const char expected_prefix[] = + "Test Rule: policy test_all: "; + const size_t expected_prefix_size = strlen(expected_prefix); + + DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, + s2n_security_rule_result_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &security_policy_test_all, &result)); + EXPECT_TRUE(result.found_error); + + size_t output_size = s2n_stuffer_data_available(&result.output); + EXPECT_TRUE(output_size > expected_prefix_size); + uint8_t *output_bytes = s2n_stuffer_raw_read(&result.output, expected_prefix_size); + EXPECT_NOT_NULL(output_bytes); + EXPECT_BYTEARRAY_EQUAL(output_bytes, expected_prefix, expected_prefix_size); + }; + + /* Test: Truncates output if insufficient memory for output */ + { + const char full_output[] = + "Test Rule: policy unnamed: curve: secp384r1 (#1)\n"; + const size_t full_output_size = strlen(full_output); + struct { + uint32_t mem_size; + const char expected_output[sizeof(full_output)]; + } test_cases[] = { + /* No truncation if sufficient space */ + { + .mem_size = full_output_size, + .expected_output = "Test Rule: policy unnamed: curve: secp384r1 (#1)\n" }, + /* Truncate if necessary */ + { + .mem_size = full_output_size - 1, + .expected_output = "Test Rule: policy unnamed: curve: secp384r1 ...\n" }, + /* Print only the truncation message */ + { + .mem_size = strlen("...\n"), + .expected_output = "...\n" }, + /* Truncate the truncation message */ + { + .mem_size = 3, + .expected_output = "..\n" }, + { .mem_size = 1, + .expected_output = "\n" }, + }; + + struct s2n_security_policy test_policy = valid_policy; + test_policy.ecc_preferences = &invalid_ecc_prefs; + + for (size_t i = 0; i < s2n_array_len(test_cases); i++) { + DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, + s2n_security_rule_result_free); + EXPECT_SUCCESS(s2n_stuffer_alloc(&result.output, test_cases[i].mem_size)); + + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + + size_t output_size = s2n_stuffer_data_available(&result.output); + EXPECT_EQUAL(output_size, test_cases[i].mem_size); + uint8_t *output_bytes = s2n_stuffer_raw_read(&result.output, test_cases[i].mem_size); + EXPECT_NOT_NULL(output_bytes); + EXPECT_BYTEARRAY_EQUAL(test_cases[i].expected_output, output_bytes, output_size); + } + }; + }; + }; + + /* Test s2n_security_policy_validate_security_rules + * (and S2N_PERFECT_FORWARD_SECRECY-- we need to test with a real rule) */ + { + struct s2n_cipher_suite *not_forward_secret_suite = &s2n_rsa_with_aes_128_gcm_sha256; + const struct s2n_cipher_preferences not_forward_secret_prefs = { + .suites = ¬_forward_secret_suite, + .count = 1, + }; + + struct s2n_cipher_suite *forward_secret_suites[] = { + &s2n_ecdhe_ecdsa_with_aes_128_gcm_sha256, + &s2n_tls13_aes_256_gcm_sha384, + }; + const struct s2n_cipher_preferences forward_secret_prefs = { + .suites = forward_secret_suites, + .count = s2n_array_len(forward_secret_suites), + }; + + struct s2n_security_policy test_policy = valid_policy; + + /* Test: valid policy passes */ + { + test_policy.rules = S2N_PERFECT_FORWARD_SECRECY_FLAG; + test_policy.cipher_preferences = &forward_secret_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_policy_validate_security_rules(&test_policy, &result)); + EXPECT_FALSE(result.found_error); + }; + + /* Test: invalid policy fails */ + { + test_policy.rules = S2N_PERFECT_FORWARD_SECRECY_FLAG; + test_policy.cipher_preferences = ¬_forward_secret_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_policy_validate_security_rules(&test_policy, &result)); + EXPECT_TRUE(result.found_error); + }; + + /* Test: valid policy without rule passes */ + { + test_policy.rules = 0; + test_policy.cipher_preferences = &forward_secret_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_policy_validate_security_rules(&test_policy, &result)); + EXPECT_FALSE(result.found_error); + }; + + /* Test: invalid policy without rule passes */ + { + test_policy.rules = 0; + test_policy.cipher_preferences = ¬_forward_secret_prefs; + + struct s2n_security_rule_result result = { 0 }; + EXPECT_OK(s2n_security_policy_validate_security_rules(&test_policy, &result)); + EXPECT_FALSE(result.found_error); + }; + }; + + END_TEST(); +} diff --git a/tests/unit/s2n_stuffer_text_test.c b/tests/unit/s2n_stuffer_text_test.c index b8e066f317b..b66b6d61e21 100644 --- a/tests/unit/s2n_stuffer_text_test.c +++ b/tests/unit/s2n_stuffer_text_test.c @@ -162,6 +162,159 @@ int main(int argc, char **argv) EXPECT_SUCCESS(memcmp("not a line", token.blob.data, s2n_stuffer_data_available(&token))); }; + /* Test s2n_stuffer_printf and s2n_stuffer_nprintf */ + { + const char *format_str = "str (%s) and int (%i)"; + const char *str_arg = "hello"; + const int int_arg = 5; + + const char *expected_str = "str (hello) and int (5)"; + const size_t expected_len = strlen(expected_str); + const size_t mem_size = expected_len + 1; + + /* Verify expected_str */ + { + char result_str[100] = { 0 }; + int result_len = snprintf(result_str, sizeof(result_str), + format_str, str_arg, int_arg); + EXPECT_TRUE(result_len < sizeof(result_str)); + + EXPECT_EQUAL(result_len, expected_len); + EXPECT_BYTEARRAY_EQUAL(expected_str, result_str, result_len); + } + + /* Print formatted message */ + { + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_alloc(&test, mem_size)); + EXPECT_SUCCESS(s2n_stuffer_printf(&test, format_str, str_arg, int_arg)); + + EXPECT_EQUAL(s2n_stuffer_data_available(&test), expected_len); + uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, expected_len); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, expected_len); + }; + + /* Print formatted message with no arguments */ + { + const char no_args_str[] = "hello world"; + const size_t no_args_str_len = strlen(no_args_str); + + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_alloc(&test, sizeof(no_args_str))); + EXPECT_SUCCESS(s2n_stuffer_printf(&test, no_args_str)); + + EXPECT_EQUAL(s2n_stuffer_data_available(&test), no_args_str_len); + uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, no_args_str_len); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, no_args_str, no_args_str_len); + }; + + /* Message too large for fixed size stuffer */ + { + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_alloc(&test, mem_size - 1)); + EXPECT_FAILURE_WITH_ERRNO( + s2n_stuffer_printf(&test, format_str, str_arg, int_arg), + S2N_ERR_STUFFER_IS_FULL); + }; + + /* Message too large for growable stuffer */ + { + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&test, 0)); + EXPECT_EQUAL(test.blob.allocated, 0); + + EXPECT_SUCCESS(s2n_stuffer_printf(&test, format_str, str_arg, int_arg)); + + EXPECT_EQUAL(s2n_stuffer_data_available(&test), expected_len); + uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, expected_len); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, expected_len); + + EXPECT_TRUE(test.blob.allocated > 0); + }; + + /* Multiple writes */ + { + const char full_str[] = "hello world"; + const size_t full_str_size = strlen(full_str); + + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_alloc(&test, mem_size)); + EXPECT_SUCCESS(s2n_stuffer_printf(&test, "hel")); + EXPECT_SUCCESS(s2n_stuffer_printf(&test, "%s", "lo")); + EXPECT_SUCCESS(s2n_stuffer_printf(&test, "%cworl%c", ' ', 'd')); + + EXPECT_EQUAL(s2n_stuffer_data_available(&test), full_str_size); + uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, full_str_size); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, full_str, full_str_size); + }; + + /* Stuffer tracking of 'tainted' unaffected */ + { + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&test, 100)); + EXPECT_FALSE(test.tainted); + + EXPECT_SUCCESS(s2n_stuffer_printf(&test, "hello")); + EXPECT_FALSE(test.tainted); + + uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, 1); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_TRUE(test.tainted); + + EXPECT_SUCCESS(s2n_stuffer_printf(&test, "hello")); + EXPECT_TRUE(test.tainted); + }; + + /* Maximum size respected */ + { + DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); + EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&test, 0)); + + uint32_t max_size = 1; + uint32_t write_size = 0; + EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); + EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); + + max_size = 2; + /* The underlying snprintf method reserves 1 byte for the final '\0' */ + write_size = max_size - 1; + EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); + EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); + uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, write_size); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, write_size); + + max_size = expected_len - 1; + /* The underlying snprintf method reserves 1 byte for the final '\0' */ + write_size = max_size - 1; + EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); + EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); + actual_bytes = s2n_stuffer_raw_read(&test, write_size); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, write_size); + + max_size = expected_len; + /* The underlying snprintf method reserves 1 byte for the final '\0' */ + write_size = max_size - 1; + EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); + EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); + actual_bytes = s2n_stuffer_raw_read(&test, write_size); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, write_size); + + max_size = expected_len + 1; + EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); + EXPECT_EQUAL(s2n_stuffer_data_available(&test), expected_len); + actual_bytes = s2n_stuffer_raw_read(&test, expected_len); + EXPECT_NOT_NULL(actual_bytes); + EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, expected_len); + }; + }; + END_TEST(); return 0; } diff --git a/tls/s2n_security_policies.c b/tls/s2n_security_policies.c index 0ede9a44fdf..cb744e94569 100644 --- a/tls/s2n_security_policies.c +++ b/tls/s2n_security_policies.c @@ -1297,3 +1297,16 @@ S2N_RESULT s2n_validate_certificate_signature_preferences(const struct s2n_signa RESULT_ENSURE(rsa_pss_scheme_count == NUM_RSA_PSS_SCHEMES || rsa_pss_scheme_count == 0, S2N_ERR_INVALID_SECURITY_POLICY); return S2N_RESULT_OK; } + +S2N_RESULT s2n_security_policy_get_version(const struct s2n_security_policy *security_policy, const char **version) +{ + RESULT_ENSURE_REF(version); + *version = NULL; + for (uint8_t i = 0; security_policy_selection[i].version != NULL; i++) { + if (security_policy_selection[i].security_policy == security_policy) { + *version = security_policy_selection[i].version; + return S2N_RESULT_OK; + } + } + RESULT_BAIL(S2N_ERR_INVALID_SECURITY_POLICY); +} diff --git a/tls/s2n_security_policies.h b/tls/s2n_security_policies.h index 953cb1d2598..24e6e49d533 100644 --- a/tls/s2n_security_policies.h +++ b/tls/s2n_security_policies.h @@ -20,6 +20,7 @@ #include "tls/s2n_cipher_preferences.h" #include "tls/s2n_ecc_preferences.h" #include "tls/s2n_kem_preferences.h" +#include "tls/s2n_security_rules.h" #include "tls/s2n_signature_scheme.h" /* Kept up-to-date by s2n_security_policies_test */ @@ -69,6 +70,7 @@ struct s2n_security_policy { * https://www.rfc-editor.org/rfc/rfc8446#section-4.2.7 */ const struct s2n_ecc_preferences *ecc_preferences; + s2n_security_flag rules; }; struct s2n_security_policy_selection { @@ -188,3 +190,4 @@ bool s2n_security_policy_supports_tls13(const struct s2n_security_policy *securi int s2n_find_security_policy_from_version(const char *version, const struct s2n_security_policy **security_policy); int s2n_validate_kem_preferences(const struct s2n_kem_preferences *kem_preferences, bool pq_kem_extension_required); S2N_RESULT s2n_validate_certificate_signature_preferences(const struct s2n_signature_preferences *s2n_certificate_signature_preferences); +S2N_RESULT s2n_security_policy_get_version(const struct s2n_security_policy *security_policy, const char **version); diff --git a/tls/s2n_security_rules.c b/tls/s2n_security_rules.c new file mode 100644 index 00000000000..3e7e413fd85 --- /dev/null +++ b/tls/s2n_security_rules.c @@ -0,0 +1,229 @@ +/* + * 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 "tls/s2n_security_rules.h" + +#include + +#include "tls/s2n_cipher_suites.h" +#include "tls/s2n_signature_scheme.h" +#include "utils/s2n_result.h" +#include "utils/s2n_safety.h" + +#define TRUNCATED_STR "..." +#define TRUNCATED_STR_LEN (sizeof(TRUNCATED_STR) - 1) + +static S2N_RESULT s2n_security_rule_result_write_output(struct s2n_security_rule_result *result, + bool condition, const char *format, va_list vargs) +{ + RESULT_ENSURE_REF(result); + if (result->output_truncated) { + return S2N_RESULT_OK; + } + + if (s2n_stuffer_vprintf(&result->output, format, vargs) != S2N_SUCCESS) { + /* If we failed to write the full message, then our stuffer isn't growable + * and doesn't have sufficient space. We'll need to truncate. + */ + result->output_truncated = true; + + /* Write as much of the full message as possible */ + uint32_t space_remaining = s2n_stuffer_space_remaining(&result->output); + RESULT_GUARD_POSIX(s2n_stuffer_vnprintf(&result->output, space_remaining, format, vargs)); + + /* Overwrite the end of the message to indicate the truncation. + * We don't want readers to think they have the whole message. + */ + uint32_t to_rewrite = MIN(s2n_stuffer_data_available(&result->output), TRUNCATED_STR_LEN); + RESULT_GUARD_POSIX(s2n_stuffer_wipe_n(&result->output, to_rewrite)); + RESULT_GUARD_POSIX(s2n_stuffer_write_bytes(&result->output, (const uint8_t *) TRUNCATED_STR, to_rewrite)); + } + + /* The stuffer_printf methods always allow space for one extra char due to + * the '\0' written by the underlying snprintf method. + * We use that extra char for a newline. + */ + RESULT_GUARD_POSIX(s2n_stuffer_write_char(&result->output, '\n')); + return S2N_RESULT_OK; +} + +static S2N_RESULT s2n_security_rule_result_process(struct s2n_security_rule_result *result, + bool condition, const char *format, ...) +{ + RESULT_ENSURE_REF(result); + if (condition) { + return S2N_RESULT_OK; + } + result->found_error = true; + + va_list vargs; + va_start(vargs, format); + /* A failure writing the output shouldn't cause the entire validation to fail */ + s2n_result_ignore(s2n_security_rule_result_write_output(result, condition, format, vargs)); + va_end(vargs); + return S2N_RESULT_OK; +} + +static S2N_RESULT s2n_security_rule_validate_forward_secret( + const struct s2n_cipher_suite *cipher_suite, bool *valid) +{ + RESULT_ENSURE_REF(cipher_suite); + RESULT_ENSURE_REF(cipher_suite->key_exchange_alg); + *valid = cipher_suite->key_exchange_alg->is_ephemeral; + return S2N_RESULT_OK; +} + +static S2N_RESULT s2n_security_rule_all_sig_schemes( + const struct s2n_signature_scheme *sig_scheme, bool *valid) +{ + RESULT_ENSURE_REF(valid); + *valid = true; + return S2N_RESULT_OK; +} + +static S2N_RESULT s2n_security_rule_all_curves( + const struct s2n_ecc_named_curve *curve, bool *valid) +{ + RESULT_ENSURE_REF(valid); + *valid = true; + return S2N_RESULT_OK; +} + +const struct s2n_security_rule security_rule_definitions[] = { + [S2N_PERFECT_FORWARD_SECRECY] = { + .name = "Perfect Forward Secrecy", + .validate_cipher_suite = s2n_security_rule_validate_forward_secret, + .validate_sig_scheme = s2n_security_rule_all_sig_schemes, + .validate_cert_sig_scheme = s2n_security_rule_all_sig_schemes, + .validate_curve = s2n_security_rule_all_curves, + }, +}; + +S2N_RESULT s2n_security_rule_validate_policy(const struct s2n_security_rule *rule, + const struct s2n_security_policy *policy, struct s2n_security_rule_result *result) +{ + RESULT_ENSURE_REF(rule); + RESULT_ENSURE_REF(policy); + RESULT_ENSURE_REF(result); + + const char *policy_name = NULL; + s2n_result_ignore(s2n_security_policy_get_version(policy, &policy_name)); + if (policy_name == NULL) { + policy_name = "unnamed"; + } + + const char *error_msg_format_name = "%s: policy %s: %s: %s (#%i)"; + const char *error_msg_format_iana = "%s: policy %s: %s: %x (#%i)"; + + const struct s2n_cipher_preferences *cipher_prefs = policy->cipher_preferences; + RESULT_ENSURE_REF(cipher_prefs); + for (size_t i = 0; i < cipher_prefs->count; i++) { + const struct s2n_cipher_suite *cipher_suite = cipher_prefs->suites[i]; + RESULT_ENSURE_REF(cipher_suite); + bool is_valid = false; + RESULT_GUARD(rule->validate_cipher_suite(cipher_suite, &is_valid)); + RESULT_GUARD(s2n_security_rule_result_process(result, is_valid, + error_msg_format_name, rule->name, policy_name, + "cipher suite", cipher_suite->name, i + 1)); + } + + const struct s2n_signature_preferences *sig_prefs = policy->signature_preferences; + RESULT_ENSURE_REF(sig_prefs); + for (size_t i = 0; i < sig_prefs->count; i++) { + const struct s2n_signature_scheme *sig_scheme = sig_prefs->signature_schemes[i]; + RESULT_ENSURE_REF(sig_scheme); + bool is_valid = false; + RESULT_GUARD(rule->validate_sig_scheme(sig_scheme, &is_valid)); + RESULT_GUARD(s2n_security_rule_result_process(result, is_valid, + error_msg_format_iana, rule->name, policy_name, + "signature scheme", sig_scheme->iana_value, i + 1)); + } + + const struct s2n_signature_preferences *cert_sig_prefs = policy->certificate_signature_preferences; + if (cert_sig_prefs) { + for (size_t i = 0; i < cert_sig_prefs->count; i++) { + const struct s2n_signature_scheme *sig_scheme = cert_sig_prefs->signature_schemes[i]; + RESULT_ENSURE_REF(sig_scheme); + bool is_valid = false; + RESULT_GUARD(rule->validate_cert_sig_scheme(sig_scheme, &is_valid)); + RESULT_GUARD(s2n_security_rule_result_process(result, is_valid, + error_msg_format_iana, rule->name, policy_name, + "certificate signature scheme", sig_scheme->iana_value, i + 1)); + } + } + + const struct s2n_ecc_preferences *ecc_prefs = policy->ecc_preferences; + RESULT_ENSURE_REF(ecc_prefs); + for (size_t i = 0; i < ecc_prefs->count; i++) { + const struct s2n_ecc_named_curve *curve = ecc_prefs->ecc_curves[i]; + RESULT_ENSURE_REF(curve); + bool is_valid = false; + RESULT_GUARD(rule->validate_curve(curve, &is_valid)); + RESULT_GUARD(s2n_security_rule_result_process(result, is_valid, + error_msg_format_name, rule->name, policy_name, + "curve", curve->name, i + 1)); + } + + return S2N_RESULT_OK; +} + +static S2N_RESULT s2n_security_policy_get_security_rules( + const struct s2n_security_policy *policy, const struct s2n_security_rule **rules, + size_t max_rules_count, size_t *rules_count) +{ + RESULT_ENSURE_REF(policy); + RESULT_ENSURE_REF(rules_count); + *rules_count = 0; + + size_t flags = policy->rules; + size_t count = 0; + for (size_t rule = 0; rule < S2N_SECURITY_RULES_COUNT; rule++) { + bool is_set = flags % 2; + flags = flags >> 1; + + if (!is_set) { + continue; + } + + RESULT_ENSURE_INCLUSIVE_RANGE(0, rule, s2n_array_len(security_rule_definitions)); + RESULT_ENSURE_LT(count, max_rules_count); + rules[count] = &security_rule_definitions[rule]; + count++; + } + + *rules_count = count; + return S2N_RESULT_OK; +} + +S2N_RESULT s2n_security_policy_validate_security_rules( + const struct s2n_security_policy *policy, struct s2n_security_rule_result *result) +{ + const struct s2n_security_rule *rules[S2N_SECURITY_RULES_COUNT] = { 0 }; + size_t rules_count = 0; + RESULT_GUARD(s2n_security_policy_get_security_rules(policy, + rules, s2n_array_len(rules), &rules_count)); + for (size_t i = 0; i < rules_count; i++) { + RESULT_GUARD(s2n_security_rule_validate_policy(rules[i], policy, result)); + } + return S2N_RESULT_OK; +} + +S2N_CLEANUP_RESULT s2n_security_rule_result_free(struct s2n_security_rule_result *result) +{ + if (result) { + RESULT_GUARD_POSIX(s2n_stuffer_free(&result->output)); + } + return S2N_RESULT_OK; +} diff --git a/tls/s2n_security_rules.h b/tls/s2n_security_rules.h new file mode 100644 index 00000000000..384ec892667 --- /dev/null +++ b/tls/s2n_security_rules.h @@ -0,0 +1,53 @@ +/* + * 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. + */ + +#pragma once + +#include "stuffer/s2n_stuffer.h" +#include "utils/s2n_result.h" + +typedef enum { + S2N_PERFECT_FORWARD_SECRECY = 0, + S2N_SECURITY_RULES_COUNT, +} s2n_security_rule_id; + +#define TO_FLAG(rule) rule##_FLAG = (1 << (rule)) +typedef enum { + TO_FLAG(S2N_PERFECT_FORWARD_SECRECY), +} s2n_security_flag; + +struct s2n_security_rule_result { + bool found_error; + bool output_truncated; + struct s2n_stuffer output; +}; +S2N_CLEANUP_RESULT s2n_security_rule_result_free(struct s2n_security_rule_result *result); + +struct s2n_security_policy; +struct s2n_cipher_suite; +struct s2n_signature_scheme; +struct s2n_ecc_named_curve; + +struct s2n_security_rule { + const char *name; + S2N_RESULT (*validate_cipher_suite)(const struct s2n_cipher_suite *cipher_suite, bool *valid); + S2N_RESULT (*validate_sig_scheme)(const struct s2n_signature_scheme *sig_scheme, bool *valid); + S2N_RESULT (*validate_cert_sig_scheme)(const struct s2n_signature_scheme *sig_scheme, bool *valid); + S2N_RESULT (*validate_curve)(const struct s2n_ecc_named_curve *curve, bool *valid); +}; + +S2N_RESULT s2n_security_policy_validate_security_rules( + const struct s2n_security_policy *policy, + struct s2n_security_rule_result *result); From 3b86e0a75495070530fbfa7709c5c4ebd67bdb73 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Sun, 26 Nov 2023 23:31:01 -0800 Subject: [PATCH 2/5] Address PR comments-- mostly simplify output stuffer --- stuffer/s2n_stuffer.h | 7 +-- stuffer/s2n_stuffer_text.c | 20 +------- tests/unit/s2n_security_rules_test.c | 73 ++++++++-------------------- tests/unit/s2n_stuffer_text_test.c | 59 +++------------------- tls/s2n_security_rules.c | 56 +++++++-------------- tls/s2n_security_rules.h | 3 +- 6 files changed, 50 insertions(+), 168 deletions(-) diff --git a/stuffer/s2n_stuffer.h b/stuffer/s2n_stuffer.h index 51f3eff47d7..0e8950c2ed0 100644 --- a/stuffer/s2n_stuffer.h +++ b/stuffer/s2n_stuffer.h @@ -170,14 +170,11 @@ int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data, * - They write bytes, not strings. They do not write a final '\0'. Unfortunately, * they do still require enough space for a final '\0'-- we'd have to reimplement * sprintf to avoid that. - * - vprintf/vnprintf do not consume their vargs. They call va_copy before using - * the varg argument, so they can be called repeatedly with the same vargs. - * - nprintf/vnprintf do not return the actual size of the formatted string. + * - vprintf does not consume the vargs. It calls va_copy before using + * the varg argument, so can be called repeatedly with the same vargs. */ int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...); -int s2n_stuffer_nprintf(struct s2n_stuffer *stuffer, uint32_t max, const char *format, ...); int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs); -int s2n_stuffer_vnprintf(struct s2n_stuffer *stuffer, uint32_t max, const char *format, va_list vargs); /* Read a private key from a PEM encoded stuffer to an ASN1/DER encoded one */ int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1, int *type); diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index de9ff62b96b..c5443417b22 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -225,8 +225,7 @@ static void s2n_va_list_cleanup(struct s2n_va_list *list) } } -int s2n_stuffer_vnprintf(struct s2n_stuffer *stuffer, uint32_t max_size, - const char *format, va_list vargs_in) +int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs_in) { POSIX_PRECONDITION(s2n_stuffer_validate(stuffer)); POSIX_ENSURE_REF(format); @@ -241,8 +240,7 @@ int s2n_stuffer_vnprintf(struct s2n_stuffer *stuffer, uint32_t max_size, */ int str_len = vsnprintf(NULL, 0, format, vargs_1.va_list); POSIX_ENSURE_GTE(str_len, 0); - POSIX_ENSURE_LT((unsigned int) str_len, UINT32_MAX); - uint32_t mem_size = MIN((uint32_t) str_len + 1, max_size); + int mem_size = str_len + 1; bool previously_tainted = stuffer->tainted; char *str = s2n_stuffer_raw_write(stuffer, mem_size); @@ -263,12 +261,6 @@ int s2n_stuffer_vnprintf(struct s2n_stuffer *stuffer, uint32_t max_size, return S2N_SUCCESS; } -int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs) -{ - POSIX_GUARD(s2n_stuffer_vnprintf(stuffer, UINT32_MAX, format, vargs)); - return S2N_SUCCESS; -} - int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...) { DEFER_CLEANUP(struct s2n_va_list vargs = { 0 }, s2n_va_list_cleanup); @@ -276,11 +268,3 @@ int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...) POSIX_GUARD(s2n_stuffer_vprintf(stuffer, format, vargs.va_list)); return S2N_SUCCESS; } - -int s2n_stuffer_nprintf(struct s2n_stuffer *stuffer, uint32_t max, const char *format, ...) -{ - DEFER_CLEANUP(struct s2n_va_list vargs = { 0 }, s2n_va_list_cleanup); - va_start(vargs.va_list, format); - POSIX_GUARD(s2n_stuffer_vnprintf(stuffer, max, format, vargs.va_list)); - return S2N_SUCCESS; -} diff --git a/tests/unit/s2n_security_rules_test.c b/tests/unit/s2n_security_rules_test.c index 9d7c8e43229..f6b6e74d992 100644 --- a/tests/unit/s2n_security_rules_test.c +++ b/tests/unit/s2n_security_rules_test.c @@ -202,6 +202,22 @@ int main(int argc, char **argv) * I think it's worthwhile to clearly verify the output. */ { + /* Test: Writes no output if output not enabled */ + { + struct s2n_security_policy test_policy = valid_policy; + test_policy.ecc_preferences = &invalid_ecc_prefs; + + DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, + s2n_security_rule_result_free); + + EXPECT_OK(s2n_security_rule_validate_policy( + &test_rule, &test_policy, &result)); + EXPECT_TRUE(result.found_error); + + size_t output_size = s2n_stuffer_data_available(&result.output); + EXPECT_EQUAL(output_size, 0); + }; + /* Test: Writes single line of output */ { struct s2n_security_policy test_policy = valid_policy; @@ -211,7 +227,7 @@ int main(int argc, char **argv) DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, s2n_security_rule_result_free); - EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + EXPECT_OK(s2n_security_rule_result_init_output(&result)); EXPECT_OK(s2n_security_rule_validate_policy( &test_rule, &test_policy, &result)); @@ -234,7 +250,7 @@ int main(int argc, char **argv) DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, s2n_security_rule_result_free); - EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + EXPECT_OK(s2n_security_rule_result_init_output(&result)); EXPECT_OK(s2n_security_rule_validate_policy( &test_rule, &test_policy, &result)); @@ -259,7 +275,7 @@ int main(int argc, char **argv) DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, s2n_security_rule_result_free); - EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + EXPECT_OK(s2n_security_rule_result_init_output(&result)); EXPECT_OK(s2n_security_rule_validate_policy( &test_rule, &test_policy, &result)); @@ -280,7 +296,7 @@ int main(int argc, char **argv) DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, s2n_security_rule_result_free); - EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&result.output, 100)); + EXPECT_OK(s2n_security_rule_result_init_output(&result)); EXPECT_OK(s2n_security_rule_validate_policy( &test_rule, &security_policy_test_all, &result)); @@ -292,55 +308,6 @@ int main(int argc, char **argv) EXPECT_NOT_NULL(output_bytes); EXPECT_BYTEARRAY_EQUAL(output_bytes, expected_prefix, expected_prefix_size); }; - - /* Test: Truncates output if insufficient memory for output */ - { - const char full_output[] = - "Test Rule: policy unnamed: curve: secp384r1 (#1)\n"; - const size_t full_output_size = strlen(full_output); - struct { - uint32_t mem_size; - const char expected_output[sizeof(full_output)]; - } test_cases[] = { - /* No truncation if sufficient space */ - { - .mem_size = full_output_size, - .expected_output = "Test Rule: policy unnamed: curve: secp384r1 (#1)\n" }, - /* Truncate if necessary */ - { - .mem_size = full_output_size - 1, - .expected_output = "Test Rule: policy unnamed: curve: secp384r1 ...\n" }, - /* Print only the truncation message */ - { - .mem_size = strlen("...\n"), - .expected_output = "...\n" }, - /* Truncate the truncation message */ - { - .mem_size = 3, - .expected_output = "..\n" }, - { .mem_size = 1, - .expected_output = "\n" }, - }; - - struct s2n_security_policy test_policy = valid_policy; - test_policy.ecc_preferences = &invalid_ecc_prefs; - - for (size_t i = 0; i < s2n_array_len(test_cases); i++) { - DEFER_CLEANUP(struct s2n_security_rule_result result = { 0 }, - s2n_security_rule_result_free); - EXPECT_SUCCESS(s2n_stuffer_alloc(&result.output, test_cases[i].mem_size)); - - EXPECT_OK(s2n_security_rule_validate_policy( - &test_rule, &test_policy, &result)); - EXPECT_TRUE(result.found_error); - - size_t output_size = s2n_stuffer_data_available(&result.output); - EXPECT_EQUAL(output_size, test_cases[i].mem_size); - uint8_t *output_bytes = s2n_stuffer_raw_read(&result.output, test_cases[i].mem_size); - EXPECT_NOT_NULL(output_bytes); - EXPECT_BYTEARRAY_EQUAL(test_cases[i].expected_output, output_bytes, output_size); - } - }; }; }; diff --git a/tests/unit/s2n_stuffer_text_test.c b/tests/unit/s2n_stuffer_text_test.c index b66b6d61e21..04a0a1ad94d 100644 --- a/tests/unit/s2n_stuffer_text_test.c +++ b/tests/unit/s2n_stuffer_text_test.c @@ -162,7 +162,7 @@ int main(int argc, char **argv) EXPECT_SUCCESS(memcmp("not a line", token.blob.data, s2n_stuffer_data_available(&token))); }; - /* Test s2n_stuffer_printf and s2n_stuffer_nprintf */ + /* Test s2n_stuffer_printf */ { const char *format_str = "str (%s) and int (%i)"; const char *str_arg = "hello"; @@ -172,7 +172,7 @@ int main(int argc, char **argv) const size_t expected_len = strlen(expected_str); const size_t mem_size = expected_len + 1; - /* Verify expected_str */ + /* Sanity check: Verify expected_str matches snprintf output */ { char result_str[100] = { 0 }; int result_len = snprintf(result_str, sizeof(result_str), @@ -183,7 +183,7 @@ int main(int argc, char **argv) EXPECT_BYTEARRAY_EQUAL(expected_str, result_str, result_len); } - /* Print formatted message */ + /* Test: Print formatted message */ { DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); EXPECT_SUCCESS(s2n_stuffer_alloc(&test, mem_size)); @@ -195,7 +195,7 @@ int main(int argc, char **argv) EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, expected_len); }; - /* Print formatted message with no arguments */ + /* Test: Print formatted message with no arguments */ { const char no_args_str[] = "hello world"; const size_t no_args_str_len = strlen(no_args_str); @@ -210,7 +210,7 @@ int main(int argc, char **argv) EXPECT_BYTEARRAY_EQUAL(actual_bytes, no_args_str, no_args_str_len); }; - /* Message too large for fixed size stuffer */ + /* Test: Message too large for fixed size stuffer */ { DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); EXPECT_SUCCESS(s2n_stuffer_alloc(&test, mem_size - 1)); @@ -219,7 +219,7 @@ int main(int argc, char **argv) S2N_ERR_STUFFER_IS_FULL); }; - /* Message too large for growable stuffer */ + /* Test: Message too large for growable stuffer */ { DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&test, 0)); @@ -235,7 +235,7 @@ int main(int argc, char **argv) EXPECT_TRUE(test.blob.allocated > 0); }; - /* Multiple writes */ + /* Test: Multiple writes */ { const char full_str[] = "hello world"; const size_t full_str_size = strlen(full_str); @@ -268,51 +268,6 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_stuffer_printf(&test, "hello")); EXPECT_TRUE(test.tainted); }; - - /* Maximum size respected */ - { - DEFER_CLEANUP(struct s2n_stuffer test = { 0 }, s2n_stuffer_free); - EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&test, 0)); - - uint32_t max_size = 1; - uint32_t write_size = 0; - EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); - EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); - - max_size = 2; - /* The underlying snprintf method reserves 1 byte for the final '\0' */ - write_size = max_size - 1; - EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); - EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); - uint8_t *actual_bytes = s2n_stuffer_raw_read(&test, write_size); - EXPECT_NOT_NULL(actual_bytes); - EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, write_size); - - max_size = expected_len - 1; - /* The underlying snprintf method reserves 1 byte for the final '\0' */ - write_size = max_size - 1; - EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); - EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); - actual_bytes = s2n_stuffer_raw_read(&test, write_size); - EXPECT_NOT_NULL(actual_bytes); - EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, write_size); - - max_size = expected_len; - /* The underlying snprintf method reserves 1 byte for the final '\0' */ - write_size = max_size - 1; - EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); - EXPECT_EQUAL(s2n_stuffer_data_available(&test), write_size); - actual_bytes = s2n_stuffer_raw_read(&test, write_size); - EXPECT_NOT_NULL(actual_bytes); - EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, write_size); - - max_size = expected_len + 1; - EXPECT_SUCCESS(s2n_stuffer_nprintf(&test, max_size, format_str, str_arg, int_arg)); - EXPECT_EQUAL(s2n_stuffer_data_available(&test), expected_len); - actual_bytes = s2n_stuffer_raw_read(&test, expected_len); - EXPECT_NOT_NULL(actual_bytes); - EXPECT_BYTEARRAY_EQUAL(actual_bytes, expected_str, expected_len); - }; }; END_TEST(); diff --git a/tls/s2n_security_rules.c b/tls/s2n_security_rules.c index 3e7e413fd85..c8b466b1a29 100644 --- a/tls/s2n_security_rules.c +++ b/tls/s2n_security_rules.c @@ -22,43 +22,6 @@ #include "utils/s2n_result.h" #include "utils/s2n_safety.h" -#define TRUNCATED_STR "..." -#define TRUNCATED_STR_LEN (sizeof(TRUNCATED_STR) - 1) - -static S2N_RESULT s2n_security_rule_result_write_output(struct s2n_security_rule_result *result, - bool condition, const char *format, va_list vargs) -{ - RESULT_ENSURE_REF(result); - if (result->output_truncated) { - return S2N_RESULT_OK; - } - - if (s2n_stuffer_vprintf(&result->output, format, vargs) != S2N_SUCCESS) { - /* If we failed to write the full message, then our stuffer isn't growable - * and doesn't have sufficient space. We'll need to truncate. - */ - result->output_truncated = true; - - /* Write as much of the full message as possible */ - uint32_t space_remaining = s2n_stuffer_space_remaining(&result->output); - RESULT_GUARD_POSIX(s2n_stuffer_vnprintf(&result->output, space_remaining, format, vargs)); - - /* Overwrite the end of the message to indicate the truncation. - * We don't want readers to think they have the whole message. - */ - uint32_t to_rewrite = MIN(s2n_stuffer_data_available(&result->output), TRUNCATED_STR_LEN); - RESULT_GUARD_POSIX(s2n_stuffer_wipe_n(&result->output, to_rewrite)); - RESULT_GUARD_POSIX(s2n_stuffer_write_bytes(&result->output, (const uint8_t *) TRUNCATED_STR, to_rewrite)); - } - - /* The stuffer_printf methods always allow space for one extra char due to - * the '\0' written by the underlying snprintf method. - * We use that extra char for a newline. - */ - RESULT_GUARD_POSIX(s2n_stuffer_write_char(&result->output, '\n')); - return S2N_RESULT_OK; -} - static S2N_RESULT s2n_security_rule_result_process(struct s2n_security_rule_result *result, bool condition, const char *format, ...) { @@ -68,10 +31,14 @@ static S2N_RESULT s2n_security_rule_result_process(struct s2n_security_rule_resu } result->found_error = true; + if (!result->write_output) { + return S2N_RESULT_OK; + } + va_list vargs; va_start(vargs, format); - /* A failure writing the output shouldn't cause the entire validation to fail */ - s2n_result_ignore(s2n_security_rule_result_write_output(result, condition, format, vargs)); + RESULT_GUARD_POSIX(s2n_stuffer_vprintf(&result->output, format, vargs)); + RESULT_GUARD_POSIX(s2n_stuffer_write_char(&result->output, '\n')); va_end(vargs); return S2N_RESULT_OK; } @@ -220,10 +187,21 @@ S2N_RESULT s2n_security_policy_validate_security_rules( return S2N_RESULT_OK; } +S2N_RESULT s2n_security_rule_result_init_output(struct s2n_security_rule_result *result) +{ + /* For the expected, happy case, the rule isn't violated, so nothing is written + * to the stuffer, so no memory is allocated. + */ + RESULT_GUARD_POSIX(s2n_stuffer_growable_alloc(&result->output, 0)); + result->write_output = true; + return S2N_RESULT_OK; +} + S2N_CLEANUP_RESULT s2n_security_rule_result_free(struct s2n_security_rule_result *result) { if (result) { RESULT_GUARD_POSIX(s2n_stuffer_free(&result->output)); } + *result = (struct s2n_security_rule_result) { 0 }; return S2N_RESULT_OK; } diff --git a/tls/s2n_security_rules.h b/tls/s2n_security_rules.h index 384ec892667..75c75abea4a 100644 --- a/tls/s2n_security_rules.h +++ b/tls/s2n_security_rules.h @@ -30,9 +30,10 @@ typedef enum { struct s2n_security_rule_result { bool found_error; - bool output_truncated; + bool write_output; struct s2n_stuffer output; }; +S2N_RESULT s2n_security_rule_result_init_output(struct s2n_security_rule_result *result); S2N_CLEANUP_RESULT s2n_security_rule_result_free(struct s2n_security_rule_result *result); struct s2n_security_policy; From 4e09fce9b8b9f239ef49a8befd7ae739791d776c Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Mon, 27 Nov 2023 00:47:26 -0800 Subject: [PATCH 3/5] Add comment about tainted --- stuffer/s2n_stuffer_text.c | 7 +++++++ tls/s2n_security_rules.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index c5443417b22..1e876befbd8 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -242,6 +242,12 @@ int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list POSIX_ENSURE_GTE(str_len, 0); int mem_size = str_len + 1; + /* 'tainted' indicates that pointers to the contents of the stuffer exist, + * so resizing / reallocated the stuffer will invalidate those pointers. + * However, we do no resize the stuffer in this method after creating `str` + * and `str` does not live beyond this method, so ignore `str` for the + * purposes of tracking 'tainted'. + */ bool previously_tainted = stuffer->tainted; char *str = s2n_stuffer_raw_write(stuffer, mem_size); stuffer->tainted = previously_tainted; @@ -258,6 +264,7 @@ int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list /* We don't actually use c-strings, so erase the final '\0' */ POSIX_GUARD(s2n_stuffer_wipe_n(stuffer, 1)); + POSIX_POSTCONDITION(s2n_stuffer_validate(stuffer)); return S2N_SUCCESS; } diff --git a/tls/s2n_security_rules.c b/tls/s2n_security_rules.c index c8b466b1a29..c9a0cf71787 100644 --- a/tls/s2n_security_rules.c +++ b/tls/s2n_security_rules.c @@ -202,6 +202,6 @@ S2N_CLEANUP_RESULT s2n_security_rule_result_free(struct s2n_security_rule_result if (result) { RESULT_GUARD_POSIX(s2n_stuffer_free(&result->output)); } - *result = (struct s2n_security_rule_result) { 0 }; + *result = (struct s2n_security_rule_result){ 0 }; return S2N_RESULT_OK; } From 0f5684cbc6fb03cdbe26a872fd54f0d2a72fa3b7 Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Mon, 27 Nov 2023 16:17:57 -0800 Subject: [PATCH 4/5] Make cppcheck happy --- tls/s2n_security_rules.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tls/s2n_security_rules.c b/tls/s2n_security_rules.c index c9a0cf71787..e2ed43029d7 100644 --- a/tls/s2n_security_rules.c +++ b/tls/s2n_security_rules.c @@ -37,9 +37,10 @@ static S2N_RESULT s2n_security_rule_result_process(struct s2n_security_rule_resu va_list vargs; va_start(vargs, format); - RESULT_GUARD_POSIX(s2n_stuffer_vprintf(&result->output, format, vargs)); - RESULT_GUARD_POSIX(s2n_stuffer_write_char(&result->output, '\n')); + int ret = s2n_stuffer_vprintf(&result->output, format, vargs); va_end(vargs); + RESULT_GUARD_POSIX(ret); + RESULT_GUARD_POSIX(s2n_stuffer_write_char(&result->output, '\n')); return S2N_RESULT_OK; } From abd35bb54cd467062cebf5714d65dfb1ddb0b4ae Mon Sep 17 00:00:00 2001 From: Lindsay Stewart Date: Fri, 1 Dec 2023 16:38:52 -0800 Subject: [PATCH 5/5] Update va_list struct comment --- stuffer/s2n_stuffer_text.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/stuffer/s2n_stuffer_text.c b/stuffer/s2n_stuffer_text.c index 1e876befbd8..6a95c9d8f3d 100644 --- a/stuffer/s2n_stuffer_text.c +++ b/stuffer/s2n_stuffer_text.c @@ -210,8 +210,10 @@ int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data, /* If we call va_start or va_copy there MUST be a matching call to va_end, * so we should use DEFER_CLEANUP with our va_lists. - * Unfortunately, some environments implement va_list in unexpected ways - * (for example, as an array). + * Unfortunately, some environments implement va_list in ways that don't + * act as expected when passed by reference. For example, because va_end is + * a macro it may expect va_list to be an array (maybe to call sizeof), + * but passing va_list by reference will cause it to decay to a pointer instead. * To avoid any surprises, just wrap the va_list in our own struct. */ struct s2n_va_list {