Don't loop forever in BN_mod_sqrt on invalid inputs.

BN_mod_sqrt implements the Tonelli–Shanks algorithm, which requires a
prime modulus. It was written such that, given a composite modulus, it
would sometimes loop forever. This change fixes the algorithm to always
terminate. However, callers must still pass a prime modulus for the
function to have a defined output.

In OpenSSL, this loop resulted in a DoS vulnerability, CVE-2022-0778.
BoringSSL is mostly unaffected by this. In particular, this case is not
reachable in BoringSSL from certificate and other ASN.1 elliptic curve
parsing code. Any impact in BoringSSL is limited to:

- Callers of EC_GROUP_new_curve_GFp that take untrusted curve parameters
- Callers of BN_mod_sqrt that take untrusted moduli

This CL updates documentation of those functions to clarify that callers
should not pass attacker-controlled values. Even with the infinite loop
fixed, doing so breaks preconditions and will give undefined output.

Change-Id: I64dc1220aaaaafedba02d2ac0e4232a3a0648160
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51925
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Adam Langley <agl@google.com>

crypto/fipsmodule/bn/bn_test.cc

@@ -2712,6 +2712,22 @@ TEST_F(BNTest, WriteIntoNegative) {
   EXPECT_FALSE(BN_is_negative(r.get()));
 }
 
