bn/asm/rsaz-avx2.pl: fix digit correction bug in rsaz_1024_mul_avx2.

Credit to OSS-Fuzz for finding this.

CVE-2017-3738

(Imported from upstream's 5630661aecbea5fe3c4740f5fea744a1f07a6253 and
77d75993651b63e872244a3256e37967bb3c3e9e.)

Confirmed with Intel SDE that the fix makes the test vector pass and
that, without the fix, the test vector does not. (Well, we knew the
latter already, since it was our test vector.)

(cherry-picked from 296a61d600)

Change-Id: I167aa3407ddab3b434bacbd18e099c55aa40ac4c
Reviewed-on: https://boringssl-review.googlesource.com/23884
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-on: https://boringssl-review.googlesource.com/23924

crypto/curve25519/internal.h

@@ -101,6 +101,26 @@ void x25519_ge_scalarmult_base(ge_p3 *h, const uint8_t a[32]);
 void x25519_ge_scalarmult(ge_p2 *r, const uint8_t *scalar, const ge_p3 *A);
 void x25519_sc_reduce(uint8_t *s);
 
+enum spake2_state_t {
+  spake2_state_init = 0,
+  spake2_state_msg_generated,
+  spake2_state_key_generated,
+};
+
+struct spake2_ctx_st {
+  uint8_t private_key[32];
+  uint8_t my_msg[32];
+  uint8_t password_scalar[32];
+  uint8_t password_hash[64];
+  uint8_t *my_name;
+  size_t my_name_len;
+  uint8_t *their_name;
+  size_t their_name_len;
+  enum spake2_role_t my_role;
+  enum spake2_state_t state;
+  char disable_password_scalar_hack;
+};
+
 
 #if defined(__cplusplus)
 }  // extern C

crypto/curve25519/spake25519.c

@@ -14,6 +14,7 @@
 
 #include <openssl/curve25519.h>
 
+#include <assert.h>
 #include <string.h>
 
 #include <openssl/bytestring.h>
@@ -267,25 +268,6 @@ static const uint8_t kSpakeMSmallPrecomp[15 * 2 * 32] = {
     0xa6, 0x76, 0x81, 0x28, 0xb2, 0x65, 0xe8, 0x47, 0x14, 0xc6, 0x39, 0x06,
 };
 
-enum spake2_state_t {
-  spake2_state_init = 0,
-  spake2_state_msg_generated,
-  spake2_state_key_generated,
-};
-
-struct spake2_ctx_st {
-  uint8_t private_key[32];
-  uint8_t my_msg[32];
-  uint8_t password_scalar[32];
-  uint8_t password_hash[SHA512_DIGEST_LENGTH];
-  uint8_t *my_name;
-  size_t my_name_len;
-  uint8_t *their_name;
-  size_t their_name_len;
-  enum spake2_role_t my_role;
-  enum spake2_state_t state;
-};
-
 SPAKE2_CTX *SPAKE2_CTX_new(enum spake2_role_t my_role,
                            const uint8_t *my_name, size_t my_name_len,
                            const uint8_t *their_name, size_t their_name_len) {
@@ -332,6 +314,48 @@ static void left_shift_3(uint8_t n[32]) {
   }
 }
 
