1753535 - AddressSanitizer: use-after-poison [@ SEC_ASN1DecoderUpdate_Util] with READ of size 8

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Christian Holler (:decoder)]

Using a fuzzing prototype that tests the MIME subsystem through mailnews/mime/src/nsStreamConverter.cpp, you can trigger the following crash (comm-central revision 6d820788220d+, patched fuzzing build):

==3552==ERROR: AddressSanitizer: use-after-poison on address 0x61d000c5c228 at pc 0x7f7c35049aa1 bp 0x7ffc908abe10 sp 0x7ffc908abe08
READ of size 8 at 0x61d000c5c228 thread T0
    #0 0x7f7c35049aa0 in SEC_ASN1DecoderUpdate_Util security/nss/lib/util/secasn1d.c:2942:43
    #1 0x7f7c330d09e1 in NSS_CMSDecoder_Update security/nss/lib/smime/cmsdecode.c:663:14
    #2 0x7f7c1e4d7df2 in nsCMSDecoder::Update(char const*, int) comm/mailnews/extensions/smime/nsCMS.cpp:1018:3
    #3 0x7f7c1e8a83a2 in MimeCMS_write(char const*, int, void*) comm/mailnews/mime/src/mimecms.cpp:545:33
    #4 0x7f7c1e8cc6fc in mime_decode_base64_buffer comm/mailnews/mime/src/mimeenc.cpp:287:12
    #5 0x7f7c1e8cc6fc in MimeDecoderWrite(MimeDecoderData*, char const*, int, int*) comm/mailnews/mime/src/mimeenc.cpp:742:14
    #6 0x7f7c1e905506 in MimeMessage_parse_line(char const*, int, MimeObject*) comm/mailnews/mime/src/mimemsg.cpp
    #7 0x7f7c1e8a28b8 in convert_and_send_buffer comm/mailnews/mime/src/mimebuf.cpp:132:10
    #8 0x7f7c1e8a28b8 in mime_LineBuffer comm/mailnews/mime/src/mimebuf.cpp:205:14
    #9 0x7f7c1e913d52 in MimeObject_parse_buffer(char const*, int, MimeObject*) comm/mailnews/mime/src/mimeobj.cpp:223:10
    #10 0x7f7c1e8f3a90 in mime_display_stream_write comm/mailnews/mime/src/mimemoz2.cpp:861:10
    #11 0x7f7c1e933cd9 in nsStreamConverter::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) comm/mailnews/mime/src/nsStreamConverter.cpp:812:9
    #12 0x7f7c0f3672ce in nsBaseChannel::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) netwerk/base/nsBaseChannel.cpp:861:28
    #13 0x7f7c0f3ba68b in nsInputStreamPump::OnStateTransfer() netwerk/base/nsInputStreamPump.cpp:541:23
    #14 0x7f7c0f3b9349 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/nsInputStreamPump.cpp:374:21
    #15 0x7f7c0ef057fd in RunAsyncWaitCallback xpcom/io/NonBlockingAsyncInputStream.cpp:398:13
    #16 0x7f7c0ef057fd in mozilla::NonBlockingAsyncInputStream::AsyncWaitRunnable::Run() xpcom/io/NonBlockingAsyncInputStream.cpp:33:14
    #17 0x7f7c0f01ed70 in mozilla::RunnableTask::Run() xpcom/threads/TaskController.cpp:467:16
    #18 0x7f7c0efd3b83 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:770:26
    #19 0x7f7c0efd0648 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:606:15
    #20 0x7f7c0efd0d3f in mozilla::TaskController::ProcessPendingMTTask(bool) xpcom/threads/TaskController.cpp:390:36
    #21 0x7f7c0f011734 in operator() xpcom/threads/TaskController.cpp:127:37
    #22 0x7f7c0f011734 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_1>::Run() xpcom/threads/nsThreadUtils.h:531:5
    #23 0x7f7c0eff6f47 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1195:16
    #24 0x7f7c0f003dd1 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:467:10
    #25 0x7f7c0bfed0b1 in bool mozilla::SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, mozilla::net::FuzzingStreamListener::waitUntilDone()::'lambda'()>(nsTSubstring<char> const&, mozilla::net::FuzzingStreamListener::waitUntilDone()::'lambda'()&&, nsIThread*) dist/include/mozilla/SpinEventLoopUntil.h:176:25
    #26 0x7f7c0de6b80b in waitUntilDone dist/include/mozilla/fuzzing/FuzzingStreamListener.h:23:5
    #27 0x7f7c0de6b80b in FuzzingMimeDecoder comm/mailnews/mime/fuzz/TestMimeFuzz.cpp:93:21
    [...]

