1185033 - (CVE-2016-1979) ASan: use-after-poison in PK11_ImportDERPrivateKeyInfoAndReturnKey()

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 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.

Comment 2

[Daniel Veditz [:dveditz]]

Rating assuming this DER could come from web content (or an TLS cert?)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

Mr. Keeler, can you weigh in on the severity of this? Do you know who might have some cycles to try to fix it?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

Kai, does this seem reasonable? Thanks.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Mass cc to get some NSS eyes on these bugs.

Comment 9

[Robert Relyea]

Comment on attachment 8644582 [details] [diff] [review]
patch

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

r+ rrelyea

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Attachment: Remove unnecessary (char *) casts for the first argument to PORT_Memset

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.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch updated

Thanks for the reviews. I removed the comment - carrying over r+.

Comment 14

[Wan-Teh Chang]

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

Comment 15

[Wan-Teh Chang]

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.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

[Wan-Teh Chang]

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?

Comment 18

[Daniel Veditz [: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.)

Comment 19

[Wan-Teh Chang]

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

[Wan-Teh Chang]

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

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 41 as well.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 23

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

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.

Comment 24

[Daniel Veditz [:dveditz]]

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?

Comment 25

[Kai Engert [:KaiE:]]

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.)

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 29

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks David. Based on comment 31, let's review this bug again for the next esr i.e. 38.5.0 or later.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Not tracking as I guess this will be managed by the new version of nss uplift is 42.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Also untracking for 43.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

This should be fixed when we land nss 3.21, likely in esr 38.7.0 and Firefox 45.

Comment 36

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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?

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 38

[Kai Engert [:KaiE:]]

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.

Comment 39

[Kai Engert [:KaiE:]]

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

Comment 40

[Kai Engert [:KaiE:]]

Attachment: patch that applies cleanly on the NSS_3_19_2_X_BRANCH

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.

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for esr38.8

Comment 42

[Kai Engert [:KaiE:]]

Checked in to NSS_3_19_2_X_BRANCH
https://hg.mozilla.org/projects/nss/rev/df5c58f4b38d
https://hg.mozilla.org/projects/nss/rev/348a65c4f55f

Comment 43

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

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?

Comment 44

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

This is marked tracking for ESR38.8.

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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.

Comment 46

[Sylvestre Ledru [:Sylvestre]]

Same as in bug 1190248, this is going to be a wontfix. (here, we already shipped 45 with the fix)

Comment 47

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

(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.

Comment 48

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

https://hg.mozilla.org/releases/mozilla-esr38/rev/c64ca84020a1 is the update we took.

Comment 49

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

Comment 42 shows that the fixes for this (and the other two bugs) went in on March 9, 12 days before the ESR38 checkin.

Comment 50

[Daniel Veditz [:dveditz]]

NSS 3.19.2.4 landed on the ESR-38 branch in bug 1254986

Comment 51

[Sylvestre Ledru [:Sylvestre]]

My bad, thanks :)