Closed Bug 1540541 (CVE-2019-11719) Opened 5 years ago Closed 5 years ago

PK11_ImportDERPrivateKeyInfoAndReturnKey causes OOB read when importing private keys with leading 0x00 bytes

Categories

(NSS :: Libraries, defect, P1)

3.43
defect

Tracking

(firefox-esr60 fixed, firefox67 wontfix, firefox67.0.1 wontfix, firefox68 fixed, firefox69 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr60 --- fixed
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: henrycg, Assigned: kjacobs)

References

Details

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

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

The attached C program exercises the bug. To reproduce:

  1. Import a curve25519 private key in PKCS#8 format using PK11_ImportDERPrivateKeyInfoAndReturnKey, where one or more of the leading bytes of the key is 0x00. The import operation succeeds. I am importing a key in the format used here:
    https://searchfox.org/nss/rev/cfd5fcba7efbfe116e2c08848075240ec3a92718/gtests/pk11_gtest/pk11_curve25519_unittest.cc#66

  2. Try to use the private key for DH key agreement (e.g., with PK11_PubDeriveWithKDF). The operation succeeds but returns an incorrect results. Furthermore, in the process, the operation makes what I believe is an OOB read.

Actual results:

  1. When importing a key using PK11_ImportDERPrivateKeyInfoAndReturnKey, the import operation strips leading 0x00 bytes from the private key:
    https://searchfox.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pk12.c#530

  2. Later on, when using PK11_PubDeriveWithKDF to use this secret curve25519 key to derive a DH secret, ECDH_Derive is invoked here:
    https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/ec.c#523

  3. I believe that ECDH_Derive then invokes ec_Curve25519_mul here:
    https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/ecl/ecp_25519.c#l
    Notice that ec_Curve25519_mul assumes that the input secret s is 32 bytes long:
    https://searchfox.org/mozilla-central/source/security/nss/lib/freebl/ecl/curve25519_32.c#370

If the leading zeros have been truncated, then s is < 32 bytes and we will have an OOB read. In addition, the operation returns an incorrect answer.

Expected results:

OPTION 1: The CKA_VALUE parameter for Curve25519 keys should not have their leading 0x00 bytes stripped in when importing:
https://searchfox.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pk12.c#513
There should be an assertion somewhere in this code to check that the CKA_VALUE string is exactly 32 bytes long when the key represents a Curve25519 public key.
(I haven't tested it, but I suspect that the CKA_EC_POINT parameter shouldn't have it's leading 0x00 bytes stripped either.)

OPTION 2: In ec_Curve25519_mul, rewrite the code to handle the case when the secret key s has length < 32 bytes. In particular, the mul code could add leading 0x00 bytes to pad the secret key up to 32 bytes.

Henry, thank you for reporting!

Confirmed OOB read via ASAN:

gcc -fsanitize=address -fsanitize-address-use-after-scope -fno-omit-frame-pointer -fno-optimize-sibling-calls --debug -mpascal-strings -O0 -gdwarf-2 -arch x86_64 -fPIC -I/Users/jcjones/hg/dist/Debug/include/nspr -I/Users/jcjones/hg/dist/public/nss -L/Users/jcjones/hg/dist/Debug/lib -lnss3 -lnspr4 -lfreebl3 /Users/jcjones/Downloads/encrypt.c
=================================================================
==11006==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300000899f at pc 0x00010c11a6df bp 0x7ffee41dffb0 sp 0x7ffee41df760
READ of size 32 at 0x60300000899f thread T0
    #0 0x10c11a6de in __asan_memcpy (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x596de)
    #1 0x10c0346db in Hacl_EC_crypto_scalarmult Hacl_Curve25519.c:824
    #2 0x10c035224 in Hacl_Curve25519_crypto_scalarmult Hacl_Curve25519.c:844
    #3 0x10c034444 in ec_Curve25519_mul curve25519_64.c:12
    #4 0x10bf35cc4 in ec_Curve25519_pt_mul ecp_25519.c:118
    #5 0x10bf1db21 in ECDH_Derive ec.c:569
    #6 0x10df17d28 in ECDH_Derive loader.c:1195
    #7 0x10decb256 in NSC_DeriveKey pkcs11c.c:7570
    #8 0x10bb347dd in pk11_PubDeriveECKeyWithKDF pk11skey.c:2212
    #9 0x10bb33225 in PK11_PubDeriveWithKDF pk11skey.c:2326
    #10 0x10ba1e34f in derive_dh_secret encrypt.c:78
    #11 0x10ba1dfff in main encrypt.c:186
    #12 0x7fff6302ced8 in start (libdyld.dylib:x86_64+0x16ed8)

0x60300000899f is located 0 bytes to the right of 31-byte region [0x603000008980,0x60300000899f)
allocated by thread T0 here:
    #0 0x10c11d053 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5c053)
    #1 0x10bd465b7 in PR_Malloc prmem.c:435
    #2 0x10cde4d16 in PORT_Alloc_Util secport.c:87
    #3 0x10cddf55f in SECITEM_CopyItem_Util secitem.c:280
    #4 0x10decaf07 in NSC_DeriveKey pkcs11c.c:7541
    #5 0x10bb347dd in pk11_PubDeriveECKeyWithKDF pk11skey.c:2212
    #6 0x10bb33225 in PK11_PubDeriveWithKDF pk11skey.c:2326
    #7 0x10ba1e34f in derive_dh_secret encrypt.c:78
    #8 0x10ba1dfff in main encrypt.c:186
    #9 0x7fff6302ced8 in start (libdyld.dylib:x86_64+0x16ed8)

SUMMARY: AddressSanitizer: heap-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x596de) in __asan_memcpy
Shadow bytes around the buggy address:
  0x1c06000010e0: fa fa fd fd fd fa fa fa 00 00 00 00 fa fa 00 00
  0x1c06000010f0: 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa
  0x1c0600001100: 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00
  0x1c0600001110: fa fa 00 00 00 00 fa fa fd fd fd fa fa fa fd fd
  0x1c0600001120: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
