Bug 1185033 (CVE-2016-1979)

ASan: use-after-poison in PK11_ImportDERPrivateKeyInfoAndReturnKey()

RESOLVED FIXED in Firefox 45

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: keeler)

Tracking

(Blocks 1 bug, {csectype-uaf, sec-high})

Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox39 wontfix, firefox40 wontfix, firefox41+ wontfix, firefox42- wontfix, firefox43- wontfix, firefox44 wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed, firefox-esr3846+ fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-v2.2r affected, b2g-master affected)

Details

(Whiteboard: [post-critsmash-triage][b2g-adv-main2.5?][adv-main45+][adv-esr38.8+])

Attachments

(4 attachments, 1 obsolete attachment)

Not sure if this is a security-sensitive issue, feel free to remove the flag if it isn't. Keep in mind that the WebCrypto API basically provides direct access to this function via JS.


==16549==ERROR: AddressSanitizer: use-after-poison on address 0x61d0000188c0 at pc 0x7f739cc30fdc bp 0x7ffe6379a0f0 sp 0x7ffe6379a0e0
WRITE of size 6 at 0x61d0000188c0 thread T0
    #0 0x7f739cc30fdb in memset /usr/include/x86_64-linux-gnu/bits/string3.h:90
    #1 0x7f739cc30fdb in SECKEY_DestroyPrivateKeyInfo /home/tim/nss/lib/cryptohi/seckey.c:1567
    #2 0x7f739ccf7a39 in PK11_ImportDERPrivateKeyInfoAndReturnKey /home/tim/nss/lib/pk11wrap/pk11pk12.c:245
    #3 0x40470e in main /home/tim/nss/cmd/checkcert/checkcert.c:39
    #4 0x7f739bd70a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)
    #5 0x404c68 in _start (/tmp/afl-ramdisk/fuzz-pkcs8/checkcert+0x404c68)

0x61d0000188c0 is located 576 bytes inside of 2048-byte region [0x61d000018680,0x61d000018e80)
allocated by thread T0 here:
    #0 0x7f739d5c4827 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x57827)
    #1 0x7f739b425e85 in PL_ArenaAllocate ../../../lib/ds/plarena.c:203

SUMMARY: AddressSanitizer: use-after-poison /usr/include/x86_64-linux-gnu/bits/string3.h:90 memset
Shadow bytes around the buggy address:
  0x0c3a7fffb0c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3a7fffb0d0: 00 00 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb0e0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb0f0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb100: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
=>0x0c3a7fffb110: f7 f7 f7 f7 f7 f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb120: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb130: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb140: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb150: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a7fffb160: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==16549==ABORTING
Posted file testcase.zip
Here's a zip file containing the code for the executable I used for fuzzing. I simply replaced nss/cmd/checkcert/checkcert.c and built everything with ASan and AFL. The input file reproduces the use-after-poision.
Rating assuming this DER could come from web content (or an TLS cert?)
Blocks: nss-fuzz
STR:

1) Check out AFL, NSPR, and NSS. Build AFL.
2) Modify nss/coreconf/Linux.mk so that

CC = /path/to/afl/afl-gcc
CCC = /path/to/afl/afl-g++
ZDEFS_FLAG =

3) Copy test.c from the attached .zip file to nss/cmd/checkcert/checkcert.c.

4) Cd into NSS directory and do:

AFL_USE_ASAN=1 AFL_HARDEN=1 make nss_build_all

5) Feed input.der from the attached .zip file to the newly built checkcert binary:

nss/cmd/checkcert/Linux...OBJ/checkcert input.der


I'm reusing the checkcert binary only because that had a working Makefile and the quickest way to get something running. If you want to let AFL run for a while I could give you a few more PKCS#8 DER files that I started fuzzing with, but you could simply create a few.
Mr. Keeler, can you weigh in on the severity of this? Do you know who might have some cycles to try to fix it?
Flags: needinfo?(dkeeler)
I can reproduce this via content. I think the current rating is correct. I can have a better look tomorrow, but I'll also bring this up at the NSS meeting to see if there's someone who might be better suited.
Flags: needinfo?(dkeeler)
After some investigation, the issue appears to be that PK11_ImportDERPrivateKeyInfoAndReturnKey incorrectly assumes data structures populated by SEC_ASN1DecodeItem will be valid even if it returns SECFailure. PK11_ImportDERPrivateKeyInfoAndReturnKey unconditionally calls SECKEY_DestroyPrivateKeyInfo on the SECKEYPrivateKeyInfo that it gave to SEC_ASN1DecodeItem to fill out. If the decoding failed, SECKEY_DestroyPrivateKeyInfo will operate on already-freed data.
Posted patch patch (obsolete) — Splinter Review
Kai, does this seem reasonable? Thanks.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8644582 - Flags: review?(kaie)
See Also: → CVE-2015-7181
Mass cc to get some NSS eyes on these bugs.

