commit f8068ce88574b36be69bd24d72290c9c3f5ceaf2
author Adam Langley <agl@google.com> Mon Dec 17 11:16:23 2018 -0800
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Mon Dec 17 21:02:58 2018 +0000
tree d37c85fd0afd0ad0eb3215bc7f855969477723d7
parent 41c10e2b5f37edce8b9f292f7f3bacb7e30e25c4

HRSS: be strict about unused bits being zero.

It's excessively complex to worry about leaving these few bits for
extensions. If we need to change things, we can spin a new curve ID in
TLS. We don't need to support two versions during the transition because
a fallback to X25519 is still fine.

Change-Id: I0a4019d5693db0f0f3a5379909d99c2e2c762560
Reviewed-on: https://boringssl-review.googlesource.com/c/33704
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>

crypto/hrss/hrss.c

diff --git a/crypto/hrss/hrss.c b/crypto/hrss/hrss.c
index dd3f979..d9ed0d9 100644
--- a/crypto/hrss/hrss.c
+++ b/crypto/hrss/hrss.c
@@ -1718,7 +1718,7 @@
   out[6] = 0xf & (p[3] >> 9);
 }
 
-static void poly_unmarshal(struct poly *out, const uint8_t in[POLY_BYTES]) {
+static int poly_unmarshal(struct poly *out, const uint8_t in[POLY_BYTES]) {
   uint16_t *p = out->v;
 
   for (size_t i = 0; i < N / 8; i++) {
@@ -1751,9 +1751,10 @@
     out->v[i] = (int16_t)(out->v[i] << 3) >> 3;
   }
 
-  // There are four unused bits at the top of the final byte. They are always
-  // marshaled as zero by this code but we allow them to take any value when
-  // parsing in order to support future extension.
+  // There are four unused bits in the last byte. We require them to be zero.
+  if ((in[6] & 0xf0) != 0) {
+    return 0;
+  }
 
   // Set the final coefficient as specifed in [HRSSNIST] 1.9.2 step 6.
   uint32_t sum = 0;
@@ -1762,6 +1763,8 @@
   }
 
   out->v[N - 1] = (uint16_t)(0u - sum);
+
+  return 1;
 }
 
 // mod3_from_modQ maps {0, 1, Q-1, 65535} -> {0, 1, 2, 2}. Note that |v| may
@@ -2156,16 +2159,15 @@
                         "HRSS shared key length incorrect");
   SHA256_Final(out_shared_key, &hash_ctx);
 
+  struct poly c;
   // If the ciphertext is publicly invalid then a random shared key is still
   // returned to simply the logic of the caller, but this path is not constant
   // time.
-  if (ciphertext_len != HRSS_CIPHERTEXT_BYTES) {
+  if (ciphertext_len != HRSS_CIPHERTEXT_BYTES ||
+      !poly_unmarshal(&c, ciphertext)) {
     return;
   }
 
-  struct poly c;
-  poly_unmarshal(&c, ciphertext);
-
   struct poly f;
   poly_from_poly3(&f, &priv->f);
 
@@ -2231,7 +2233,9 @@
 int HRSS_parse_public_key(struct HRSS_public_key *out,
                           const uint8_t in[HRSS_PUBLIC_KEY_BYTES]) {
   struct public_key *pub = public_key_from_external(out);
-  poly_unmarshal(&pub->ph, in);
+  if (!poly_unmarshal(&pub->ph, in)) {
+    return 0;
+  }
   OPENSSL_memset(&pub->ph.v[N], 0, 3 * sizeof(uint16_t));
   return 1;
 }

crypto/hrss/hrss_test.cc

diff --git a/crypto/hrss/hrss_test.cc b/crypto/hrss/hrss_test.cc
index ead717d..714cc7a 100644
--- a/crypto/hrss/hrss_test.cc
+++ b/crypto/hrss/hrss_test.cc
@@ -208,7 +208,7 @@
     for (unsigned j = 0; j < 10; j++) {
       uint8_t encap_entropy[HRSS_ENCAP_BYTES];
       RAND_bytes(encap_entropy, sizeof(encap_entropy));
-      SCOPED_TRACE(Bytes(generate_key_entropy));
+      SCOPED_TRACE(Bytes(encap_entropy));
 
       uint8_t ciphertext[HRSS_CIPHERTEXT_BYTES];
       uint8_t shared_key[HRSS_KEY_BYTES];
@@ -216,8 +216,15 @@
 
       uint8_t shared_key2[HRSS_KEY_BYTES];
       HRSS_decap(shared_key2, &pub, &priv, ciphertext, sizeof(ciphertext));
-
       EXPECT_EQ(Bytes(shared_key), Bytes(shared_key2));
+
+      uint32_t offset;
+      RAND_bytes((uint8_t*) &offset, sizeof(offset));
+      uint8_t bit;
+      RAND_bytes(&bit, sizeof(bit));
+      ciphertext[offset % sizeof(ciphertext)] ^= (1 << (bit & 7));
+      HRSS_decap(shared_key2, &pub, &priv, ciphertext, sizeof(ciphertext));
+      EXPECT_NE(Bytes(shared_key), Bytes(shared_key2));
     }
   }
 }