Sending an empty certificate to s2n causes a client crash #71
Description
If a server vends a 0-length certificate list, towards s2n as a client, it's possible to crash s2n when used as a client. Important note: in s2n, client mode is disabled and this issue is not trigger-able.
The crash occurs inside OpenSSL's RSA_size:
#0 0xb7e9c397 in RSA_size () from /root/s2n/s2n-master/lib/libs2n.so
#1 0xb7e84df9 in s2n_rsa_public_encrypted_size () from /root/s2n/s2n-master/lib/libs2n.so
#2 0xb7e788ef in s2n_client_key_send () from /root/s2n/s2n-master/lib/libs2n.so
#3 0xb7e7ac9e in s2n_negotiate () from /root/s2n/s2n-master/lib/libs2n.so
#4 0x08049b44 in echo ()
#5 0x080494d2 in main ()
Per https://github.com/openssl/openssl/blob/35a1cc90bc1795e8893c11e442790ee7f659fffb/crypto/rsa/rsa_crpt.c#L69 RSA_size() is referencing rsa->n, a bignum which in this case is empty.
What's happening is that we're never calling s2n_asn1der_to_rsa_public_key(), because size_of_all_certificates is 0, and so the inner while loop is never exercised. See https://github.com/awslabs/s2n/blob/e17fd1a4370ec96830cade867879fa07655130c8/tls/s2n_server_cert.c#L58
Issue reported by Mikko from Codenomicon.
The following patch fixes the issue and adds some additional checks in similar code-paths (though none are similarly vulnerable).
diff --git a/crypto/s2n_rsa.c b/crypto/s2n_rsa.c
index 34e2bd7..373bed1 100644
--- a/crypto/s2n_rsa.c
+++ b/crypto/s2n_rsa.c
@@ -95,11 +95,17 @@ int s2n_rsa_private_key_free(struct s2n_rsa_private_key *key)
int s2n_rsa_public_encrypted_size(struct s2n_rsa_public_key *key)
{
+ notnull_check(key->rsa);
+ notnull_check(key->rsa->n);
+
return RSA_size(key->rsa);
}
int s2n_rsa_private_encrypted_size(struct s2n_rsa_private_key *key)
{
+ notnull_check(key->rsa);
+ notnull_check(key->rsa->n);
+
return RSA_size(key->rsa);
}
diff --git a/tls/s2n_client_key_exchange.c b/tls/s2n_client_key_exchange.c
index 6ce1363..a979c51 100644
--- a/tls/s2n_client_key_exchange.c
+++ b/tls/s2n_client_key_exchange.c
@@ -56,6 +56,8 @@ static int s2n_rsa_client_key_recv(struct s2n_connection *conn)
encrypted.size = s2n_stuffer_data_available(in);
encrypted.data = s2n_stuffer_raw_read(in, length);
+ gt_check(encrypted.size, 0);
+
/* Set rsa_failed to 1 if s2n_rsa_decrypt returns anything other than zero */
conn->handshake.rsa_failed = !!s2n_rsa_decrypt(&conn->config->cert_and_key_pairs->private_key, &encrypted, &pms);
diff --git a/tls/s2n_server_cert.c b/tls/s2n_server_cert.c
index 6d1c3fb..ac14713 100644
--- a/tls/s2n_server_cert.c
+++ b/tls/s2n_server_cert.c
@@ -32,17 +32,17 @@ int s2n_server_cert_recv(struct s2n_connection *conn)
GUARD(s2n_stuffer_read_uint24(&conn->handshake.io, &size_of_all_certificates));
- if (size_of_all_certificates > s2n_stuffer_data_available(&conn->handshake.io)) {
+ if (size_of_all_certificates > s2n_stuffer_data_available(&conn->handshake.io) || size_of_all_certificates < 3) {
S2N_ERROR(S2N_ERR_BAD_MESSAGE);
}
- int certificate = 0;
+ int certificate_count = 0;
while (s2n_stuffer_data_available(&conn->handshake.io)) {
uint32_t certificate_size;
GUARD(s2n_stuffer_read_uint24(&conn->handshake.io, &certificate_size));
- if (certificate_size > s2n_stuffer_data_available(&conn->handshake.io)) {
+ if (certificate_size > s2n_stuffer_data_available(&conn->handshake.io) || certificate_size == 0) {
S2N_ERROR(S2N_ERR_BAD_MESSAGE);
}
@@ -52,15 +52,18 @@ int s2n_server_cert_recv(struct s2n_connection *conn)
notnull_check(asn1cert.data);
/* TODO: certificate validation goes here */
+ gt_check(certificate_size, 0);
/* Pull the public key from the first certificate */
- if (certificate == 0) {
+ if (certificate_count == 0) {
GUARD(s2n_asn1der_to_rsa_public_key(&conn->pending.server_rsa_public_key, &asn1cert));
}
- certificate++;
+ certificate_count++;
}
+ gte_check(certificate_count, 1);
+
conn->handshake.next_state = SERVER_HELLO_DONE;
if (conn->status_type == S2N_STATUS_REQUEST_OCSP) {
diff --git a/tls/s2n_server_key_exchange.c b/tls/s2n_server_key_exchange.c
index 330ab2f..ec25dcd 100644
--- a/tls/s2n_server_key_exchange.c
+++ b/tls/s2n_server_key_exchange.c
@@ -102,6 +102,8 @@ static int s2n_ecdhe_server_key_recv(struct s2n_connection *conn)
signature.data = s2n_stuffer_raw_read(in, signature.size);
notnull_check(signature.data);
+ gt_check(signature_length, 0);
+
if (s2n_rsa_verify(&conn->pending.server_rsa_public_key, &signature_hash, &signature) < 0) {
S2N_ERROR(S2N_ERR_BAD_MESSAGE);
}
@@ -191,6 +193,8 @@ static int s2n_dhe_server_key_recv(struct s2n_connection *conn)
signature.data = s2n_stuffer_raw_read(in, signature.size);
notnull_check(signature.data);
+ gt_check(signature_length, 0);
+
if (s2n_rsa_verify(&conn->pending.server_rsa_public_key, &signature_hash, &signature) < 0) {
S2N_ERROR(S2N_ERR_BAD_MESSAGE);
}
Metadata
Metadata
Assignees
Labels
No labels