Bug 1485864 - improve padding checks in RSA_DecryptBlock, r=mt

Differential Revision: https://phabricator.services.mozilla.com//D10357

--HG--
extra : rebase_source : 800aaba2803e2ade438d8794486eaf3a441fc53d
extra : histedit_source : 6fc56b35bd97b5976582c90918ebcf441aa6f118%2C99120a289d1035717e7f171ca28e8cc0917d7d3e

gtests/freebl_gtest/rsa_unittest.cc

@@ -21,7 +21,7 @@ struct ScopedDelete {
 typedef std::unique_ptr<RSAPrivateKey, ScopedDelete<RSAPrivateKey>>
     ScopedRSAPrivateKey;
 
-class RSANewKeyTest : public ::testing::Test {
+class RSATest : public ::testing::Test {
  protected:
   RSAPrivateKey* CreateKeyWithExponent(int keySizeInBits,
                                        unsigned char publicExponent) {
@@ -34,28 +34,64 @@ class RSANewKeyTest : public ::testing::Test {
   }
 };
 
-TEST_F(RSANewKeyTest, expOneTest) {
+TEST_F(RSATest, expOneTest) {
   ScopedRSAPrivateKey key(CreateKeyWithExponent(2048, 0x01));
   ASSERT_TRUE(key == nullptr);
 }
-TEST_F(RSANewKeyTest, expTwoTest) {
+TEST_F(RSATest, expTwoTest) {
   ScopedRSAPrivateKey key(CreateKeyWithExponent(2048, 0x02));
   ASSERT_TRUE(key == nullptr);
 }
-TEST_F(RSANewKeyTest, expFourTest) {
+TEST_F(RSATest, expFourTest) {
   ScopedRSAPrivateKey key(CreateKeyWithExponent(2048, 0x04));
   ASSERT_TRUE(key == nullptr);
 }
-TEST_F(RSANewKeyTest, WrongKeysizeTest) {
+TEST_F(RSATest, WrongKeysizeTest) {
   ScopedRSAPrivateKey key(CreateKeyWithExponent(2047, 0x03));
   ASSERT_TRUE(key == nullptr);
 }
 
-TEST_F(RSANewKeyTest, expThreeTest) {
+TEST_F(RSATest, expThreeTest) {
   ScopedRSAPrivateKey key(CreateKeyWithExponent(2048, 0x03));
 #ifdef NSS_FIPS_DISABLED
   ASSERT_TRUE(key != nullptr);
 #else
   ASSERT_TRUE(key == nullptr);
 #endif
 }
+
+TEST_F(RSATest, DecryptBlockTestErrors) {
+  unsigned char pubExp[3] = {0x01, 0x00, 0x01};
+  SECItem exp = {siBuffer, pubExp, 3};
+  ScopedRSAPrivateKey key(RSA_NewKey(2048, &exp));
+  ASSERT_TRUE(key);
+  uint8_t out[10] = {0};
+  uint8_t in_small[100] = {0};
+  unsigned int outputLen = 0;
+  unsigned int maxOutputLen = sizeof(out);
+
+  // This should fail because input the same size as the modulus (256).
+  SECStatus rv = RSA_DecryptBlock(key.get(), out, &outputLen, maxOutputLen,
+                                  in_small, sizeof(in_small));
+  EXPECT_EQ(SECFailure, rv);
+
+  uint8_t in[256] = {0};
+  // This should fail because the padding checks will fail.
+  rv = RSA_DecryptBlock(key.get(), out, &outputLen, maxOutputLen, in,
+                        sizeof(in));
+  EXPECT_EQ(SECFailure, rv);
+  // outputLen should be maxOutputLen.
+  EXPECT_EQ(maxOutputLen, outputLen);
+
+  // This should fail because the padding checks will fail.
+  uint8_t out_long[260] = {0};
+  maxOutputLen = sizeof(out_long);
+  rv = RSA_DecryptBlock(key.get(), out_long, &outputLen, maxOutputLen, in,
+                        sizeof(in));
+  EXPECT_EQ(SECFailure, rv);
+  // outputLen should <= 256-11=245.
+  EXPECT_LE(outputLen, 245u);
+  // Everything over 256 must be 0 in the output.
+  uint8_t out_long_test[4] = {0};
+  EXPECT_EQ(0, memcmp(out_long_test, &out_long[256], 4));
+}

lib/freebl/rsapkcs.c

@@ -938,48 +938,56 @@ RSA_DecryptBlock(RSAPrivateKey *key,
                  const unsigned char *input,
                  unsigned int inputLen)
 {
-    SECStatus rv;
+    PRInt8 rv;
     unsigned int modulusLen = rsa_modulusLen(&key->modulus);
     unsigned int i;
-    unsigned char *buffer;
+    unsigned char *buffer = NULL;
+    unsigned int outLen = 0;
+    unsigned int copyOutLen = modulusLen - 11;
 
-    if (inputLen != modulusLen)
-        goto failure;
+    if (inputLen != modulusLen || modulusLen < 10) {
+        return SECFailure;
+    }
 
-    buffer = (unsigned char *)PORT_Alloc(modulusLen + 1);
-    if (!buffer)
-        goto failure;
+    if (copyOutLen > maxOutputLen) {
+        copyOutLen = maxOutputLen;
+    }
 
-    rv = RSA_PrivateKeyOp(key, buffer, input);
-    if (rv != SECSuccess)
-        goto loser;
+    // Allocate enough space to decrypt + copyOutLen to allow copying outLen later.
+    buffer = PORT_ZAlloc(modulusLen + 1 + copyOutLen);
+    if (!buffer) {
+        return SECFailure;
+    }
 
-    /* XXX(rsleevi): Constant time */
-    if (buffer[0] != RSA_BLOCK_FIRST_OCTET ||
-        buffer[1] != (unsigned char)RSA_BlockPublic) {
-        goto loser;
+    // rv is 0 if everything is going well and 1 if an error occurs.
+    rv = RSA_PrivateKeyOp(key, buffer, input) != SECSuccess;
+    rv |= (buffer[0] != RSA_BLOCK_FIRST_OCTET) |
+          (buffer[1] != (unsigned char)RSA_BlockPublic);
+
+    // There have to be at least 8 bytes of padding.
+    for (i = 2; i < 10; i++) {
+        rv |= buffer[i] == RSA_BLOCK_AFTER_PAD_OCTET;
     }
-    *outputLen = 0;
-    for (i = 2; i < modulusLen; i++) {
-        if (buffer[i] == RSA_BLOCK_AFTER_PAD_OCTET) {
-            *outputLen = modulusLen - i - 1;
-            break;
-        }
+
+    for (i = 10; i < modulusLen; i++) {
+        unsigned int newLen = modulusLen - i - 1;
+        unsigned int c = (buffer[i] == RSA_BLOCK_AFTER_PAD_OCTET) & (outLen == 0);
+        outLen = constantTimeCondition(c, newLen, outLen);
     }
-    if (*outputLen == 0)
-        goto loser;
-    if (*outputLen > maxOutputLen)
-        goto loser;
+    rv |= outLen == 0;
+    rv |= outLen > maxOutputLen;
 
-    PORT_Memcpy(output, buffer + modulusLen - *outputLen, *outputLen);
+    // Note that output is set even if SECFailure is returned.
+    PORT_Memcpy(output, buffer + modulusLen - outLen, copyOutLen);
+    *outputLen = constantTimeCondition(outLen > maxOutputLen, maxOutputLen,
+                                       outLen);
 
     PORT_Free(buffer);
-    return SECSuccess;
 
-loser:
-    PORT_Free(buffer);
-failure:
-    return SECFailure;
+    for (i = 1; i < sizeof(rv) * 8; i <<= 1) {
+        rv |= rv << i;
+    }
+    return (SECStatus)rv;
 }
 
 /*