From 3a322f5e4837a0c761d1a64f1bfea82a19f44e45 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Wed, 26 Oct 2016 12:45:35 -0400
Subject: [PATCH] Revise signing preferences.

We currently preferentially sign the largest hash available and
advertise such a preference for signatures we accept. We're just as
happy with SHA-256 and, all else equal, a smaller hash would be epsilon
more performant. We also currently claim, in TLS 1.3, we prefer P-384
over P-256 which is off.

Instead order SHA-256 first, next the larger SHA-2 hashes, and leave
SHA-1 at the bottom. Within a hash, order ECDSA > RSA-PSS > RSA-PKCS1.

This has the added consequence that we will preferentially pair P-256
with SHA-256 in signatures we generate instead of larger hashes that get
truncated anyway.

Change-Id: If4aee068ba6829e8c0ef7948f56e67a5213e4c50
Reviewed-on: https://boringssl-review.googlesource.com/11821
Reviewed-by: Adam Langley <agl@google.com>
---
 ssl/ssl_test.cc |  4 +--
 ssl/t1_lib.c    | 83 ++++++++++++++++++++++++++++---------------------
 2 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index cfce2d0c..1c4ba7fe 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1900,8 +1900,8 @@ static bool TestClientHello() {
       0xc0, 0x28, 0x00, 0x39, 0x00, 0x6b, 0x00, 0x9c, 0x00, 0x9d, 0x00, 0x2f,
       0x00, 0x3c, 0x00, 0x35, 0x00, 0x3d, 0x00, 0x0a, 0x01, 0x00, 0x00, 0x37,
       0xff, 0x01, 0x00, 0x01, 0x00, 0x00, 0x17, 0x00, 0x00, 0x00, 0x23, 0x00,
-      0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x08, 0x06, 0x06, 0x01, 0x08,
-      0x05, 0x05, 0x01, 0x05, 0x03, 0x08, 0x04, 0x04, 0x01, 0x04, 0x03, 0x02,
+      0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x04, 0x03, 0x08, 0x04, 0x04,
+      0x01, 0x05, 0x03, 0x08, 0x05, 0x05, 0x01, 0x08, 0x06, 0x06, 0x01, 0x02,
       0x01, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x0a, 0x00, 0x08, 0x00,
       0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18,
   };
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index c3d39a3f..c3864991 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -442,62 +442,73 @@ int tls1_check_group_id(SSL *ssl, uint16_t group_id) {
 }
 
 /* kVerifySignatureAlgorithms is the default list of accepted signature
- * algorithms for verifying. */
+ * algorithms for verifying.
+ *
+ * For now, RSA-PSS signature algorithms are not enabled on Android's system
+ * BoringSSL. Once the change in Chrome has stuck and the values are finalized,
+ * restore them. */
 static const uint16_t kVerifySignatureAlgorithms[] = {
-    /* For now, do not enable RSA-PSS signature algorithms on Android's system
-     * BoringSSL. Once TLS 1.3 is finalized and the change in Chrome has stuck,
-     * restore them. */
+    /* Prefer SHA-256 algorithms. */
+    SSL_SIGN_ECDSA_SECP256R1_SHA256,
 #if !defined(BORINGSSL_ANDROID_SYSTEM)
-    SSL_SIGN_RSA_PSS_SHA512,
+    SSL_SIGN_RSA_PSS_SHA256,
 #endif
-    SSL_SIGN_RSA_PKCS1_SHA512,
+    SSL_SIGN_RSA_PKCS1_SHA256,
+
+    /* Larger hashes are acceptable. */
+    SSL_SIGN_ECDSA_SECP384R1_SHA384,
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+    SSL_SIGN_RSA_PSS_SHA384,
+#endif
+    SSL_SIGN_RSA_PKCS1_SHA384,
+
     /* TODO(davidben): Remove this entry and SSL_CURVE_SECP521R1 from
      * kDefaultGroups. */
 #if defined(BORINGSSL_ANDROID_SYSTEM)
     SSL_SIGN_ECDSA_SECP521R1_SHA512,
 #endif
-
-#if !defined(BORINGSSL_ANDROID_SYSTEM)
-    SSL_SIGN_RSA_PSS_SHA384,
-#endif
-    SSL_SIGN_RSA_PKCS1_SHA384,
-    SSL_SIGN_ECDSA_SECP384R1_SHA384,
-
-#if !defined(BORINGSSL_ANDROID_SYSTEM)
-    SSL_SIGN_RSA_PSS_SHA256,
-#endif
-    SSL_SIGN_RSA_PKCS1_SHA256,
-    SSL_SIGN_ECDSA_SECP256R1_SHA256,
-
-    SSL_SIGN_RSA_PKCS1_SHA1,
-};
-
-/* kSignSignatureAlgorithms is the default list of supported signature
- * algorithms for signing. */
-static const uint16_t kSignSignatureAlgorithms[] = {
-    /* For now, do not enable RSA-PSS signature algorithms on Android's system
-     * BoringSSL. Once TLS 1.3 is finalized and the change in Chrome has stuck,
-     * restore them. */
 #if !defined(BORINGSSL_ANDROID_SYSTEM)
     SSL_SIGN_RSA_PSS_SHA512,
 #endif
     SSL_SIGN_RSA_PKCS1_SHA512,
-    SSL_SIGN_ECDSA_SECP521R1_SHA512,
 
-#if !defined(BORINGSSL_ANDROID_SYSTEM)
-    SSL_SIGN_RSA_PSS_SHA384,
-#endif
-    SSL_SIGN_RSA_PKCS1_SHA384,
-    SSL_SIGN_ECDSA_SECP384R1_SHA384,
+    /* For now, SHA-1 is still accepted but least preferable. */
+    SSL_SIGN_RSA_PKCS1_SHA1,
 
+};
+
+/* kSignSignatureAlgorithms is the default list of supported signature
+ * algorithms for signing.
+ *
+ * For now, RSA-PSS signature algorithms are not enabled on Android's system
+ * BoringSSL. Once the change in Chrome has stuck and the values are finalized,
+ * restore them. */
+static const uint16_t kSignSignatureAlgorithms[] = {
+    /* Prefer SHA-256 algorithms. */
+    SSL_SIGN_ECDSA_SECP256R1_SHA256,
 #if !defined(BORINGSSL_ANDROID_SYSTEM)
     SSL_SIGN_RSA_PSS_SHA256,
 #endif
     SSL_SIGN_RSA_PKCS1_SHA256,
-    SSL_SIGN_ECDSA_SECP256R1_SHA256,
 
-    SSL_SIGN_RSA_PKCS1_SHA1,
+    /* If needed, sign larger hashes.
+     *
+     * TODO(davidben): Determine which of these may be pruned. */
+    SSL_SIGN_ECDSA_SECP384R1_SHA384,
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+    SSL_SIGN_RSA_PSS_SHA384,
+#endif
+    SSL_SIGN_RSA_PKCS1_SHA384,
+
+    SSL_SIGN_ECDSA_SECP521R1_SHA512,
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+    SSL_SIGN_RSA_PSS_SHA512,
+#endif
+    SSL_SIGN_RSA_PKCS1_SHA512,
+
+    /* If the peer supports nothing else, sign with SHA-1. */
     SSL_SIGN_ECDSA_SHA1,
+    SSL_SIGN_RSA_PKCS1_SHA1,
 };
 
 size_t tls12_get_verify_sigalgs(const SSL *ssl, const uint16_t **out) {