1539788 - (CVE-2019-17006) Check length of inputs for cryptographic primitives

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Martin Thomson [:mt:]]

Benjamin Beurdouche pointed out that not a lot of cryptographic primitives check the length of their inputs. After looking at some of our code, we don't do that for ChaCha20Poly1305. It probably makes sense to do a bit of an audit.

Generally speaking, the limits for primitives are significantly higher than a sensible input would allow for, so this isn't a serious concern. However, having the checks is good hygiene.

Comment 1

[Kevin Jacobs [:kjacobs]]

This is straightforward for counter-mode block ciphers, but there's some ambiguity outside of that. So, I have a few questions:

1. For block ciphers in non-counter mode, should we apply any plaintext limit if one isn't in the spec? A number of them don't explicitly specify a max, and we're practically limited by the size of inLen being an unsigned int. (Of course, we can still check things like nonce, key, and tag lengths)

2. For ciphers that use big number arithmetic, we range-check and reduce mod n if needed (I do need to audit this more fully). Is any other check needed? E.g., we could check against the largest supported modulus length (8KB according to rsaperf)

3. For hash algorithms, do we want to check for input bit length <= 2^64-1? We already have the practical limit as in #1.

Comment 2

[Kevin Jacobs [:kjacobs]]

Question 1 applies to RC4 too (anything without a block counter, really).

Comment 3

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1539788 - Add length checks for cryptographic primitives

This patch adds some missing primitive length checks. A few notes (notation per rfc7539):

1. Added A_MAX check for GCM AAD (must be <= 2^64-1 bits)
   1. A_MAX for ChaCha20Poly1305 is <= 2^64-1 bytes. This is effectively unlimited since the adLen parameter is an unsigned int.
   2. SP800-38D (Appendix C) has some additional restrictions for short tag scenarios. However, the adLen limits are related to invocation count for a given key. Since we don't have this knowledge available, it's not clear that we should enforce anything here.
2. Added C_MAX check to ChaCha20Poly1305_Open()
3. Added P_MAX/C_MAX checks for AES GCM and CTR. We already error out after detecting a counter wrap, however SP800-38D requires stopping two blocks prior.
   1. This becomes a little harder to enforce w.r.t multi-part encryption (i.e. more than one AES_Update() call per-context). We could track the run length in the context, but the caller provides the whole ICV, so this isn’t perfect. Also, each call to PK11_Encrypt will set up its own context.. So there may not be a material difference between the two.
4. There's an issue with block padding [[ https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rsapkcs.c#146 | calculation ]]: padLen is a signed int, and with a sufficiently large data->len (unsigned), we can underflow and get a heap-buffer-overflow. This doesn’t appear to be exploitable from Firefox, but can be hit from PK11_Sign and PK11_PubEncrypt. For WebCrypto, the former hashes the data (length is controlled by the browser), for the latter, only OAEP is used (which is not vulnerable).
5. For other cases that where limits are not as well-defined:
   1. Hashes: No limits, but HMACs have key checks as appropriate.
   2. MPI-based algorithms (DSA/DH/RSA/EC): No checks added, input undergoes modular reduction or comparison against some upper limit (e.g. group order).
   3. Steam ciphers, block ciphers in ECB/CBC modes: No checks added, verified that necessary checks on key and IV length exist.

Comment 4

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9074309 [details]
Bug 1539788 - Add length checks for cryptographic primitives

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This patch is the result of an audit of cryptographic length checks in NSS. One routine handling PKCS1 v1.5 encoding has its input length checked only by an assert (which is not compiled into release builds). This results in potential for a heap buffer overflow from public PK11_Sign/PK11_PubEncrypt functions. The patch adds a fix and a test, in which the problem is rather obvious.