+typedef union {
+  uint8_t bytes[32];
+  uint32_t words[8];
+} scalar;
+
+// kOrder is the order of the prime-order subgroup of curve25519 in
+// little-endian order.
+static const scalar kOrder = {{0xed, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58,
+                               0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14,
+                               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+                               0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10}};
+
+// scalar_cmov copies |src| to |dest| if |mask| is all ones.
+static void scalar_cmov(scalar *dest, const scalar *src, crypto_word_t mask) {
+  for (size_t i = 0; i < 8; i++) {
+    dest->words[i] =
+        constant_time_select_w(mask, src->words[i], dest->words[i]);
+  }
+}
+
+// scalar_double sets |s| to |2×s|.
+static void scalar_double(scalar *s) {
+  uint32_t carry = 0;
+
+  for (size_t i = 0; i < 8; i++) {
+    const uint32_t carry_out = s->words[i] >> 31;
+    s->words[i] = (s->words[i] << 1) | carry;
+    carry = carry_out;
+  }
+}
+
+// scalar_add sets |dest| to |dest| plus |src|.
+static void scalar_add(scalar *dest, const scalar *src) {
+  uint32_t carry = 0;
+
+  for (size_t i = 0; i < 8; i++) {
+    uint64_t tmp = ((uint64_t)dest->words[i] + src->words[i]) + carry;
+    dest->words[i] = (uint32_t)tmp;
+    carry = (uint32_t)(tmp >> 32);
+  }
+}
+
 int SPAKE2_generate_msg(SPAKE2_CTX *ctx, uint8_t *out, size_t *out_len,
                          size_t max_out_len, const uint8_t *password,
                          size_t password_len) {
@@ -359,13 +383,61 @@ int SPAKE2_generate_msg(SPAKE2_CTX *ctx, uint8_t *out, size_t *out_len,
   SHA512(password, password_len, password_tmp);
   OPENSSL_memcpy(ctx->password_hash, password_tmp, sizeof(ctx->password_hash));
   x25519_sc_reduce(password_tmp);
-  OPENSSL_memcpy(ctx->password_scalar, password_tmp, sizeof(ctx->password_scalar));
+
+  // Due to a copy-paste error, the call to |left_shift_3| was omitted after
+  // the |x25519_sc_reduce|, just above. This meant that |ctx->password_scalar|
+  // was not a multiple of eight to clear the cofactor and thus three bits of
+  // the password hash would leak. In order to fix this in a unilateral way,
+  // points of small order are added to the mask point such that it is in the
+  // prime-order subgroup. Since the ephemeral scalar is a multiple of eight,
+  // these points will cancel out when calculating the shared secret.
+  //
+  // Adding points of small order is the same as adding multiples of the prime
+  // order to the password scalar. Since that's faster, that is what is done
+  // below. The prime order (kOrder) is a large prime, thus odd, thus the LSB
+  // is one. So adding it will flip the LSB. Adding twice it will flip the next
+  // bit and so one for all the bottom three bits.
+
+  scalar password_scalar;
+  OPENSSL_memcpy(&password_scalar, password_tmp, sizeof(password_scalar));
+
+  // |password_scalar| is the result of |x25519_sc_reduce| and thus is, at
+  // most, $l-1$ (where $l$ is |kOrder|, the order of the prime-order subgroup
+  // of Ed25519). In the following, we may add $l + 2×l + 4×l$ for a max value
+  // of $8×l-1$. That is < 2**256, as required.
+
+  if (!ctx->disable_password_scalar_hack) {
+    scalar order = kOrder;
+    scalar tmp;
+
+    OPENSSL_memset(&tmp, 0, sizeof(tmp));
+    scalar_cmov(&tmp, &order,
+                constant_time_eq_w(password_scalar.bytes[0] & 1, 1));
+    scalar_add(&password_scalar, &tmp);
+
+    scalar_double(&order);
+    OPENSSL_memset(&tmp, 0, sizeof(tmp));
+    scalar_cmov(&tmp, &order,
+                constant_time_eq_w(password_scalar.bytes[0] & 2, 2));
+    scalar_add(&password_scalar, &tmp);
+
+    scalar_double(&order);
+    OPENSSL_memset(&tmp, 0, sizeof(tmp));
+    scalar_cmov(&tmp, &order,
+                constant_time_eq_w(password_scalar.bytes[0] & 4, 4));
+    scalar_add(&password_scalar, &tmp);
+
+    assert((password_scalar.bytes[0] & 7) == 0);
+  }
+
+  OPENSSL_memcpy(ctx->password_scalar, password_scalar.bytes,
+                 sizeof(ctx->password_scalar));
 
   ge_p3 mask;
   x25519_ge_scalarmult_small_precomp(&mask, ctx->password_scalar,
-                              ctx->my_role == spake2_role_alice
-                                  ? kSpakeMSmallPrecomp
-                                  : kSpakeNSmallPrecomp);
+                                     ctx->my_role == spake2_role_alice
+                                         ? kSpakeMSmallPrecomp
+                                         : kSpakeNSmallPrecomp);
 
   // P* = P + mask.
   ge_cached mask_cached;

crypto/curve25519/spake25519_test.cc

@@ -23,6 +23,7 @@
 #include <gtest/gtest.h>
 
 #include "../internal.h"
+#include "internal.h"
 
 
 // TODO(agl): add tests with fixed vectors once SPAKE2 is nailed down.
@@ -46,6 +47,13 @@ struct SPAKE2Run {
       return false;
     }
 
+    if (alice_disable_password_scalar_hack) {
+      alice->disable_password_scalar_hack = 1;
+    }
+    if (bob_disable_password_scalar_hack) {
+      bob->disable_password_scalar_hack = 1;
+    }
+
     uint8_t alice_msg[SPAKE2_MAX_MSG_SIZE];
     uint8_t bob_msg[SPAKE2_MAX_MSG_SIZE];
     size_t alice_msg_len, bob_msg_len;
@@ -90,6 +98,8 @@ struct SPAKE2Run {
   std::string bob_password = "password";
   std::pair<std::string, std::string> alice_names = {"alice", "bob"};
   std::pair<std::string, std::string> bob_names = {"bob", "alice"};
+  bool alice_disable_password_scalar_hack = false;
+  bool bob_disable_password_scalar_hack = false;
   int alice_corrupt_msg_bit = -1;
 
  private:
@@ -104,6 +114,24 @@ TEST(SPAKE25519Test, SPAKE2) {
   }
 }
 
+TEST(SPAKE25519Test, OldAlice) {
+  for (unsigned i = 0; i < 20; i++) {
+    SPAKE2Run spake2;
+    spake2.alice_disable_password_scalar_hack = true;
+    ASSERT_TRUE(spake2.Run());
+    EXPECT_TRUE(spake2.key_matches());
+  }
+}
+
+TEST(SPAKE25519Test, OldBob) {
+  for (unsigned i = 0; i < 20; i++) {
+    SPAKE2Run spake2;
+    spake2.bob_disable_password_scalar_hack = true;
+    ASSERT_TRUE(spake2.Run());
+    EXPECT_TRUE(spake2.key_matches());
+  }
+}
+
 TEST(SPAKE25519Test, WrongPassword) {
   SPAKE2Run spake2;
   spake2.bob_password = "wrong password";

crypto/fipsmodule/bn/asm/rsaz-avx2.pl

@@ -232,7 +232,7 @@ $code.=<<___;
 	vmovdqu		32*8-128($ap), $ACC8
 
 	lea	192(%rsp), $tp0			# 64+128=192
-	vpbroadcastq	.Land_mask(%rip), $AND_MASK
+	vmovdqu	.Land_mask(%rip), $AND_MASK
 	jmp	.LOOP_GRANDE_SQR_1024
 
 .align	32
@@ -1082,10 +1082,10 @@ $code.=<<___;
 	vpmuludq	32*6-128($np),$Yi,$TEMP1
 	vpaddq		$TEMP1,$ACC6,$ACC6
 	vpmuludq	32*7-128($np),$Yi,$TEMP2
-	 vpblendd	\$3, $ZERO, $ACC9, $ACC9	# correct $ACC3
+	 vpblendd	\$3, $ZERO, $ACC9, $TEMP1	# correct $ACC3
 	vpaddq		$TEMP2,$ACC7,$ACC7
 	vpmuludq	32*8-128($np),$Yi,$TEMP0
-	 vpaddq		$ACC9, $ACC3, $ACC3		# correct $ACC3
+	 vpaddq		$TEMP1, $ACC3, $ACC3		# correct $ACC3
 	vpaddq		$TEMP0,$ACC8,$ACC8
 
 	mov	%rbx, %rax
@@ -1098,7 +1098,9 @@ $code.=<<___;
 	 vmovdqu	-8+32*2-128($ap),$TEMP2
 
 	mov	$r1, %rax
+	 vpblendd	\$0xfc, $ZERO, $ACC9, $ACC9	# correct $ACC3
 	imull	$n0, %eax
+	 vpaddq		$ACC9,$ACC4,$ACC4		# correct $ACC3
 	and	\$0x1fffffff, %eax
 
 	 imulq	16-128($ap),%rbx
@@ -1334,15 +1336,12 @@ ___
 #	But as we underutilize resources, it's possible to correct in
 #	each iteration with marginal performance loss. But then, as
 #	we do it in each iteration, we can correct less digits, and
-#	avoid performance penalties completely. Also note that we
-#	correct only three digits out of four. This works because
-#	most significant digit is subjected to less additions.
+#	avoid performance penalties completely.
 
 $TEMP0 = $ACC9;
 $TEMP3 = $Bi;
 $TEMP4 = $Yi;
 $code.=<<___;
-	vpermq		\$0, $AND_MASK, $AND_MASK
 	vpaddq		(%rsp), $TEMP1, $ACC0
 
 	vpsrlq		\$29, $ACC0, $TEMP1
@@ -1790,7 +1789,7 @@ $code.=<<___;
 
 .align	64
 .Land_mask:
-	.quad	0x1fffffff,0x1fffffff,0x1fffffff,-1
+	.quad	0x1fffffff,0x1fffffff,0x1fffffff,0x1fffffff
 .Lscatter_permd:
 	.long	0,2,4,6,7,7,7,7
 .Lgather_permd:

crypto/fipsmodule/bn/bn_tests.txt

@@ -10479,6 +10479,12 @@ A =  -80000000000000000000000000000000000000000000000000000000000000000000000000
 E = 61803d4973ae68cfb2ba6770dbed70d36760fa42c01a16d1482eacf0d01adf7a917bc86ece58a73b920295c1291b90f49167ef856ecad149330e1fd49ec71392fb62d47270b53e6d4f3c8f044b80a5736753364896932abc6d872c4c5e135d1edb200597a93ceb262ff6c99079177cd10808b9ed20c8cd7352d80ac7f6963103
 M =  b5d257b2c50b050d42f0852eff5cfa2571157c500cd0bd9aa0b2ccdd89c531c9609d520eb81d928fb52b06da25dc713561aa0bd365ee56db9e62ac6787a85936990f44438363560f7af9e0c16f378e5b83f658252390d849401817624da97ec613a1b855fd901847352f434a777e4e32af0cb4033c7547fb6437d067fcd3d965
 
+# Regression test for CVE-2017-3738.
+ModExp = d360792bd8210786607817c3dda64cc38c8d0f25569597cb1f363c7919a0c3587baff01a2283edaeb04fc288ac0ab3f279b2a89ffcb452d8bdf72422a9f9780f4aa702dc964cf033149d3a339883062cab8564aebdbfac0bf68985e522c6fe545b346044690c525ca85d3f4eb3e3c25cdf541545afc84a309e9b1d7807003461
+A = ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff2020202020df
+E = 2020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020FF2020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020202020
+M = ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff2020202020ff
+
 
 # Exp tests.
 #