+TEST_F(BNTest, ModSqrtInvalid) {
+  bssl::UniquePtr<BIGNUM> bn2140141 = ASCIIToBIGNUM("2140141");
+  ASSERT_TRUE(bn2140141);
+  bssl::UniquePtr<BIGNUM> bn2140142 = ASCIIToBIGNUM("2140142");
+  ASSERT_TRUE(bn2140142);
+  bssl::UniquePtr<BIGNUM> bn4588033 = ASCIIToBIGNUM("4588033");
+  ASSERT_TRUE(bn4588033);
+
+  // |BN_mod_sqrt| may fail or return an arbitrary value, so we do not use
+  // |TestModSqrt| or |TestNotModSquare|. We only promise it will not crash or
+  // infinite loop. (For some invalid inputs, it may even be non-deterministic.)
+  // See CVE-2022-0778.
+  BN_free(BN_mod_sqrt(nullptr, bn2140141.get(), bn4588033.get(), ctx()));
+  BN_free(BN_mod_sqrt(nullptr, bn2140142.get(), bn4588033.get(), ctx()));
+}
+
 #if defined(OPENSSL_BN_ASM_MONT) && defined(SUPPORTS_ABI_TEST)
 TEST_F(BNTest, BNMulMontABI) {
   for (size_t words : {4, 5, 6, 7, 8, 16, 32}) {

crypto/fipsmodule/bn/sqrt.c

@@ -306,8 +306,7 @@ BIGNUM *BN_mod_sqrt(BIGNUM *in, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx) {
   }
 
   // x := a^((q-1)/2)
-  if (BN_is_zero(t))  // special case: p = 2^e + 1
-  {
+  if (BN_is_zero(t)) {  // special case: p = 2^e + 1
     if (!BN_nnmod(t, A, p, ctx)) {
       goto end;
     }
@@ -350,7 +349,6 @@ BIGNUM *BN_mod_sqrt(BIGNUM *in, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx) {
     // We have  a*b = x^2,
     //    y^2^(e-1) = -1,
     //    b^2^(e-1) = 1.
-
     if (BN_is_one(b)) {
       if (!BN_copy(ret, x)) {
         goto end;
@@ -359,23 +357,26 @@ BIGNUM *BN_mod_sqrt(BIGNUM *in, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx) {
       goto vrfy;
     }
 
-
-    // find smallest  i  such that  b^(2^i) = 1
-    i = 1;
-    if (!BN_mod_sqr(t, b, p, ctx)) {
-      goto end;
-    }
-    while (!BN_is_one(t)) {
-      i++;
-      if (i == e) {
-        OPENSSL_PUT_ERROR(BN, BN_R_NOT_A_SQUARE);
-        goto end;
+    // Find the smallest i, 0 < i < e, such that b^(2^i) = 1
+    for (i = 1; i < e; i++) {
+      if (i == 1) {
+        if (!BN_mod_sqr(t, b, p, ctx)) {
+          goto end;
+        }
+      } else {
+        if (!BN_mod_mul(t, t, t, p, ctx)) {
+          goto end;
+        }
       }
-      if (!BN_mod_mul(t, t, t, p, ctx)) {
-        goto end;
+      if (BN_is_one(t)) {
+        break;
       }
     }
-
+    // If not found, a is not a square or p is not a prime.
+    if (i >= e) {
+      OPENSSL_PUT_ERROR(BN, BN_R_NOT_A_SQUARE);
+      goto end;
+    }
 
     // t := y^2^(e - i - 1)
     if (!BN_copy(t, y)) {
@@ -391,14 +392,15 @@ BIGNUM *BN_mod_sqrt(BIGNUM *in, const BIGNUM *a, const BIGNUM *p, BN_CTX *ctx) {
         !BN_mod_mul(b, b, y, p, ctx)) {
       goto end;
     }
+
+    // e decreases each iteration, so this loop will terminate.
+    assert(i < e);
     e = i;
   }
 
 vrfy:
   if (!err) {
-    // verify the result -- the input might have been not a square
-    // (test added in 0.9.8)
-
+    // Verify the result. The input might have been not a square.
     if (!BN_mod_sqr(x, ret, p, ctx)) {
       err = 1;
     }

include/openssl/bn.h

@@ -584,9 +584,14 @@ OPENSSL_EXPORT int BN_mod_lshift1_quick(BIGNUM *r, const BIGNUM *a,
                                         const BIGNUM *m);
 
 // BN_mod_sqrt returns a newly-allocated |BIGNUM|, r, such that
-// r^2 == a (mod p). |p| must be a prime. It returns NULL on error or if |a| is
-// not a square mod |p|. In the latter case, it will add |BN_R_NOT_A_SQUARE| to
-// the error queue.
+// r^2 == a (mod p). It returns NULL on error or if |a| is not a square mod |p|.
+// In the latter case, it will add |BN_R_NOT_A_SQUARE| to the error queue.
+// If |a| is a square and |p| > 2, there are two possible square roots. This
+// function may return either and may even select one non-deterministically.
+//
+// This function only works if |p| is a prime. If |p| is composite, it may fail
+// or return an arbitrary value. Callers should not pass attacker-controlled
+// values of |p|.
 OPENSSL_EXPORT BIGNUM *BN_mod_sqrt(BIGNUM *in, const BIGNUM *a, const BIGNUM *p,
                                    BN_CTX *ctx);
 

include/openssl/ec.h

@@ -323,7 +323,15 @@ OPENSSL_EXPORT int EC_POINT_mul(const EC_GROUP *group, EC_POINT *r,
 // |EC_GROUP_cmp| (even to themselves). |EC_GROUP_get_curve_name| will always
 // return |NID_undef|.
 //
-// Avoid using arbitrary curves and use |EC_GROUP_new_by_curve_name| instead.
+// This function is provided for compatibility with some legacy applications
+// only. Avoid using arbitrary curves and use |EC_GROUP_new_by_curve_name|
+// instead. This ensures the result meets preconditions necessary for
+// elliptic curve algorithms to function correctly and securely.
+//
+// Given invalid parameters, this function may fail or it may return an
+// |EC_GROUP| which breaks these preconditions. Subsequent operations may then
+// return arbitrary, incorrect values. Callers should not pass
+// attacker-controlled values to this function.
 OPENSSL_EXPORT EC_GROUP *EC_GROUP_new_curve_GFp(const BIGNUM *p,
                                                 const BIGNUM *a,
                                                 const BIGNUM *b, BN_CTX *ctx);