Comment 9

4 years ago
Comment on attachment 8644582 [details] [diff] [review]
patch

Review of attachment 8644582 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea
Attachment #8644582 - Flags: review?(kaie) → review+

Comment 10

4 years ago
Comment on attachment 8644582 [details] [diff] [review]
patch

Review of attachment 8644582 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

Another fix, which we can make simultaneously, seems to be removing
the PORT_Memset call:

 	    PORT_Memset(pvk->privateKey.data, 0, pvk->privateKey.len);

in SECKEY_DestroyPrivateKeyInfo. That PORT_Memset call seems
unnecessary because pvk->privateKey.data must be pointing to
data in the arena.

::: lib/pk11wrap/pk11pk12.c
@@ +237,5 @@
> +        /* If SEC_ASN1DecodeItem fails, we cannot assume anything about the
> +         * validity of the data in pki. The best we can do is free the arena
> +         * and return.
> +         */
> +	PORT_FreeArena(temparena, PR_TRUE /*zero*/);

Nit: the "zero" comment isn't necessary, or we should also
add the comment to the PR_FALSE argument on line 229 for
consistency.
Attachment #8644582 - Flags: review+

Comment 11

4 years ago
Comment on attachment 8644582 [details] [diff] [review]
patch

On second thought, SECKEY_DestroyPrivateKeyInfo only frees the
arena when |freeit| is true. So we can't depend on that.

Comment 12

4 years ago
PORT_Memset is defined as memset. The first argument to memset
is a void * pointer, so any data pointer can be passed directly
without a typecast.
Attachment #8647778 - Flags: review?(dkeeler)
Attachment #8647778 - Flags: review?(dkeeler) → review+
Posted patch patch updatedSplinter Review
Thanks for the reviews. I removed the comment - carrying over r+.
Attachment #8644582 - Attachment is obsolete: true
Attachment #8648154 - Flags: review+

Comment 14

4 years ago
Comment on attachment 8647778 [details] [diff] [review]
Remove unnecessary (char *) casts for the first argument to PORT_Memset

Patch checked in on the NSS trunk:
https://hg.mozilla.org/projects/nss/rev/d099fde0a66e
Attachment #8647778 - Flags: checked-in+

Comment 15

4 years ago
Comment on attachment 8648154 [details] [diff] [review]
patch updated

Review of attachment 8648154 [details] [diff] [review]:
-----------------------------------------------------------------

David: do you have commit access to the NSS repository? If not,
I'll check in this patch for you.
I don't believe I do have commit access to the NSS repository, so if you could check that patch in, that would be great. Is there any particular process for checking security-sensitive patches into NSS like we do for mozilla-central?

Comment 17

4 years ago
Dan: this bug is rated sec-critical. I think the security severity should be
lowered. The use-after-poison bug overwrites a block of memory with 0 bytes.
See the PORT_Memset() call on line 1567:

