Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: apply cert signature preferences locally #4407

Merged
merged 22 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions crypto/s2n_certificate.c
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ int s2n_cert_chain_and_key_load(struct s2n_cert_chain_and_key *chain_and_key)

DEFER_CLEANUP(X509 *leaf_cert = NULL, X509_free_pointer);
POSIX_GUARD_RESULT(s2n_openssl_x509_parse(&head->raw, &leaf_cert));
POSIX_GUARD_RESULT(s2n_openssl_x509_get_cert_info(leaf_cert, &head->info));

/* Parse the leaf cert for the public key and certificate type */
DEFER_CLEANUP(struct s2n_pkey public_key = { 0 }, s2n_pkey_free);
Expand All @@ -383,6 +384,16 @@ int s2n_cert_chain_and_key_load(struct s2n_cert_chain_and_key *chain_and_key)
head->ec_curve_nid = nid;
}

/* populate libcrypto nid's required for cert restrictions */
struct s2n_cert *current = head->next;
while (current != NULL) {
DEFER_CLEANUP(X509 *parsed_cert = NULL, X509_free_pointer);
POSIX_GUARD_RESULT(s2n_openssl_x509_parse(&current->raw, &parsed_cert));
POSIX_GUARD_RESULT(s2n_openssl_x509_get_cert_info(parsed_cert, &current->info));

current = current->next;
}

return S2N_SUCCESS;
}

Expand Down
1 change: 1 addition & 0 deletions crypto/s2n_certificate.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ struct s2n_cert {
s2n_pkey_type pkey_type;
uint16_t ec_curve_nid;
s2n_cert_public_key public_key;
struct s2n_cert_info info;
maddeleine marked this conversation as resolved.
Show resolved Hide resolved
struct s2n_blob raw;
struct s2n_cert *next;
};
Expand Down
1 change: 1 addition & 0 deletions error/s2n_errno.c
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ static const char *no_such_error = "Internal s2n error";
ERR_ENTRY(S2N_ERR_CERT_INVALID, "Certificate is invalid") \
ERR_ENTRY(S2N_ERR_CERT_MAX_CHAIN_DEPTH_EXCEEDED, "The maximum certificate chain depth has been exceeded") \
ERR_ENTRY(S2N_ERR_CERT_REJECTED, "Certificate failed custom application validation") \
ERR_ENTRY(S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT, "Incompatibility found between loaded certificates and chosen security policy") \
ERR_ENTRY(S2N_ERR_CRL_LOOKUP_FAILED, "No CRL could be found for the corresponding certificate") \
ERR_ENTRY(S2N_ERR_CRL_SIGNATURE, "The signature of the CRL is invalid") \
ERR_ENTRY(S2N_ERR_CRL_ISSUER, "Unable to get the CRL issuer certificate") \
Expand Down
1 change: 1 addition & 0 deletions error/s2n_errno.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ typedef enum {
S2N_ERR_KTLS_RENEG,
S2N_ERR_ATOMIC,
S2N_ERR_KTLS_KEY_LIMIT,
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT,
S2N_ERR_T_USAGE_END,
} s2n_error;

Expand Down
28 changes: 28 additions & 0 deletions tests/unit/s2n_cert_chain_and_key_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,34 @@ int main(int argc, char **argv)
};
};

/* s2n_cert_chain_and_key_load_pem */
{
/* when loading a chain, all certs have a info associated with them and root is self-signed */
{
DEFER_CLEANUP(struct s2n_cert_chain_and_key *chain = NULL,
s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&chain, "ec", "ecdsa",
"p384", "sha256"));
struct s2n_cert *leaf = chain->cert_chain->head;
EXPECT_EQUAL(leaf->info.self_signed, false);
EXPECT_EQUAL(leaf->info.signature_nid, NID_ecdsa_with_SHA256);
EXPECT_EQUAL(leaf->info.signature_digest_nid, NID_sha256);

struct s2n_cert *intermediate = leaf->next;
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_NOT_NULL(intermediate);
EXPECT_EQUAL(intermediate->info.self_signed, false);
EXPECT_EQUAL(intermediate->info.signature_nid, NID_ecdsa_with_SHA256);
EXPECT_EQUAL(intermediate->info.signature_digest_nid, NID_sha256);

struct s2n_cert *root = intermediate->next;
EXPECT_NOT_NULL(intermediate);
EXPECT_NULL(root->next);
EXPECT_EQUAL(root->info.self_signed, true);
EXPECT_EQUAL(root->info.signature_nid, NID_ecdsa_with_SHA256);
EXPECT_EQUAL(root->info.signature_digest_nid, NID_sha256);
};
};

