Support ECDSA signatures #671

alexeblee · 2018-01-07T00:22:14Z

Use the new pkey interface to support ECDSA signatures. I've also added a single cipher suite for testing.

There was some refactoring done to the signature_algorithms extension parsing. s2n_map is used to store all signature/hash algorithm pairs to be used later during server certificate selection, cipher suite selection, etc. I was looking for a set-like data structure so I added a function s2n_map_offer to ignore attempts to add duplicate keys. I'm open to other ways to store the sig/hash pairs.

For now, this just sends the only configured certificate and expects the cipher suite list to be compatible with the certificate type. The next step will be to add a function that picks the correct certificate to send based on the cipher suite and signature algorithms during client hello parsing. This might require some fancier certificate parsing during s2n_config_add_cert_chain_and_key and adding a type field to s2n_pkey.

lurkshark · 2018-01-17T09:12:47Z

tls/s2n_config.c

+int s2n_config_get_cert_type(struct s2n_config *config, s2n_cert_type *cert_type)
+{
+    notnull_check(config);
+    notnull_check(config->cert_and_key_pairs);


I had considered adding NONE types for the optional client auth change instead of just null checking. This change might make that a cleaner choice now for things like the s2n_set_signature_and_hash_algorithm stuff.

alexw91 · 2018-01-30T19:44:11Z

Can you rebase this onto the tip of master?

colmmacc

I like how this change is laid out and am excited about getting ECDSA support! some initial comments ...

colmmacc · 2018-02-01T22:22:35Z

tests/integration/s2n_handshake_test_gnutls.py

@@ -50,7 +55,7 @@ def try_gnutls_handshake(endpoint, port, priority_str, mfl_extension_test, enter

    if mfl_extension_test:
        gnutls_cmd.append("--recordsize=" + str(mfl_extension_test))
-
+    


Looks like some errant whitespace

colmmacc · 2018-02-01T22:25:47Z

tls/s2n_cipher_suites.c

@@ -148,6 +149,7 @@ struct s2n_cipher_suite s2n_rsa_with_rc4_128_md5 = /* 0x00,0x04 */ {
    .name = "RC4-MD5",
    .iana_value = { TLS_RSA_WITH_RC4_128_MD5 },
    .key_exchange_alg = &s2n_rsa,
+    .signature_alg = S2N_SIGNATURE_RSA,


Is this a bit counter-intuitive? The TLS_RSA_WITH_* cipher suites don't actually use RSA signatures, they use RSA encryption for key exchange, and then HMAC. The DHE/EDH ones do use RSA signatures though. Would these cipher suites break if this were left at null or NONE or something?

The relationship between cipher suites, certificates, and signature algorithms was the most difficult part for me to wrap my head around. I think in TLS 1.1, there was a requirement that the algorithm used to sign a certificate must be the same as the key type. I had interpreted that to mean that TLS_RSA_WITH_* cipher suites required certificates that had an RSA signature and that is sort of where this came from.

I tried to rethink what this field was modeling and where it was needed in the handshake. It is used in two places: to check that a received cert is compatible with the cipher suite, and to set a default signature/hash algorithm pair if ECDSA signatures will be used (also related to the cipher suite). I thought of renaming this to represent an authentication method rather than a signature algorithm. For some cipher suites, a signature would be used for authentication, but for the TLS_RSA_WITH_* the authentication would be implicit in the RSA key exchange. I'm very open to thoughts on this approach.

colmmacc · 2018-02-01T22:32:52Z

tls/s2n_client_extensions.c

+    GUARD(s2n_stuffer_read_uint8(in, &signature_algorithm));
+
+    if (signature_algorithm != TLS_SIGNATURE_ALGORITHM_RSA && signature_algorithm != TLS_SIGNATURE_ALGORITHM_ECDSA) {
+        S2N_ERROR(S2N_ERR_INVALID_SIGNATURE_ALGORITHM);
    }


Is this going to be too brittle? Especially in client mode ... what happens when folks go add some new signature type in the future?

colmmacc · 2018-02-01T22:33:49Z

tls/s2n_handshake_io.c

@@ -488,7 +487,7 @@ static int read_full_handshake_message(struct s2n_connection *conn, uint8_t * me

    /* We don't have the whole message, so we'll need to go again */
    GUARD(s2n_stuffer_reread(&conn->handshake.io));
-
+    


colmmacc · 2018-02-01T22:34:59Z

tls/s2n_signature_algorithms.c

+    s2n_signature_algorithm sig_alg_chosen = S2N_SIGNATURE_RSA;
+    if (cipher_suite_sig_alg == S2N_SIGNATURE_ECDSA) {
+        sig_alg_chosen = S2N_SIGNATURE_ECDSA;
+        hash_alg_chosen = S2N_HASH_SHA1;


Do we want to use SHA1 as our default? why not SHA256?

My interpretation of the RFCs were that this was the required default: https://tools.ietf.org/html/rfc5246#section-7.4.1.4.1

If the client does not send the signature_algorithms extension, the server MUST do the following: - If the negotiated key exchange algorithm is one of (RSA, DHE_RSA, DH_RSA, RSA_PSK, ECDH_RSA, ECDHE_RSA), behave as if client had sent the value {sha1,rsa}. - If the negotiated key exchange algorithm is one of (DHE_DSS, DH_DSS), behave as if the client had sent the value {sha1,dsa}. - If the negotiated key exchange algorithm is one of (ECDH_ECDSA, ECDHE_ECDSA), behave as if the client had sent value {sha1,ecdsa}. Note: this is a change from TLS 1.1 where there are no explicit rules, but as a practical matter one can assume that the peer supports MD5 and SHA-1.

colmmacc · 2018-02-01T22:35:33Z

utils/s2n_map.c

@@ -139,7 +139,7 @@ int s2n_map_add(struct s2n_map *map, struct s2n_blob *key, struct s2n_blob *valu
    }

    uint32_t slot = s2n_map_slot(map, key);
-
+    


colmmacc · 2018-02-01T22:40:52Z

utils/s2n_map.c

@@ -160,6 +160,20 @@ int s2n_map_add(struct s2n_map *map, struct s2n_blob *key, struct s2n_blob *valu
    return 0;
 }

+int s2n_map_offer(struct s2n_map *map, struct s2n_blob *key, struct s2n_blob *value)


Should we just have an s2n_map_put() that does add-or-replace insertion? The offer() way isn't something I've ever seen before, so it took me a bit to understand what it's doing and why. It favors leaving the former value if it's there, but any reason to favor that?

colmmacc · 2018-02-01T22:41:29Z

crypto/s2n_ecdsa.c

@@ -30,6 +30,14 @@
 #include "crypto/s2n_openssl.h"
 #include "crypto/s2n_pkey.h"

+int s2n_ecdsa_signature_size(const struct s2n_pkey *pkey)


Would it be worth making this even more specific and seeing s2n_ecdsa_der_signature_size() ?

colmmacc

LGTM now!

- Write out signature length after signing. - Use ECDSA cert when negotiating ECDSA ciphersuites in integration tests. - Initialize certificate keys after parsing based on cert type.

…ded for testing

…e refactoring

… FIPS mode

…d signature algorithms, and function names

colmmacc

Thanks for these insanely awesome additions! Time to merge.

raycoll · 2018-02-23T02:11:03Z

tls/s2n_signature_algorithms.c

-int s2n_set_signature_hash_pair_from_preference_list(struct s2n_connection *conn, uint8_t sig_hash_algs[TLS_SIGNATURE_ALGORITHM_COUNT][TLS_HASH_ALGORITHM_COUNT], 
+static int s2n_sig_hash_algs_pairs_set(struct s2n_sig_hash_alg_pairs *sig_hash_algs, uint8_t sig_alg, uint8_t hash_alg)
+{
+    S2N_ERROR_IF(hash_alg >= TLS_HASH_ALGORITHM_COUNT, S2N_ERR_HASH_INVALID_ALGORITHM);


If the hash_alg value is coming from peer's signature_algorithm extension, shouldn't we skip an unknown value rather than throwing an error? The peer could be sending a code point that s2n has not yet implemented. If there is no overlap between the peer's set and ours we will error later during cipher suite/cert selection.

Will follow up with a patch.

Actually, this is okay because we aren't checking the return value of s2n_sig_hash_alg_pairs_set in s2n_recv_supported_signature_algorithms. I'll validate with a little unit test.

raycoll · 2018-02-23T02:13:44Z

tls/s2n_signature_algorithms.c

+static int s2n_sig_hash_algs_pairs_set(struct s2n_sig_hash_alg_pairs *sig_hash_algs, uint8_t sig_alg, uint8_t hash_alg)
+{
+    S2N_ERROR_IF(hash_alg >= TLS_HASH_ALGORITHM_COUNT, S2N_ERR_HASH_INVALID_ALGORITHM);
+    S2N_ERROR_IF(sig_alg >= TLS_SIGNATURE_ALGORITHM_COUNT, S2N_ERR_INVALID_SIGNATURE_ALGORITHM);


alexeblee closed this Jan 7, 2018

alexeblee reopened this Jan 16, 2018

alexeblee force-pushed the ecdsa branch from 82caa3c to 27223d8 Compare January 16, 2018 01:00

lurkshark reviewed Jan 17, 2018

View reviewed changes

colmmacc suggested changes Feb 1, 2018

View reviewed changes

colmmacc previously approved these changes Feb 15, 2018

View reviewed changes

alexeblee dismissed colmmacc’s stale review via 75a4b6a February 15, 2018 20:17

alexeblee force-pushed the ecdsa branch 3 times, most recently from 1961de7 to c351a79 Compare February 22, 2018 00:28

alexeblee added 12 commits February 22, 2018 08:31

Add ECDSA suites to s2n_handshake_test

3252d7c

Add one ECDSA ciphersuite to test with

59d7199

Add client config to test ECDSA server cert validation

52c1235

Move RSA and ECDSA size functions to pkey interface

da29f02

Allow client certificate to be an ECDSA type

c478b1b

Consolidate validation checks for RSA and ECDSA keys

3f27c7c

Clean up new pkey function definitions to fix unit tests

182be9e

- Refactor signature_algorithms to include ECDSA.

34654c3

- Write out signature length after signing. - Use ECDSA cert when negotiating ECDSA ciphersuites in integration tests. - Initialize certificate keys after parsing based on cert type.

Check cert type against negotiated cipher suite and clean up code nee…

cf8d059

…ded for testing

Fix fuzz test for s2n_asn1der_to_public_key_and_type name change

23ab783

Fixing incorrect check in s2n_handshake_test that was introduced whil…

5b47f25

…e refactoring

Skip handshake tests for ciphers that are not FIPS compatible when in…

b94cc8b

… FIPS mode

alexeblee force-pushed the ecdsa branch from c351a79 to beaa6d3 Compare February 22, 2018 16:38

alexeblee added 6 commits February 22, 2018 09:42

Set sig_algs map to NULL after freeing the connection

789affe

Do not attempt to parse signature algorithms more than once

d22b54c

Remove s2n_map_offer and add s2n_map_put to replace entries in s2n_map

ba818ca

Make s2n_ecdsa_signature_size more specifically named for DER encoding

81e669b

Remove extra whitespace

e0ff3ee

Refactor based on feedback: cipher suite definition, list of supporte…

aba666d

…d signature algorithms, and function names

alexeblee added 3 commits February 22, 2018 09:44

Remove s2n_map_offer from s2n_map.h

7830432

Fix incorrect index in for loop

a74ab58

Use matrix to store sig-hash algorithm pairs used in the handshake

9875343

alexeblee force-pushed the ecdsa branch from beaa6d3 to 9875343 Compare February 22, 2018 17:53

Wrap operations for storing and retrieving sig-hash algorithm pairs

40d68dd

colmmacc approved these changes Feb 23, 2018

View reviewed changes

colmmacc merged commit 4b10257 into aws:master Feb 23, 2018

raycoll reviewed Feb 23, 2018

View reviewed changes

This was referenced Feb 23, 2018

Support ECDSA signatures #7

Closed

Create Signature Algorithm Preference List instead of hardcoding RSA #465

Closed

alexeblee deleted the ecdsa branch August 20, 2018 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ECDSA signatures #671

Support ECDSA signatures #671

alexeblee commented Jan 7, 2018 •

edited

lurkshark Jan 17, 2018

alexw91 commented Jan 30, 2018

colmmacc left a comment

colmmacc Feb 1, 2018

colmmacc Feb 1, 2018

alexeblee Feb 15, 2018

colmmacc Feb 1, 2018

colmmacc Feb 1, 2018

colmmacc Feb 1, 2018

alexeblee Feb 15, 2018

colmmacc Feb 1, 2018

colmmacc Feb 1, 2018

colmmacc Feb 1, 2018

colmmacc left a comment

colmmacc left a comment

raycoll Feb 23, 2018

raycoll Feb 23, 2018

raycoll Feb 23, 2018 •

edited

raycoll Feb 23, 2018

		@@ -50,7 +55,7 @@ def try_gnutls_handshake(endpoint, port, priority_str, mfl_extension_test, enter

		if mfl_extension_test:
		gnutls_cmd.append("--recordsize=" + str(mfl_extension_test))

		@@ -488,7 +487,7 @@ static int read_full_handshake_message(struct s2n_connection conn, uint8_t me

		/* We don't have the whole message, so we'll need to go again */
		GUARD(s2n_stuffer_reread(&conn->handshake.io));

		@@ -139,7 +139,7 @@ int s2n_map_add(struct s2n_map map, struct s2n_blob key, struct s2n_blob *valu
		}

		uint32_t slot = s2n_map_slot(map, key);

Support ECDSA signatures #671

Support ECDSA signatures #671

Conversation

alexeblee commented Jan 7, 2018 • edited

Choose a reason for hiding this comment

alexw91 commented Jan 30, 2018

colmmacc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colmmacc left a comment

Choose a reason for hiding this comment

colmmacc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raycoll Feb 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeblee commented Jan 7, 2018 •

edited

raycoll Feb 23, 2018 •

edited