Sending an empty certificate to s2n causes a client crash

[colmmacc]

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);
     }