This does not appear have a direct exploit path from WebCrypto (only OAEP asymmetric encryption is exposed, not PKCS1 v1.5. For signing, the browser hashes the message first, reducing its length to a valid range). W.r.t. process separation it's less clear to me if there could be a problem for Firefox here, but this may have higher severity for other NSS consumers.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Not difficult or risky
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely (it's essentially changing an assert to an if).

Comment 5

[Al Billings [:abillings - ex-MoCo]]

JC, when is the next NSS release? If I approved this for checkin now, how long until non-Firefox customers would get this?

Comment 6

[J.C. Jones [:jcj] (he/they)]

Our release schedule mirrors Firefox's, so NSS 3.46 will be frozen on 23 August and released on 30 August.

https://wiki.mozilla.org/NSS:Release_Versions

Comment 7

[Al Billings [:abillings - ex-MoCo]]

I'm giving sec-approval+ but only for checkin on August 6, 2019, a bit further into the current cycle. We'll want to backport this to Beta, ESR68, and ESR60 at that time.

Comment 8

[J.C. Jones [:jcj] (he/they)]

After discussion with :abillings, if we need to pull check-in earlier a couple of days in order to get NSS 3.44.2 and 3.45.1 released earlier for a different security issue, that's OK, we don't need to plan a second pair of releases in in the same week.

Comment 9

[J.C. Jones [:jcj] (he/they)]

Al, releng wants to double-check that this is acceptable to roll live in that next-7-ish days point release, compared to an original schedule that would have been based around only a beta release next week.

Comment 10

[Kevin Jacobs [:kjacobs]]

Daiki, could you take a look at bullet #4 in the attached patch? This doesn't have an exploit path from Firefox, but we'd like input from your side before landing.

Specifically: Do you want a CVE for this? Can we land this into 3.46 (vs. 3.44.2 and 3.45.1)?

Thanks.

Comment 11

[J.C. Jones [:jcj] (he/they)]

Per Mozilla internal team meeting, removing from upcoming point releases to lower the point-releases' risk. Can re-add per comment 10 if needed.

Comment 12

[Daiki Ueno [:ueno]]

Sorry for late response.

(In reply to Kevin Jacobs [:kjacobs] from comment #10)

Daiki, could you take a look at bullet #4 in the attached patch? This doesn't have an exploit path from Firefox, but we'd like input from your side before landing.

The change looks good, thank you. As for the impact for us, given the raw RSA (CKM_RSA_X_509) mechanism is typically used as a fallback when appropriate mechanism is not available, I think the impact is quite limited. Let me double check with our security response.

Comment 13

[Kevin Jacobs [:kjacobs]]

Thanks. Just to be clear, CKM_RSA_PKCS will trigger the same issue and is presumably more common.

Comment 14

[J.C. Jones [:jcj] (he/they)]

Update to remove from point releases, per comment 11.

Comment 15

[Daiki Ueno [:ueno]]

(In reply to Kevin Jacobs [:kjacobs] from comment #13)

Just to be clear, CKM_RSA_PKCS will trigger the same issue and is presumably more common.

I failed to see how it could hit the same issue. Could you elaborate a little? My understanding is that the relevant code is exposed through RSA_SignRaw and RSA_EncryptRaw from freebl, and those functions are called by C_SignInit and C_EncryptInit only when the mechanism is CKM_RSA_X_509:
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#2625
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#820

Comment 16

[Kevin Jacobs [:kjacobs]]

Relevant code is exposed through RSA_Sign and RSA_Encrypt. Here's the call path for Sign:

https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#2622
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#2417
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rsapkcs.c#1307
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rsapkcs.c#241 (line 239 is not compiled in release builds)

Encrypt (as in the added test) is similar. rsa_FormatBlock's case for RSA_BlockRaw checks the length, so actually CKM_RSA_X_509 is not affected.

Comment 17

[J.C. Jones [:jcj] (he/they)]

Daiki, FYI - I'm going to land this to nss default and mozilla-central on Monday unless I hear otherwise.

Comment 18

[J.C. Jones [:jcj] (he/they)]

Since we're not going to produce an ESR uplift for this, let's split the fix out from the tests, and land the tests at a later time.

Comment 19

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/dfd6996fe7425eb0437346d11a01082f16fcfe34

Comment 20

[J.C. Jones [:jcj] (he/they)]

script failure, leave-open.

Comment 21

[J.C. Jones [:jcj] (he/they)]

ASAN failure (https://tools.taskcluster.net/task-inspector/#S0dpasvcTLiKKgUGIR3PgA):

[----------] 1 test from Pkcs11CipherOp
[ RUN      ] Pkcs11CipherOp.SingleCtxMultipleUnalignedCipherOps
../../lib/freebl/ctr.c:132:24: runtime error: shift exponent 128 is too large for 64-bit type 'unsigned long long'
    #0 0x7f41f5c5e866 in CTR_Update /home/worker/nss/out/Debug/../../lib/freebl/ctr.c:132:24
    #1 0x7f41f5d01283 in AES_Encrypt /home/worker/nss/out/Debug/../../lib/freebl/rijndael.c:1097:12
    #2 0x7f41f69daee8 in AES_Encrypt /home/worker/nss/out/Debug/../../lib/freebl/loader.c:458:12
    #3 0x7f41f695b6a6 in NSC_EncryptUpdate /home/worker/nss/out/Debug/../../lib/softoken/pkcs11c.c:1348:10
    #4 0x7f41fb24d5fc in PK11_CipherOp /home/worker/nss/out/Debug/../../lib/pk11wrap/pk11cxt.c:698:19
    #5 0x6efa09 in nss_test::GetBytes(PK11ContextStr*, unsigned char*, unsigned long) /home/worker/nss/out/Debug/../../gtests/pk11_gtest/pk11_cipherop_unittest.cc:28:18
    #6 0x6eeca3 in nss_test::Pkcs11CipherOp_SingleCtxMultipleUnalignedCipherOps_Test::TestBody() /home/worker/nss/out/Debug/../../gtests/pk11_gtest/pk11_cipherop_unittest.cc:71:3
    #7 0x84f80b in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2443:10
    #8 0x7c26f1 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2479:14
    #9 0x7c2086 in testing::Test::Run() /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2517:5
    #10 0x7c5ef6 in testing::TestInfo::Run() /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2693:11
    #11 0x7c96f8 in testing::TestCase::Run() /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2811:28
    #12 0x7f326a in testing::internal::UnitTestImpl::RunAllTests() /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:5177:43
    #13 0x85c59b in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2443:10
    #14 0x7f14d1 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:2479:14
    #15 0x7f0be1 in testing::UnitTest::Run() /home/worker/nss/out/Debug/../../gtests/google_test/gtest/src/gtest.cc:4786:10
    #16 0x795d4f in RUN_ALL_TESTS() /home/worker/nss/out/Debug/../../gtests/google_test/gtest/include/gtest/gtest.h:2341:46
    #17 0x795bf9 in main /home/worker/nss/out/Debug/../../gtests/common/gtests.cc:31:12
    #18 0x7f41fa987b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #19 0x568c99 in _start (/home/worker/dist/Debug/bin/pk11_gtest+0x568c99)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../lib/freebl/ctr.c:132:24 in 
gtests.sh: #10: pk11_gtest run successfully  - FAILED
test output dir: /home/worker/tests_results/security/localhost.1/gtests/pk11_gtest/report.xml
executing sed to parse the xml report
sed: can't read /home/worker/tests_results/security/localhost.1/gtests/pk11_gtest/report.xml: No such file or directory
processing the parsed report
~/tests_results/security/localhost.1/gtests

Comment 22

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1539788 - UBSAN fixup for 128b counter. r?mt,jcj

Comment 23

[J.C. Jones [:jcj] (he/they)]

https://hg.mozilla.org/projects/nss/rev/9d1f5e71773d4e3146524096d74cb96c8df51abe

Comment 24

[Kevin Jacobs [:kjacobs]]

Fixed by commit ^

Comment 25

[Daiki Ueno [:ueno]]

(In reply to Kevin Jacobs [:kjacobs] from comment #16)

Relevant code is exposed through RSA_Sign and RSA_Encrypt. Here's the call path for Sign:

https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#2622
https://searchfox.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#2417
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rsapkcs.c#1307
https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/rsapkcs.c#241 (line 239 is not compiled in release builds)

Encrypt (as in the added test) is similar. rsa_FormatBlock's case for RSA_BlockRaw checks the length, so actually CKM_RSA_X_509 is not affected.

Indeed, I misread the code. Huzaifa suggested that we have a CVE for this; would it be possible to request one?

Comment 26

[Kevin Jacobs [:kjacobs]]

:tjr, could we please get a CVE allocated for the issue in comment 4? We released the fix in NSS 3.46, FWIW.

Comment 27

[Huzaifa Sidhpurwala]

Can i please request you to:

• Add this CVE to the release notes, i see the bug number is already added to: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.46_release_notes , adding CVE would be great.
• Can this bug be please made public, now that the fix is out.

Thanks!

Comment 28

[Mark Straver]

Please do not rush making these bugs open to the public. We are still in the process of adjusting our code.

Comment 29

[J.C. Jones [:jcj] (he/they)]

(In reply to Huzaifa Sidhpurwala from comment #27)

Can i please request you to:

• Add this CVE to the release notes, i see the bug number is already added to: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.46_release_notes , adding CVE would be great.

Release notes are updated.

(In reply to Mark Straver from comment #28)

Please do not rush making these bugs open to the public. We are still in the process of adjusting our code.

Any estimate on that timeline, Mark? We should put it in the whiteboard here otherwise our periodic bug-openings will catch this one.

Comment 30

[Mark Straver]

Ah this one was caught in the 70 cycle, not 71. We should be good on that one, then. I didn't pay close attention when I saw two bugs with a request on publication and the first one I looked at was the current cycle (for which our release was today).

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Can we land the tests now?

Comment 32

[Kevin Jacobs [:kjacobs]]

The test for this one has landed: https://hg.mozilla.org/projects/nss/rev/4abc6ff828aba538ee3799dc6bef3afc8afd1fe6