=>0x1c0600001130: 00 00 00[07]fa fa 00 00 00 00 fa fa fa fa fa fa
  0x1c0600001140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0600001150: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0600001160: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0600001170: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c0600001180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==11006==ABORTING
Abort trap: 6

This looks like it would be doable from WebCrypto, so I'm going to give this an initial categorization of sec-moderate as the corruption there would be in the content process. Since right now it looks like it's only a read, sec-low might be more appropriate, but I err upwards.

Keywords: sec-moderate
OS: Unspecified → All
Hardware: Unspecified → All

I have a local fix for this using Option 2, as IMHO it's less likely to cause unintended consequences (the downside being that we may have similar bugs elsewhere, which won't be fixed by this patch. I'd like to spend a little time reviewing for such cases...)

Regarding the public CKA_EC_POINT length check: currently the leading 0x00 bytes will be stripped and we'll reject the key for being short. Is this behavior we want to keep? There seems to be some debate over what level of validation is needed on Curve25519 public keys (1, 2, 3). If there are interoperability concerns from being too restrictive, we may want to loosen the check to allow padding with zero-bytes. Note that on the ECDH_Derive call path, we already check the public against forbiddenValues, including (0,0).

So I think that for curves with fixed input size, we should be preserving leading zeros. We should not even be looking at the values of key voters, public or private. But that was not good when I tried to do it generically. I have a patch that breaks everything.

If we're able to do option 1 without hitting too much code, that would be much better.

We can simply preserve based on the key and attribute types. Then we have two possibilities:

  1. The encoded value is the expected length (including any and all leading-zeros), everything works correctly.

  2. The encoded value is shorter than expected, in which case simply preserving didn't help us. We could either reject such private keys, or copy the bytes into a fixed size buffer prior to ec_mul. Any concerns with doing the former?

Adding to the above... Choosing to not strip zeros for EC is essentially whitelistng at a higher level based on the key type, e.g.:

for (ap = signedattr; signedcount; ap++, signedcount--) {
    if (keyType == CKK_EC) {
        if (ap->type == CKA_EC_POINT ||
            ap->type == CKA_VALUE)
            continue;
    }
    pk11_SignedToUnsigned(ap);
}

This feels like breaking encapsulation, and in looking at usages of 'pk11_SignedToUnsigned', a comment suggests it may not be simple to use this behavior globally. Further, any future callers of ecp_25519.c could re-expose the same bug (or a SECFailure) if they don't account for this.

ecp_256_32.c will pad to ORDER_LEN, and lib/freebl/rsa.c will handle either case (i.e. leading-0 present or not).

I still think option 2 is the cleaner approach here (perhaps with a future code review item requiring implementations to handle this gracefully...)

Does this seem reasonable?

Assignee: nobody → kjacobs.bugzilla
Priority: -- → P1

The thing I'm worried about is creating timing side channels out of private values.

In all cases, we should have a private value that is the same size as the group. We should not be looking at the octets except in the context of performing constant-time operations on that value. Any fixup we do is just going to leak. Sure, it's low probability, and we have blinding for the truly awful cases (hello RSA), but as a principle, we should not be doing this.

Thus, the ideal state is for us to take opaque octets, and save them without looking at them. When we import them, we test that there are the right number of bits and error out. When we use them, the same test is redundant, but we should check anyway.

However, I realize that we don't have that right now. So I would suggest this instead: don't strip leading zeros at all for any key. See if that works. It probably doesn't for all keys (I had a hell of a time with DSA and DH if I remember rightly). Then for those that break without trimmed leading zeros, add them to a list of safe options. Open a bug for each one to investigate why they break and see if that can be fixed.

Does that make sense?

That makes sense; I had the same concern about the padding calculation/timing side channel but didn't want to start a larger refactor without buy in.

Thanks for the feedback!

Blocks: 1558234

Targeting NSS 3.44.1 (Bug 1558549) and NSS 3.36.8 (Bug 1558548)

Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9065781 - Attachment description: Bug 1540541 - Don't strip leading 0's from EC key material during PKCS11 import. r=jcj!,mt! → Bug 1540541 - Don't unnecessarily strip leading 0's from key material during PKCS11 import. r=jcj!,mt!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.45
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-11719
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.