0x61d000c5c228 is located 1448 bytes inside of 2048-byte region [0x61d000c5bc80,0x61d000c5c480)
allocated by thread T0 here:
    #0 0x55bef556768d in __interceptor_malloc compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3
    #1 0x7f7c350bba07 in PL_ArenaAllocate nsprpub/lib/ds/plarena.c:134:27
    #2 0x7f7c350575f6 in PORT_ArenaAlloc_Util security/nss/lib/util/secport.c:318:9
    #3 0x7f7c3505770a in PORT_ArenaZAlloc_Util security/nss/lib/util/secport.c:339:9
    #4 0x7f7c3504b479 in SEC_ASN1DecoderStart_Util security/nss/lib/util/secasn1d.c:3015:36
    #5 0x7f7c330cf78b in NSS_CMSDecoder_Start security/nss/lib/smime/cmsdecode.c:618:18
    #6 0x7f7c1e4d7cb8 in nsCMSDecoder::Start(void (*)(void*, char const*, unsigned long), void*) comm/mailnews/extensions/smime/nsCMS.cpp:1007:11
    #7 0x7f7c1e8a71f6 in MimeCMS_init(MimeObject*, int (*)(char const*, int, void*), void*) comm/mailnews/mime/src/mimecms.cpp:482:33
    #8 0x7f7c1e8ab36e in MimeEncrypted_parse_begin(MimeObject*) comm/mailnews/mime/src/mimecryp.cpp:75:25
    #9 0x7f7c1e906cb2 in MimeMessage_close_headers comm/mailnews/mime/src/mimemsg.cpp:429:12
    #10 0x7f7c1e906cb2 in MimeMessage_parse_line(char const*, int, MimeObject*) comm/mailnews/mime/src/mimemsg.cpp:213:14
    #11 0x7f7c1e8a28b8 in convert_and_send_buffer comm/mailnews/mime/src/mimebuf.cpp:132:10
    #12 0x7f7c1e8a28b8 in mime_LineBuffer comm/mailnews/mime/src/mimebuf.cpp:205:14
    #13 0x7f7c1e913d52 in MimeObject_parse_buffer(char const*, int, MimeObject*) comm/mailnews/mime/src/mimeobj.cpp:223:10
    #14 0x7f7c1e8f3a90 in mime_display_stream_write comm/mailnews/mime/src/mimemoz2.cpp:861:10
    #15 0x7f7c1e933cd9 in nsStreamConverter::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) comm/mailnews/mime/src/nsStreamConverter.cpp:812:9
    #16 0x7f7c0f3672ce in nsBaseChannel::OnDataAvailable(nsIRequest*, nsIInputStream*, unsigned long, unsigned int) netwerk/base/nsBaseChannel.cpp:861:28
    #17 0x7f7c0f3ba68b in nsInputStreamPump::OnStateTransfer() netwerk/base/nsInputStreamPump.cpp:541:23
    #18 0x7f7c0f3b9349 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/nsInputStreamPump.cpp:374:21
    #19 0x7f7c0ef057fd in RunAsyncWaitCallback xpcom/io/NonBlockingAsyncInputStream.cpp:398:13
    #20 0x7f7c0ef057fd in mozilla::NonBlockingAsyncInputStream::AsyncWaitRunnable::Run() xpcom/io/NonBlockingAsyncInputStream.cpp:33:14
    #21 0x7f7c0f01ed70 in mozilla::RunnableTask::Run() xpcom/threads/TaskController.cpp:467:16
    #22 0x7f7c0efd3b83 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:770:26
    #23 0x7f7c0efd0648 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:606:15
    #24 0x7f7c0efd0d3f in mozilla::TaskController::ProcessPendingMTTask(bool) xpcom/threads/TaskController.cpp:390:36
    #25 0x7f7c0f011734 in operator() xpcom/threads/TaskController.cpp:127:37
    #26 0x7f7c0f011734 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_1>::Run() xpcom/threads/nsThreadUtils.h:531:5
    #27 0x7f7c0eff6f47 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1195:16
    #28 0x7f7c0f003dd1 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:467:10
    #29 0x7f7c0bfed0b1 in bool mozilla::SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, mozilla::net::FuzzingStreamListener::waitUntilDone()::'lambda'()>(nsTSubstring<char> const&, mozilla::net::FuzzingStreamListener::waitUntilDone()::'lambda'()&&, nsIThread*) dist/include/mozilla/SpinEventLoopUntil.h:176:25
    #30 0x7f7c0de6b80b in waitUntilDone dist/include/mozilla/fuzzing/FuzzingStreamListener.h:23:5
    [...]

SUMMARY: AddressSanitizer: use-after-poison security/nss/lib/util/secasn1d.c:2942:43 in SEC_ASN1DecoderUpdate_Util
Shadow bytes around the buggy address:
  0x0c3a80183830: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
=>0x0c3a80183840: f7 f7 f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c3a80183850: 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
  Poisoned by user:        f7
==3552==ABORTING

I haven't investigated this one yet, it could be a use-after-free in the internal allocator of NSS or some other reason why the memory is poisoned. Will take a closer look tomorrow.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

