Closed Bug 1515342 (CVE-2019-11729) Opened 2 years ago Closed 2 years ago

Empty or malformed p256-ECDH public keys from project Wycheproof cause segfault


(NSS :: Libraries, defect, P1)



(firefox-esr6068+ fixed, firefox67 wontfix, firefox68+ fixed, firefox69+ fixed)

Tracking Status
firefox-esr60 68+ fixed
firefox67 --- wontfix
firefox68 + fixed
firefox69 + fixed


(Reporter: jallmann, Assigned: mt)



(Keywords: csectype-nullptr, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main68+][adv-esr60.8+])


(5 files)

Attached file 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
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.
Severity: normal → critical
Keywords: sec-high
Priority: -- → P1
We can run ECDH with webcrypto.
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.

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 - ([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.
This is the easy DER piece.
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).
mt, note that jallmann is out now until January.
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.)
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.

All part of applying better discipline throughout.

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.

Attachment #9069833 - Flags: sec-approval?

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

Attachment #9069833 - Flags: sec-approval? → sec-approval+
Assignee: nobody → mt
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.45

A reminder here to both add tests and to back out when this is opened up.

Group: crypto-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main68+][adv-esr60.8+]
Alias: CVE-2019-11729
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.