1515342 - (CVE-2019-11729) Empty or malformed p256-ECDH public keys from project Wycheproof cause segfault

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jonas Allmann [:jallmann]]

Attachment: p256_testcases.txt

Three testcases from Project Wycheproof for p256-ECDH cause segfaults in public key verification functions.

The testvector integration for p256-ECDH is not completed yet, so none of the wycheproof testvectors for p256-ECDH is runnning in CI yet. 
For now, the bug(s) can be reproduced by simply adding the three testcases to the testvector header file for the Curve25519 unittest. This test can handle the testvectors for p256 without any modification. 

<<<>>>
Steps to reproduce:

Checkout current version of NSS.

Add testvectors attached to this bug to gtests/common/testvectors/curve25519-vectors.h

Build NSS

run '../dist/Debug/bin/pk11_gtest --gtest_filter="*Curve25519*" '


<<<>>>
Expected Result

No test failures.

<<<>>>
Actual Result

segfault produced by each of the inserted p256 testcase(s) at line

https://searchfox.org/nss/rev/7bc70a3317b800aac07bad83e74b6c79a9ec5bff/lib/freebl/ec.c#451

Comment 1

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

This seems like a sec-high to me. While to my knowledge we can't prompt ecdh from JS, this would be doable at a misconfigured server, and thus could be a remote segfault in the parent process. So until otherwise determined, let's go sec-high.

Comment 2

[Martin Thomson [:mt:]]

We can run ECDH with webcrypto.

Comment 3

[Martin Thomson [:mt:]]

Yuck, this is not one but probably three.  One in quickder.c (again!), maybe another in our public key import, and certainly another in our ECDH derivation.

https://searchfox.org/nss/rev/69203eee0575aace4900ae5d33e365b3d49413dd/lib/util/quickder.c#762

This is the result of our bit string decoder failing to account for overflow in one corner case.  In a simple example, we receive 03 01 04 as input.  The 03 is the tag for a bit string, the 01 its length.  The first octet of a DER bit string is the number of unused bits at the end of the sequence.

But the sequence is not present - it is zero length.  DER would have this byte be zero in this case, which would result in temp.len = (temp.len - 1) * 8 - (temp.data[0] & 0x7) = 0, but because it is a non-zero value, we underflow the integer without generating an error.

As a result, we return success, and pass garbage as a public key.  For this example, {type = siBuffer, data = <the input buffer>, len = 4294967292}.  That's bug 1.

However bad this is, it's not the cause of the segfault.  We would generate the same segfault with an otherwise valid encoding of public value that is zero length.

We pass the garbage SPKI to SECKEY_ExtractPublicKey().  What is unique about EC here is that we don't decode the public key, we just cram it into our internal structure blindly.  Other key types are parsed (again!), and errors might be detected.  But for EC, we just copy the value in without even really sanity checking it.

Bizarrely, this works fine, because DER_ConvertBitString() re-overflows the length value back to 0.  The resulting zero-length key is copied in, and SECITEM_CopyItem helpfully sets the .data pointer to NULL.  This is maybe not a bug, because that garbage length is so large we might reasonably attempt to add the value without checking for overflow, but we should definitely be checking for zero-length public values and failing to create a public key if we were passed an empty key.  That's bug 2.

The resulting public key (with a NULL public value) is ultimately passed to ECDH_ValidatePublicKey, where we fail to check that the public value is non-NULL and non-zero-length before reading off the first octet.  That's bug 3.

In this case, the error should have been caught in quickder.  That it wasn't means that any check we might reasonably add to SECKEY_ExtractPublicKey might have missed this problem, but ECDH_ValidatePublicKey has no such excuse for dereferencing NULL.

Based on this analysis, the only way in which this will manifest is as a read of memory location 0.  We might be able to downgrade this to sec-moderate, as much as these are - together - pretty bad.

Comment 4

[Martin Thomson [:mt:]]

Attachment: Bug 1515342 - Checks for invalid bit strings, r?jallmann

This is the easy DER piece.

Comment 5

[Martin Thomson [:mt:]]

Attachment: Bug 1515342 - Tests for invalid DH public keys, r?jallmann

This is a work in progress.  It demonstrates that the problem exists for DH and
ECDH.  The tests fail.

Note that this is intentionally more complex than necessary, creating RSA and
DSA keys, because I took that code from another in-flight patch.  I realized
that having this facility generically would be very useful, so I extracted the
common part.  I plan to use the other pieces in a related patch (bug 1496124).

Comment 6

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

mt, note that jallmann is out now until January.

Comment 7

[Martin Thomson [:mt:]]

That's fine.  You might have just volunteered yourself for review :)  I figured that Jonas would be more likely to have context, but I will take reviews from anyone.  (I wonder if we can't setup a group alias as other teams have.)

Comment 8

[Jonas Allmann [:jallmann]]

I honestly don't feel confident and competent enough to review these patches, so I would like to pass this task on to someone more experienced. My understanding of this issue does not reach very far into the inner workings of NSS.
Unless I'm supposed to see it as an exercise in code review, for which a security critical bug is maybe not the best option.

Comment 9

[Martin Thomson [:mt:]]

Attachment: Bug 1515342 - More thorough input checking, r=jcj

All part of applying better discipline throughout.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 9069833 [details]
Bug 1515342 - More thorough input checking, r=jcj

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This patch rolls up the fixes in the real patches here (D15061 and D15062), but does not include any test changes.

The tests would light the path to an exploit too easily and mixing a bunch of related changes makes it much less obvious which are the nasty ones. There is an integer underflow in a buffer length calculation mixed in here, so this is potentially very bad.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all of them
• 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?: This patch applies on older branches cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely.

From a code stability perspective, this is a small targeted change with plenty of testing.

From a compatibility perspective, if there are invalid encodings that are in use, this would start to reject those encodings. But I checked openssl code and they (correctly) reject invalid values in the same way as in this patch, so I'm comfortable that the compatibility risk is very low.

Comment 11

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

sec-approval+ for trunk. We'll want this on the beta (68) and ESR60 branches as well. Please nominate patches for these as well.

Comment 12

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

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

Comment 13

[Kevin Jacobs [:kjacobs]]

Attachment: Bug 1515342 - Ignore spki decode failures on negative (expect_fail) tests. r=jcj

Comment 14

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

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

Comment 15

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

3.36 branch:

• https://hg.mozilla.org/projects/nss/rev/d84ee4ba9eebe9d9a1cd138538a34444653d5a2e
• https://hg.mozilla.org/projects/nss/rev/247d2280022c5aae01a8ce39ba5c039776d2f5dc

3.44 branch:

• https://hg.mozilla.org/projects/nss/rev/fb9932d6e083322e7b5dfcd3d6e67477e0bb075a
• https://hg.mozilla.org/projects/nss/rev/416a8f7cf8986103b4d74694aac1198edbb08b3e

Comment 16

[Martin Thomson [:mt:]]

A reminder here to both add tests and to back out https://hg.mozilla.org/projects/nss/rev/ebc93d6daeaa9001d31fd18b5199779da99ae9aa when this is opened up.

Comment 17

[Martin Thomson [:mt:]]

Tests landed for NSS 3.47.

https://hg.mozilla.org/projects/nss/rev/f4fe0da734468df650b01fa260bb8df191a63e61
https://hg.mozilla.org/projects/nss/rev/7e3476b7a91246e5581e818e5f182566a2dc6232