866525 - ASan: Several tests cause use-after-poison [@ port_ArenaZeroAfterMark] through ASN1 decoder in NSS

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Christian Holler (:decoder)]

I'm seeing this trace from several tests (e.g. security/manager/ssl/tests/unit/test_signed_apps.js) on mozilla-central revision 9d8977cbbfc6:

 ==22911==ERROR: AddressSanitizer: use-after-poison on address 0x61d00009bf7f at pc 0x7fe4fa33b16f bp 0x7fe4def9e950 sp 0x7fe4def9e948
 WRITE of size 224 at 0x61d00009bf7f thread T9
     #0 0x7fe4fa33b16e in port_ArenaZeroAfterMark security/nss/lib/util/secport.c:405
     #1 0x7fe4fa33a573 in port_ArenaRelease security/nss/lib/util/secport.c:457
     #2 0x7fe4fa32fecb in sec_asn1d_free_child security/nss/lib/util/secasn1d.c:1385
     #3 0x7fe4fa32f0af in sec_asn1d_pop_state security/nss/lib/util/secasn1d.c:2313
     #4 0x7fe4fa328d57 in SEC_ASN1DecoderUpdate_Util security/nss/lib/util/secasn1d.c:2664
     #5 0x7fe4fa3bcc37 in SEC_PKCS7DecoderUpdate security/nss/lib/pkcs7/p7decode.c:1034
     #6 0x7fe4fa3bcf3b in SEC_PKCS7DecodeItem security/nss/lib/pkcs7/p7decode.c:1098
     #7 0x7fe4f165d18c in (anonymous namespace)::OpenSignedJARFile(nsIFile*, nsIZipReader**, nsIX509Cert3**) security/manager/ssl/src/JARSignatureVerification.cpp:565
     #8 0x7fe4f165caa5 in (anonymous namespace)::OpenSignedJARFileTask::CalculateResult() security/manager/ssl/src/JARSignatureVerification.cpp:738
     #9 0x7fe4f165c3f3 in mozilla::CryptoTask::Run() security/manager/ssl/src/CryptoTask.cpp:40
     #10 0x7fe4f254a347 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627
     #11 0x7fe4f24a09b3 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan64dbg/xpcom/build/nsThreadUtils.cpp:238
     #12 0x7fe4f254842c in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:265
     #13 0x7fe4fa45a9a3 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:204
     #14 0x41fd17 in __asan::AsanThread::ThreadStart() _asan_rtl_
     #15 0x7fe4fa097e99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
     #16 0x7fe4ed3b04bc in ?? /build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112
 0x61d00009bf7f is located 1279 bytes inside of 2048-byte region [0x61d00009ba80,0x61d00009c280)
[...]
 =>0x0c3a8000b7e0: 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00[01]
   0x0c3a8000b7f0: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
   Poisoned by user:      f7


From the message and the legend, this looks like an out-of-bounds write, possibly an off-by-one error. Marking s-s until resolved. 

This error is very likely uncovered by the recent NSPR poisoning added (bug 844513). Next time we make such a large change, we should run our own tests against the patch before landing.

Comment 1

[Christian Holler (:decoder)]

Right now, the following xpcshell tests are failing due to this error:

security/manager/ssl/tests/unit/test_signed_apps.js
security/manager/ssl/tests/unit/test_signed_apps-marketplace.js
services/crypto/tests/unit/test_crypto_deriveKey.js
services/sync/tests/unit/test_utils_deriveKey.js
services/sync/tests/unit/test_upgrade_old_sync_key.js

Also, we have several failures on mochitest 1 to 5 and mochitest-other.

Comment 2

[Christian Holler (:decoder)]

Making this a Q2 goal for Security Assurance in terms of supporting developers to get this fixed.

Comment 3

[Daniel Veditz [:dveditz]]

zero the memory /before/ we free it?

Comment 4

[Wan-Teh Chang]

Attachment: Patch

Please review this patch.

I test for the PL_MAKE_MEM_UNDEFINED macro to allow NSS
to compiled against older versions of NSPR that don't
have the ASan annotation macros.

Comment 5

[Christian Holler (:decoder)]

Inspecting the trace, it seems that we die when trying to zero memory before we release it. That means we're trying to release memory that has already been released (and poisoned therefore).

I tried to find out where the memory has initially been poisoned and came up with this trace:

>    Breakpoint 1, __asan_poison_memory_region (addr=0x61d0000500a0, size=2016) at /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_poisoning.cc:67
>    67      void __asan_poison_memory_region(void const volatile *addr, uptr size) {
>    #0  __asan_poison_memory_region (addr=0x61d0000500a0, size=2016) at /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_poisoning.cc:67
>    #1  0x00007f6d8c654a19 in FreeArenaList (pool=0x608000015da0, head=0x608000015da0, reallyFree=<optimized out>) at nsprpub/lib/ds/plarena.c:281
>    #2  0x00007f6d8c5afc67 in PORT_FreeArena_Util (arena=0x608000015da0, zero=<optimized out>) at secport.c:318
>    #3  0x00007f6d719c04b3 in sftkdb_write (handle=0x60b00000e740, object=<optimized out>, objectID=0x622000012118) at sftkdb.c:1205
>    #4  0x00007f6d7197cf9c in sftk_handleTrustObject (session=<optimized out>, object=0x622000012100) at pkcs11.c:769
>    #5  0x00007f6d7197cba2 in sftk_handleObject (object=0x622000012100, session=0x60d00000eb10) at pkcs11.c:1588
>    #6  0x00007f6d71986816 in NSC_CreateObject (hSession=16777220, pTemplate=<optimized out>, ulCount=11, phObject=0x7fff5d8d9e80) at pkcs11.c:4097
>    #7  0x00007f6d80adb539 in import_object (tok=0x61d0000492b0, sessionOpt=<optimized out>, objectTemplate=0x7fff5d8da0e0, otsize=11) at devtoken.c:214
>    #8  0x00007f6d80adedbd in nssToken_ImportTrust (tok=0x61d0000492b0, sessionOpt=0x0, certEncoding=<optimized out>, certIssuer=<optimized out>, 
> certSerial=<optimized out>, serverAuth=nssTrustLevel_MustVerify, clientAuth=<optimized out>, codeSigning=<optimized out>, emailProtection=<optimized out>,
> stepUpApproved=<optimized out>, asTokenObject=<optimized out>) at devtoken.c:1111
>    #9  0x00007f6d80ad746e in STAN_ChangeCertTrust (cc=<optimized out>, trust=<optimized out>) at pki3hack.c:1207
>    #10 0x00007f6d80abc893 in __CERT_AddTempCertToPerm (cert=0x61d0000500a0, nickname=<optimized out>, trust=0x7e0) at stanpcertdb.c:330
>    #11 0x00007f6d859766eb in nsNSSCertificateDB::AddCertFromBase64 (this=<optimized out>, aBase64=0x7fff5d8da6f0 "", aTrust=<optimized out>, aName=0xc3a0000a012 "")
>        at security/manager/ssl/src/nsNSSCertificateDB.cpp:1611
>    #12 0x00007f6d859769d1 in nsNSSCertificateDB::AddCert (this=0x60700001f3c0, aCertDER=..., aTrust=0x60200001b030 ",,CTu", aName=0x60200001b010 "test-root")
>        at security/manager/ssl/src/nsNSSCertificateDB.cpp:1624
>    #13 0x00007f6d86823ceb in NS_InvokeByIndex (that=0x61d0000500a0, methodIndex=<optimized out>, paramCount=3, params=0x7fff5d8dac40)
>        at xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
>    #14 0x00007f6d855c9d26 in CallMethodHelper::Call (this=0x7fff5d8dac00) at js/xpconnect/src/XPCWrappedNative.cpp:2280
>    #15 0x00007f6d855c9a4e in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at js/xpconnect/src/XPCWrappedNative.cpp:2246 


I verified that the address ASan complained about is within the range of that poison (addr=0x61d0000500a0, size=2016) and there was no repoisoning of that area afterwards.

Comment 6

[Christian Holler (:decoder)]

(In reply to Wan-Teh Chang from comment #4)
> Created attachment 743880 [details] [diff] [review]
> Patch
> 
> Please review this patch.
> 
> I test for the PL_MAKE_MEM_UNDEFINED macro to allow NSS
> to compiled against older versions of NSPR that don't
> have the ASan annotation macros.

I don't fully understand. The NSPR version in the tree has the ASan annotation macros right?

Comment 7

[Christian Holler (:decoder)]

@wtc, Nevermind, it gets clear after reading the comments in your patch. Testing now, thx!

Comment 8

[Wan-Teh Chang]

decoder: no problem. When testing, please also touch
security/nss/coreconf/coreconf.dep. This is the hack to force NSS
to be recompiled. Thank you for your help.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 743880 [details] [diff] [review]
Patch

+ * The #ifdef PL_MAKE_MEM_UNDEFINED tests can be removed when NSPR 4.10+
+ * is widely available.

I suggest to add
  TODO:
in front of that part of the comment.

Comment 10

[Christian Holler (:decoder)]

Comment on attachment 743880 [details] [diff] [review]
Patch

Confirmed this fixes the issue. Also asking cdiehl to provide feedback, as this could possibly also fix bug 865921.

Comment 11

[Wan-Teh Chang]

Ah, I thought this bug is bug 865921 :-)

Bug 865921 was most likely caused by not recompiling NSS
because of the lack of header dependencies in NSS makefiles.
Bug 865921 needs to be fixed by a dummy change to
security/nss/coreconf/coreconf.dep.

Bug 865921 won't affect a clobber build. A clobber build
will run into this bug, which requires the patch in this
bug.

Comment 12

[Wan-Teh Chang]

Attachment: Patch v2

I made the comment change that Kai suggested and checked this in:
https://hg.mozilla.org/projects/nss/rev/3986289106f1

I will push this fix to mozilla-central tomorrow.

Comment 13

[Christian Holler (:decoder)]

Thanks a lot! :)

Comment 14

[Wan-Teh Chang]

Comment on attachment 743920 [details] [diff] [review]
Patch v2

Patch pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0314d200873a

Unfortunately I used the wrong bug number (NSPR bug 844513) in the hg
commit message.

Comment 15

[Christian Holler (:decoder)]

Not a security bug. The NSS code was using internals of the NSPR arena stuff, bypassing the poisoning/unpoisoning which lead to this false positive.

Comment 16

[Wan-Teh Chang]

Comment on attachment 743920 [details] [diff] [review]
Patch v2

Patch pushed to mozilla-inbound (second attempt):
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f5c1bbcf76

Comment 17

[Wan-Teh Chang]

https://hg.mozilla.org/mozilla-central/rev/c6f5c1bbcf76

choller: thank you for your patience.