EXPECT_SUCCESS(s2n_io_pair_close(&io_pair));
EXPECT_SUCCESS(s2n_config_free(client_config));

Expand Down
87 changes: 87 additions & 0 deletions tests/unit/s2n_config_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,13 @@
#include "tls/s2n_security_policies.h"
#include "tls/s2n_tls13.h"
#include "unstable/npn.h"
#include "utils/s2n_map.h"

#define S2N_TEST_MAX_SUPPORTED_GROUPS_COUNT 30

/* forward declaration */
int s2n_config_build_domain_name_to_cert_map(struct s2n_config *config, struct s2n_cert_chain_and_key *cert_key_pair);

static int s2n_test_select_psk_identity_callback(struct s2n_connection *conn, void *context,
struct s2n_offered_psk_list *psk_identity_list)
{
Expand Down Expand Up @@ -259,6 +263,23 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_connection_free(conn));
EXPECT_SUCCESS(s2n_config_free(config));
};

/* Test that security policy validation is enforced on the config */
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);

DEFER_CLEANUP(struct s2n_cert_chain_and_key *invalid_cert = NULL, s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&invalid_cert, "rsae", "pss", "4096", "sha384"));
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, invalid_cert));
struct s2n_security_policy rfc9151_applied_locally = security_policy_rfc9151;
rfc9151_applied_locally.certificate_preferences_apply_locally = true;
config->security_policy = &rfc9151_applied_locally;

EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_config(conn, config), S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};
};

/* s2n_config_set_session_tickets_onoff */
Expand Down Expand Up @@ -1086,5 +1107,71 @@ int main(int argc, char **argv)
}
}

/* Test s2n_config_validate_loaded_certificates */
{
DEFER_CLEANUP(struct s2n_cert_chain_and_key *invalid_cert = NULL, s2n_cert_chain_and_key_ptr_free);
DEFER_CLEANUP(struct s2n_cert_chain_and_key *valid_cert = NULL, s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&invalid_cert, "ec", "ecdsa", "p384", "sha256"));
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&valid_cert, "ec", "ecdsa", "p384", "sha384"));

struct s2n_security_policy rfc9151_applied_locally = security_policy_rfc9151;
rfc9151_applied_locally.certificate_preferences_apply_locally = true;

/* rfc9151 doesn't allow SHA256 signatures, but does allow SHA384 signatures,
* so ecdsa_p384_sha256 is invalid and ecdsa_p384_sha384 is valid */

/* valid certs are accepted */
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, valid_cert));
EXPECT_OK(s2n_config_validate_loaded_certificates(config, &security_policy_rfc9151));
};

/* when cert preferences don't apply locally, invalid certs are accepted */
{
struct s2n_security_policy non_local_rfc9151 = security_policy_rfc9151;
non_local_rfc9151.certificate_preferences_apply_locally = false;

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, invalid_cert));
EXPECT_OK(s2n_config_validate_loaded_certificates(config, &non_local_rfc9151));
};

/* Certs in an s2n_config are stored in default_certs_by_type, domain_name_to_cert_map, or
* both. We want to ensure that the s2n_config_validate_loaded_certificates method will
* validate certs in both locations.
*/

/* certs in default_certs_by_type are validated */
{
/* s2n_config_set_cert_chain_and_key_defaults populates default_certs_by_type
* but doesn't populate domain_name_to_cert_map
*/
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_set_cert_chain_and_key_defaults(config, &invalid_cert, 1));

/* domain certs is empty */
uint32_t domain_certs_count = 0;
EXPECT_OK(s2n_map_size(config->domain_name_to_cert_map, &domain_certs_count));
EXPECT_EQUAL(domain_certs_count, 0);

/* certs in default_certs_by_type are validated */
EXPECT_ERROR_WITH_ERRNO(s2n_config_validate_loaded_certificates(config, &rfc9151_applied_locally),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};