1555 void
1556 SECKEY_DestroyPrivateKeyInfo(SECKEYPrivateKeyInfo *pvk,
1557                              PRBool freeit)
1558 {
1559     PLArenaPool *poolp;
1560 
1561     if(pvk != NULL) {
1562         if(pvk->arena) {
1563             poolp = pvk->arena;
1564             /* zero structure since PORT_FreeArena does not support
1565              * this yet.
1566              */
1567             PORT_Memset(pvk->privateKey.data, 0, pvk->privateKey.len);
1568             PORT_Memset(pvk, 0, sizeof(*pvk));
1569             if(freeit == PR_TRUE) {
1570                 PORT_FreeArena(poolp, PR_TRUE);
1571             } else {
1572                 pvk->arena = poolp;
1573             }
1574         } else {

The block of memory that is zeroed may or may not have been reused.
It requires more analysis to determine the likelihood of zeroing a
reused block of memory. You can be conservative and assume that we
are zeroing a block of memory that has been reused. Is overwriting
a block of memory with 0 bytes a critical security bug?
Flags: needinfo?(dveditz)
There's no good general way to reason about whether it could be exploited or not. What kind of things could be allocated out of that arena? What members are there, and if some of their values were nulled out could bad thing happen? Could an attacker arrange to have a vulnerable object allocated in the to-be-stomped spot?

We've seen limited controlled writes of 0/null result in an exploitable vulnerability. It's not worth doing the analysis necessary to determine whether this null overwrite is one of the potentially exploitable ones compared to how easy it is to fix so we just assume the worst.

"critical" does overstate the danger: we're not in immanent danger of this being used against our users, it would take a lot of effort to turn this into an exploit (if indeed it can be). (It was a good first guess, though, based on an out of bounds write.)
Flags: needinfo?(dveditz)
Keywords: sec-criticalsec-high

Comment 19

4 years ago
Dan: can I check in David Keeler's patch now? Or does the Mozilla
security team need to choreograph the NSS checkin and release?

Comment 20

4 years ago
Comment on attachment 8648154 [details] [diff] [review]
patch updated

Review of attachment 8648154 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nss/rev/7033b1193c94
Attachment #8648154 - Flags: checked-in+

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → 3.21
Tracking for 41 as well.

Updated

4 years ago
Group: core-security → core-security-release
Dan, will there be a fix for this issue that will need to be uplifted to mozilla-beta branch? I noticed that there was a NSS related checkin in comment 20. Do we need to make any changes on our side so this is fixed for FF41? I am mainly following up because this is a 41 tracked bug.
Flags: needinfo?(dveditz)
Ritu, I expect we'd need to take an overall NSS update to get this fixed. I don't know that we're planning on updating NSS in Firefox 41.
There won't be a "beta" patch because this was checked into a different repository (nss). Fixing this on beta would require a bug for "upgrade mozilla-beta to the version of NSS that has this patch (and others we need if any)", and that in turn will require a "tag an NSS release or beta" NSS bug.

Richard: Can you coordinate such a release with the nss-dev folks?
Flags: needinfo?(dveditz) → needinfo?(rlb)
Mozilla Beta 41 uses NSS 3.19.2

In the meantime we have already released NSS 3.19.3 and NSS 3.20

I assume the fix has been checked in to the latest development branch of NSS, only, which will eventually be released as NSS 3.21

I've just returned from a vacation, but in my understanding, NSS 3.21 isn't ready for release yet.

How much depends the bugfix for this bug on other code? Would it be safe to apply the bugfix to NSS 3.19.x?

We would have to create a security patch release (e.g. 3.19.4 or 3.20.1) in order to give you a release tag. (Linux distributions rely on being able to package NSS separately, and therefore need to use released versions of NSS.)
Al, DVeditz: Last when I checked on NSS upgrade plans, I was told it would be upgraded in FF44. Please see: https://bugzilla.mozilla.org/show_bug.cgi?id=1158489#c34. Has something changed that decision as mentioned in the other bug?
I was told that the decision to upgrade NSS was pushed out to FF44 time frame. I am unaware of any changes to that plan. This is now a wontfix for 41. Please let me know if there are any concerns.
Calling this wontfix for Fx42/Fx43 seems rather peculiar to me. Shouldn't we either take the 3.21 update once it lands on m-c or an NSS bugfix release per comment 25?
Flags: needinfo?(sledru)
Ryan, I changed the status flag because of comment #27 ("upgrade NSS was pushed out to FF44 time frame").
but with the recent info about nss, let's set them back to affected.
David, after the fix from bug 1211585 is uplifted to esr38 branch on oct 16th, can I set status-esr38 as fixed? Or are there more fixes needed here?
Flags: needinfo?(dkeeler)
This landed in NSS 3.21 and it looks like NSS 3.19.2.1 will be landing on esr38, so no, this won't be fixed in esr38 by that patch (i.e. bug 1211585). I brought up the possibility of backporting this to the upcoming security releases in the NSS call last week, but due to some process complications, we decided against it. I think this will essentially be WONTFIX for any branch that doesn't update to NSS 3.21.
Flags: needinfo?(dkeeler)
Flags: needinfo?(rlb)
Thanks David. Based on comment 31, let's review this bug again for the next esr i.e. 38.5.0 or later.
Not tracking as I guess this will be managed by the new version of nss uplift is 42.
Also untracking for 43.
Whiteboard: [b2g-adv-main2.5?]
This should be fixed when we land nss 3.21, likely in esr 38.7.0 and Firefox 45.
Since this will be fixed by bug 1211568 being uplifted to ESR, and since that bug is already tracked for ESR, I'm not sure it makes sense to track this bug separately. WDYT, Liz?
Flags: needinfo?(lhenry)
Sounds like we may not uplift for 38esr in that bug, let me talk with ritu and see what she thinks. Since I suggested pushing this back a cycle in comment 35, we had a discussion about how to handle NSS updates and it looks like Kai had different plans for 38.7.
Flags: needinfo?(lhenry)
Please note that I don't make the plans for Firefox branches.

I'm reacting to requests. In order to help you, I need more advance notice.

If you want NSS security bugs fixed on older branches, please request the NSS team to provide a snapshot of NSS for you. Then we can investigate what to do, either backporting a fix to an older NSS branch, or uplifting to a newer NSS release. But such decisions should be made early in a 6-week cycle, not close to the end.
I've received a request to have this bug fixed on ESR 38.x

I'll start a discussion with the NSS developers about the recommended way to fix this bug in ESR 38.x
There have been two commits in this bug, and they apply cleanly on the NSS_3_19_2_X_BRANCH. This is a patch that combines the two commits.
Whiteboard: [b2g-adv-main2.5?] → [b2g-adv-main2.5?][adv-main45+][adv-esr38.7+]
Whiteboard: [b2g-adv-main2.5?][adv-main45+][adv-esr38.7+] → [b2g-adv-main2.5?]
Whiteboard: [b2g-adv-main2.5?] → [b2g-adv-main2.5?][adv-main45+][adv-esr38.7+]
Tracked for esr38.8
Whiteboard: [b2g-adv-main2.5?][adv-main45+][adv-esr38.7+] → [b2g-adv-main2.5?][adv-main45+]
Alias: CVE-2016-1979

Updated

3 years ago
Blocks: 1254986
Flags: qe-verify-
Whiteboard: [b2g-adv-main2.5?][adv-main45+] → [post-critsmash-triage][b2g-adv-main2.5?][adv-main45+]
According to https://kuix.de/mozilla/versions/ ESR38.7 has NSS 3.19.2.4

Mozilla software version on branch mozilla-esr38: 38.7.1esrpre
NSPR (portable runtime) version: NSPR_4_10_10_RTM
NSS (security library) version: NSS_3_19_2_4_RTM
CKBI (builtin trust) version: 2.4

Did ESR38.8 take an NSS 3.19.2.5 or similar update?
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(kaie)
This is marked tracking for ESR38.8.
(In reply to Al Billings [:abillings] from comment #44)
> This is marked tracking for ESR38.8.

Hi Al, I marked this as tracking for esr38.8 bcuz we decided *not* to take an uplift during the last week of esr38.7 so by marking it tracking back then, I wanted to leave a reminder that we should take NSS uplift for esr38.8.
Flags: needinfo?(rkothari)
Same as in bug 1190248, this is going to be a wontfix. (here, we already shipped 45 with the fix)
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #46)
> Same as in bug 1190248, this is going to be a wontfix. (here, we already
> shipped 45 with the fix)

Except Ritu says we did take an NSS update on 3/21, after 38.7 shipped. 

The question is whether that update contains this fix or not.

You can't "won't fix" this if you don't know that as it might *already* be fixed.
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Comment 42 shows that the fixes for this (and the other two bugs) went in on March 9, 12 days before the ESR38 checkin.
NSS 3.19.2.4 landed on the ESR-38 branch in bug 1254986
Flags: needinfo?(rkothari)
Whiteboard: [post-critsmash-triage][b2g-adv-main2.5?][adv-main45+] → [post-critsmash-triage][b2g-adv-main2.5?][adv-main45+][adv-esr38.8+]
My bad, thanks :)
Flags: needinfo?(sledru)
Group: core-security-release

Updated

3 years ago
Flags: needinfo?(kaie)
You need to log in before you can comment on or make changes to this bug.