I've managed to reproduce this outside of Thunderbird with a pure NSS fuzzing target. At first, I wrote a target for the CMS decoder code that would simply call Start, Update and Finish but this wouldn't work. So I came up with another target that calls Start, then chunks the input buffer into multiple parts (randomly, single leading byte for each chunk size) and send each one to Update. This reproduces the bug fairly quickly and I am concerned that this might not be limited to Thunderbird. The NSS_CMSDecoder_Update essentially just calls SEC_ASN1DecoderUpdate and that is called from more places than just this.

Comment 4

[Christian Holler (:decoder)]

Attachment: PoC Fuzz Target for NSS

Here is a PoC that can be used to reproduce this.

Apply to NSS, then build with nss/build.sh --fuzz --clang --asan.

Then run with dist/Debug/bin/nssfuzz-smime-mult.

It takes less than 30 seconds for me to find the crash just running that, but I'll also attach a sample for reproducing.

Comment 5

[Christian Holler (:decoder)]

Attachment: Sample Crash for NSS fuzzing target

Comment 6

[Christian Holler (:decoder)]

Note that the PoC target here is very crude: It does not use properly DER encoded inputs at all. This can probably be significantly improved with the custom ASN/1 mutators.

Comment 7

[John Schanck [:jschanck]]

In the afterEndOfContents case here we can have stateEnd == state->child. The call to sec_asn1d_pop_state frees state->child and then we read the invalid reference on on line 2942.

Comment 8

[John Schanck [:jschanck]]

Attachment: Simple sample

I've attached a hand minimized sample that I created to better understand the underlying problem. It corresponds to valid DER

  0x30, 0x29, 0x06, 0x01, 0xaa, 0xa0, 0x22, 0x04, 0x20, 0xaa, 0xaa, 0xaa,
  0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
  0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0xaa,
  0xaa, 0xaa, 0xaa, 0xaa, 0xaa, 0x00, 0x00

Which asn1parse interprets as:

    0:d=0  hl=2 l=  41 cons: SEQUENCE          
    2:d=1  hl=2 l=   1 prim:  OBJECT            :BAD OBJECT:[AA]
    5:d=1  hl=2 l=  34 cons:  cont [ 0 ]        
    7:d=2  hl=2 l=  32 prim:   OCTET STRING      [HEX DUMP]:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
   41:d=1  hl=2 l=   0 prim:  EOC               

Splitting this structure anywhere inside the octet string causes us to read from freed memory. I think the flaw is quite generic (not limited to CMS), but I don't see any security implications.

I suppose we could rewrite stateEnd->parent != state as stateEnd != state->child on line 2942, but I actually don't see why this condition (which is the only place we use stateEnd) is needed. It was introduced for Bug 101683. That bug contains a sample pfx file, but I've not been able to reproduce the original issue. None of our tests seem to require the stateEnd check either.

Comment 9

[Christian Holler (:decoder)]

John, can we fix this in any case, even if the security impact is unclear at this point? I'd rather be on the safe side and also it helps further NSS fuzzing.

Comment 10

[John Schanck [:jschanck]]

Attachment: Bug 1753535 - Remove obsolete stateEnd check in SEC_ASN1DecoderUpdate. r=rrelyea

The stateEnd->parent != state check was added in Bug 95458 to avoid a crash
in sec_asn1d_free_child. The diagnosis in Bug 95458 is incorrect---the crash
was actually due to a PORT_Assert(0) that was meant to highlight a memory
leak when SEC_ASN1DecoderStart was called with their_pool==NULL. The
offending assertion was removed in Bug 95311, which makes the stateEnd check
obsolete. In Bug 1753535 it was observed that the stateEnd check could read
from a poisoned region of an arena when the decoder was used in a streaming
mode. This read-after-poison could lead to an arena memory leak, although this
is mitigated by the fact that the read-after-poison is on an error-handling path
where the caller typically frees the entire arena.

Comment 11

[Kai Engert (:KaiE:)]

Could this fix be considered for esr91 backport?

Comment 12

[John Schanck [:jschanck]]

Comment on attachment 9267419 [details]
Bug 1753535 - Remove obsolete stateEnd check in SEC_ASN1DecoderUpdate. r=rrelyea

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Any exploit of this issue would depend on the caller's behavior, and we're not aware of any vulnerable callers. It's possible that some external consumer of NSS is vulnerable (to an arena memory leak).
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch applies cleanly to NSS 3.68.2, and it is a candidate for inclusion in NSS 3.68.3.
• How likely is this patch to cause regressions; how much testing does it need?: Regressions from this patch are very unlikely.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9267419 [details]
Bug 1753535 - Remove obsolete stateEnd check in SEC_ASN1DecoderUpdate. r=rrelyea

approved to land and do whatever uplifting things are necessary

Comment 14

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/800111fa3bf8225fd45a50cc192a032b62b54fc6

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.