/* certs in the domain map are validated */
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_SUCCESS(s2n_config_build_domain_name_to_cert_map(config, invalid_cert));

/* default_certs_by_type is empty. */
EXPECT_EQUAL(s2n_config_get_num_default_certs(config), 0);

/* certs in domain_map are validated */
EXPECT_ERROR_WITH_ERRNO(s2n_config_validate_loaded_certificates(config, &rfc9151_applied_locally),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};
};
END_TEST();
}
4 changes: 4 additions & 0 deletions tests/unit/s2n_connection_preferences_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,10 @@ int main(int argc, char **argv)
EXPECT_FAILURE_WITH_ERRNO(s2n_connection_get_ecc_preferences(conn, &ecc_preferences), S2N_ERR_INVALID_ECC_PREFERENCES);

EXPECT_SUCCESS(s2n_connection_free(conn));

/* The static configs were mutated. Reset them to allow following unit tests to use them. */
goatgoose marked this conversation as resolved.
Show resolved Hide resolved
s2n_wipe_static_configs();
EXPECT_SUCCESS(s2n_config_defaults_init());
};

/* s2n_connection_get_curve */
Expand Down
23 changes: 23 additions & 0 deletions tests/unit/s2n_connection_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,29 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_config_free(config));
};

/* Test s2n_connection_set_config */
{
/* when the config is not compliant with the security policy override, then
* s2n_connection_set_config will fail
*/
{
struct s2n_security_policy rfc9151_applied_locally = security_policy_rfc9151;
rfc9151_applied_locally.certificate_preferences_apply_locally = true;

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);

DEFER_CLEANUP(struct s2n_cert_chain_and_key *invalid_cert = NULL, s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&invalid_cert, "rsae", "pss", "4096", "sha384"));

EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, invalid_cert));
conn->security_policy_override = &rfc9151_applied_locally;
EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_config(conn, config), S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};
};

/* Test s2n_connection_get_wire_bytes_out */
{
DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT),
Expand Down
104 changes: 101 additions & 3 deletions tests/unit/s2n_security_policy_cert_preferences_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

#include "s2n_test.h"
#include "testlib/s2n_testlib.h"
#include "tls/s2n_security_policies.h"
#include "tls/s2n_signature_scheme.h"

Expand All @@ -33,6 +34,7 @@ int main(int argc, char **argv)

const struct s2n_security_policy test_sp = {
.certificate_signature_preferences = &test_certificate_signature_preferences,
.certificate_preferences_apply_locally = true,
};

const struct s2n_signature_scheme *const pss_sig_scheme_list[] = {
Expand Down Expand Up @@ -74,9 +76,8 @@ int main(int argc, char **argv)
.signature_nid = NID_rsassaPss,
};

EXPECT_ERROR_WITH_ERRNO(
s2n_security_policy_validate_cert_signature(&test_sp, &info),
S2N_ERR_CERT_UNTRUSTED);
EXPECT_ERROR_WITH_ERRNO(s2n_security_policy_validate_cert_signature(&test_sp, &info),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};

/* Certificates signed with an RSA PSS signature can be validated */
Expand All @@ -91,6 +92,103 @@ int main(int argc, char **argv)
};
};

