1460673 - Crash in SEC_ASN1DecoderFinish_Util importing PKCS#12 file encrypted with AES

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

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

Attachment: import-me-to-crash.p12

This bug was filed from the Socorro interface and is
report bp-53c9dfd0-6a2f-4543-a702-f4d7e0180510.
=============================================================

Top 10 frames of crashing thread:

0 libnssutil3.so SEC_ASN1DecoderFinish_Util security/nss/lib/util/secasn1d.c:2980
1 libsmime3.so SEC_PKCS7DecoderUpdate security/nss/lib/pkcs7/p7decode.c:1048
2 libnssutil3.so SEC_ASN1DecoderUpdate_Util security/nss/lib/util/secasn1d.c:2926
3 libsmime3.so sec_pkcs12_decoder_asafes_callback security/nss/lib/pkcs12/p12d.c:853
4 libsmime3.so sec_pkcs7_decoder_filter security/nss/lib/pkcs7/p7decode.c:172
5 libnssutil3.so SEC_ASN1DecoderUpdate_Util security/nss/lib/util/secasn1d.c:2926
6 libsmime3.so SEC_PKCS7DecoderUpdate security/nss/lib/pkcs7/p7decode.c:1037
7 libsmime3.so sec_pkcs12_decode_asafes_cinfo_update security/nss/lib/pkcs12/p12d.c:955
8 libnssutil3.so SEC_ASN1DecoderUpdate_Util security/nss/lib/util/secasn1d.c:2926
9 libsmime3.so SEC_PKCS12DecoderUpdate security/nss/lib/pkcs12/p12d.c:1288

=============================================================


Discovered this from https://www.mail-archive.com/[e-mail]/msg12911.html

Comment 1

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

This appears to be an NSS bug. I'll attach a reduced testcase shortly.

Comment 2

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

Attachment: test.cpp

Comment 3

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

Attachment: Makefile

Comment 4

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

STR:
1. Compile libdislocator from https://github.com/mirrorer/afl/tree/master/libdislocator
2. Compile test.cpp (`make`)
3. run `LD_PRELOAD=/path/to/libdislocator.so ./test`

Comment 5

[Franziskus Kiefer [:franziskus]]

Thanks for filing David. I responded to Jonathan who reported the issue to let him know we're looking into it.
Running this with ASAN gives more information. The issue is that SEC_PKCS7DecoderFinish is called before decoding is done, i.e. sec_pkcs12_decoder_wrap_p7_update is called on the freed p12dcx->currentASafeP7Dcx producing a heap-use-after-free.

Making this sec-moderate. A user has to actively import a p12 file into Firefox or call pk12util on the command line to be affected.

Comment 6

[Phabricator Automation]

Attachment: Bug 1460673 - don't crash

The reason for crashing here is that the p7 processor wasn't cleared after finishing the first P7 bag (for a P12 that has two P7 bags).
This patch doesn't fix the import itself but only the uaf.
The decoder isn't supporting this properly. Firefox will show an unknown P12 import error now.

test

Comment 7

[Franziskus Kiefer [:franziskus]]

This fixes the crash. Importing the p12 will still fail, but in a safe way.

https://hg.mozilla.org/projects/nss/rev/8872ebb63acdaf69cbe3a134c1aeb2c223e3eaaa

Comment 8

[Phabricator Automation]

Comment on attachment 8976067 [details]
Bug 1460673 - don't crash

Tim Taubert [:ttaubert] has approved the revision.

https://phabricator.services.mozilla.com/D1295

Comment 9

[Ryan VanderMeulen [:RyanVM]]

How far back does this issue go?

Comment 10

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> How far back does this issue go?

AFAICT, to whenever the P7 code was originally written for Netscape. It's present in March 2000 for the first checkin [1].


[1] https://hg.mozilla.org/projects/nss/file/cd12bbc0e0a9/security/nss/lib/pkcs7/p7decode.c#l1200

Comment 11

[Ryan VanderMeulen [:RyanVM]]

If we end up spinning updated NSS releases for the TLS 1.3 issue (bug 1462303) anyway, would it make sense to also include this as a ride-along?

Comment 12

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

It's probably good to get it into ESR60. I don't know that it's important to get into a 60 point release, but it would make sense to have it ride-along anyway.

(Franziskus is on PTO for the next week, so please NI me or :keeler if needed on this one now)

Comment 13

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

This is included in NSS 3.36.2 [1] and 3.37.1 [2], and landed in m-c this morning [3].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.36.2_release_notes
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.37.1_release_notes
[3] https://hg.mozilla.org/mozilla-central/rev/6d3ee860c038