diff options
author | Franziskus Kiefer <franziskuskiefer@gmail.com> | 2018-10-31 14:15:59 +0100 |
---|---|---|
committer | Franziskus Kiefer <franziskuskiefer@gmail.com> | 2018-10-31 14:15:59 +0100 |
commit | b50854e3b26193d13bbbcacd1be560d02b2d7ba7 (patch) | |
tree | e5e5e83b6dde700df4452c871d7771dd6761dcc4 | |
parent | d2f574462f473e7efcf7865cbac767e3c84549a1 (diff) | |
download | nss-hg-b50854e3b26193d13bbbcacd1be560d02b2d7ba7.tar.gz |
Bug 1485864 - improve padding checks in RSA_DecryptBlock, r=mt
Differential Revision: https://phabricator.services.mozilla.com//D10357
-rw-r--r-- | gtests/freebl_gtest/rsa_unittest.cc | 48 | ||||
-rw-r--r-- | lib/freebl/rsapkcs.c | 68 |
2 files changed, 80 insertions, 36 deletions
diff --git a/gtests/freebl_gtest/rsa_unittest.cc b/gtests/freebl_gtest/rsa_unittest.cc index 5c667a1d1..a1453168f 100644 --- a/gtests/freebl_gtest/rsa_unittest.cc +++ b/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,24 +34,24 @@ 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); @@ -59,3 +59,39 @@ TEST_F(RSANewKeyTest, expThreeTest) { 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)); +} diff --git a/lib/freebl/rsapkcs.c b/lib/freebl/rsapkcs.c index ad18c8b73..875e4e28d 100644 --- a/lib/freebl/rsapkcs.c +++ b/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; } /* |