/* s2n_security_policy_validate_certificate_chain */
{
int valid_sig_nid = NID_ecdsa_with_SHA256;
int valid_hash_nid = NID_sha256;

int invalid_sig_nid = NID_sha256WithRSAEncryption;
int invalid_hash_nide = NID_sha256;

struct s2n_cert_info valid = {
.self_signed = false,
.signature_nid = valid_sig_nid,
.signature_digest_nid = valid_hash_nid,
};
struct s2n_cert root = { 0 };
root.info = valid;
root.info.self_signed = true;

struct s2n_cert intermediate = { 0 };
intermediate.info = valid;
intermediate.next = &root;

struct s2n_cert leaf = { 0 };
leaf.info = valid;
leaf.next = &intermediate;

struct s2n_cert_chain cert_chain = { 0 };
cert_chain.head = &leaf;

struct s2n_cert_chain_and_key chain = { 0 };
chain.cert_chain = &cert_chain;

/* valid chain */
{
EXPECT_OK(s2n_security_policy_validate_certificate_chain(&test_sp, &chain));
};

/* an invalid root signature causes a failure */
{
struct s2n_cert_info valid_root = root.info;
root.info.signature_nid = invalid_sig_nid;
root.info.signature_digest_nid = invalid_hash_nide;
EXPECT_ERROR_WITH_ERRNO(s2n_security_policy_validate_certificate_chain(&test_sp, &chain),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
/* restore root values */
root.info = valid_root;
Comment on lines +138 to +139
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This is probably fine, but edging towards dangerous. Similar to the previous comment: tests should share as little state as possible, and what state they do share should be const if at all possible.

};

/* an invalid intermediate causes a failure */
{
intermediate.info.signature_nid = invalid_sig_nid;
intermediate.info.signature_digest_nid = invalid_hash_nide;
EXPECT_ERROR_WITH_ERRNO(s2n_security_policy_validate_certificate_chain(&test_sp, &chain),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
};

/* when certificate_preferences_apply_locally is false then validation succeeds */
{
struct s2n_security_policy test_sp_no_local = test_sp;
test_sp_no_local.certificate_preferences_apply_locally = false;
EXPECT_OK(s2n_security_policy_validate_certificate_chain(&test_sp_no_local, &chain));
};
};

/* s2n_connection_set_cipher_preferences */
{
DEFER_CLEANUP(struct s2n_cert_chain_and_key *invalid_cert = NULL, s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&invalid_cert, "rsae", "pss", "4096", "sha384"));

struct s2n_security_policy rfc9151_applied_locally = security_policy_rfc9151;
rfc9151_applied_locally.certificate_preferences_apply_locally = true;

/* s2n_connection_set_cipher_preferences looks up the security policy from the security_policy_selection table
* but none of our current security policies apply certificate preferences locally. So instead we rewrite the
* rfc9151 policy from the table to apply cert preference locally. */
for (int i = 0; security_policy_selection[i].version != NULL; i++) {
if (strcasecmp("rfc9151", security_policy_selection[i].version) == 0) {
security_policy_selection[i].security_policy = &rfc9151_applied_locally;
break;
jmayclin marked this conversation as resolved.
Show resolved Hide resolved
}
}
/* modify*/

jmayclin marked this conversation as resolved.
Show resolved Hide resolved
/* when certificate preferences apply locally and the connection contains
* an invalid config then s2n_connection_set_cipher_preferences fails
*/
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);
EXPECT_SUCCESS(s2n_config_add_cert_chain_and_key_to_store(config, invalid_cert));
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));
EXPECT_FAILURE_WITH_ERRNO(s2n_connection_set_cipher_preferences(conn, "rfc9151"),
S2N_ERR_SECURITY_POLICY_INCOMPATIBLE_CERT);
}
};

END_TEST();
return S2N_SUCCESS;
}
23 changes: 23 additions & 0 deletions tests/unit/s2n_x509_validator_certificate_signatures_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,29 @@ int main(int argc, char **argv)
X509_free(cert);
};

/* When the certificate signature algorithm is not in the preferences list, then an S2N_ERR_CERT_UNTRUSTED err
* is returned.
*/
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
config->security_policy = &test_sp;

DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_CLIENT), s2n_connection_ptr_free);
EXPECT_NOT_NULL(conn);
conn->actual_protocol_version = S2N_TLS12;
EXPECT_SUCCESS(s2n_connection_set_config(conn, config));

DEFER_CLEANUP(struct s2n_cert_chain_and_key *ecdsa_p384_sha256 = NULL,
s2n_cert_chain_and_key_ptr_free);
EXPECT_SUCCESS(s2n_test_cert_permutation_load_server_chain(&ecdsa_p384_sha256, "ec",
"ecdsa", "p384", "sha384"));
DEFER_CLEANUP(X509 *test_cert = NULL, X509_free_pointer);
EXPECT_OK(s2n_openssl_x509_parse(&ecdsa_p384_sha256->cert_chain->head->raw, &test_cert));

EXPECT_ERROR_WITH_ERRNO(s2n_x509_validator_check_cert_preferences(conn, test_cert), S2N_ERR_CERT_UNTRUSTED);
};

/* Certificate signature algorithm is in the test certificate signature preferences list but signature is SHA-1
* and TLS 1.3 has been negotiated.
*/
Expand Down
Loading
Loading