1359639 - heap-buffer-overflow READ size 4 in [@ nsDirIndexParser::ParseData]

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Tyson Smith [:tsmith]]

This seems very similar to bug 1344461.

==47484==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000806bd8 at pc 0x7f6424aa645f bp 0x7fffbb8ec8b0 sp 0x7fffbb8ec8a8
READ of size 4 at 0x603000806bd8 thread T0
    #0 0x7f6424aa645e in nsDirIndexParser::ParseData(nsIDirIndex*, char*, int) /home/worker/workspace/build/src/netwerk/streamconv/converters/nsDirIndexParser.cpp:209:23
    #1 0x7f6424aa42e2 in nsDirIndexParser::ProcessData(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/netwerk/streamconv/converters/nsDirIndexParser.cpp:428:18
    #2 0x7f6424aa66a9 in nsDirIndexParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/worker/workspace/build/src/netwerk/streamconv/converters/nsDirIndexParser.cpp:371:10
    #3 0x7f64245138f4 in nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/worker/workspace/build/src/netwerk/base/nsBaseChannel.cpp:885:28
    #4 0x7f642455e19c in nsInputStreamPump::OnStateTransfer() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:611:33
    #5 0x7f642455cfd7 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:436:25
    #6 0x7f642434decd in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:96:20
    #7 0x7f64243aee60 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1270:14
    #8 0x7f64243ab8a8 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:393:10
    #9 0x7f642514d841 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #10 0x7f64250b0750 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10
    #11 0x7f64250b0750 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #12 0x7f64250b0750 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #13 0x7f642a440dbf in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27
    #14 0x7f642d8b2491 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:30
    #15 0x7f642da767de in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4542:22
    #16 0x7f642da78160 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4722:8
    #17 0x7f642da7942c in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4815:21
    #18 0x4eb3c3 in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:22
    #19 0x4eb3c3 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:307
    #20 0x7f643f83082f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
    #21 0x41cf18 in _start (/home/user/workspace/browsers/firefox_cnt/firefox+0x41cf18)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Tyson Smith [:tsmith]]

Attachment: raw_data.txt

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Daniel, do you have cycles to take a look?

Comment 4

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valentin.patch

The 'mFormat' array holds a series of types to output and is terminated with a final -1 to signal the end. The 200 data contains a syntax that populates the mFormat array but it was trivial to make it fill in the mFormat array and have it not add the terminating -1. This would lead to the display code reading past the end of the array (since the end marker wasn't there) when generating the output.

I've grown bored by the complexity this method features for very little gain. I cut out the code that counts how large array to dynamically allocate and I instead made it a fixed size array as it is tiny anyway and we can get rid of complexity this way. I just made the array handle 7 types, period - there are only 6 specified ones and leave room for always terminating the array.

This makes the summary look this nice:

 2 files changed, 8 insertions(+), 37 deletions(-)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Can you comment on the severity of this issue (from a security rating standpoint) and which branches are effected?

Comment 6

[Daniel Stenberg [:bagder]]

I believe the same flaw can also be used to write integers outside of the mFormat[] array, which is allocated with new on the heap. The values it can write there are the numbers -1 and 1 to 6  (the field type ids) depending on what string matches that are in the 200 line.

Is that sec-high? I'm not sure I'm creative enough to imagine what the worst use of such out of bounds read/write can be used for.

In the patch for bug 1344467 (landed three weeks ago: https://hg.mozilla.org/mozilla-central/rev/a3c28a335237) I fixed a related problem but it was clearly only impartial, so while it changed this particular code path somewhat it did nothing to fix this issue.

This problem exists in all branches, as the code is the same. The 1344467-fix was applied in "all" branches, which ironically now makes it easier to apply a fix for this bug on several branches.

Comment 7

[Patrick McManus [:mcmanus]]

that would be sec-crit - writable overflows in c++ are assumed exploitable with native code execution. (imo anyhow). Don't spend the cycles worrying about how.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Thanks!

Comment 9

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

Sec-critical, tracking for 54 and 55.

Comment 10

[Daniel Stenberg [:bagder]]

Comment on attachment 8861822 [details] [diff] [review]
0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valentin.patch

:tsmith, can you verify if this patch fixes this problem for you?

Comment 11

[Tyson Smith [:tsmith]]

Comment on attachment 8861822 [details] [diff] [review]
0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valentin.patch

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

I can still reproduce the issue.

Comment 12

[Tyson Smith [:tsmith]]

Attachment: test_case_2.zip

==30277==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b00009cd60 at pc 0x7f64347e5f8a bp 0x7ffcc80b86d0 sp 0x7ffcc80b86c8
READ of size 4 at 0x60b00009cd60 thread T0
    #0 0x7f64347e5f89 in nsDirIndexParser::ParseData(nsIDirIndex*, char*, int) /home/user/code/mozilla-central/netwerk/streamconv/converters/nsDirIndexParser.cpp:180:23
    #1 0x7f64347e4810 in nsDirIndexParser::ProcessData(nsIRequest*, nsISupports*) /home/user/code/mozilla-central/netwerk/streamconv/converters/nsDirIndexParser.cpp:399:18
    #2 0x7f64347e612c in nsDirIndexParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/user/code/mozilla-central/netwerk/streamconv/converters/nsDirIndexParser.cpp:342:10
    #3 0x7f6434326538 in nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/user/code/mozilla-central/netwerk/base/nsBaseChannel.cpp:885:17
    #4 0x7f6434359ff2 in nsInputStreamPump::OnStateTransfer() /home/user/code/mozilla-central/netwerk/base/nsInputStreamPump.cpp:611:22
    #5 0x7f64343591cc in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/user/code/mozilla-central/netwerk/base/nsInputStreamPump.cpp:436:25
    #6 0x7f64341b7b50 in nsInputStreamReadyEvent::Run() /home/user/code/mozilla-central/xpcom/io/nsStreamUtils.cpp:96:9
    #7 0x7f6434208650 in nsThread::ProcessNextEvent(bool, bool*) /home/user/code/mozilla-central/xpcom/threads/nsThread.cpp:1270:7
    #8 0x7f64342061e0 in NS_ProcessNextEvent(nsIThread*, bool) /home/user/code/mozilla-central/xpcom/threads/nsThreadUtils.cpp:393:10
    #9 0x7f6434d52627 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/user/code/mozilla-central/ipc/glue/MessagePump.cpp:96:21
    #10 0x7f6434c69eb9 in MessageLoop::Run() /home/user/code/mozilla-central/ipc/chromium/src/base/message_loop.cc:211:3
    #11 0x7f6438baae7f in nsBaseAppShell::Run() /home/user/code/mozilla-central/widget/nsBaseAppShell.cpp:156:3
    #12 0x7f643b4396f4 in nsAppStartup::Run() /home/user/code/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:283:19
    #13 0x7f643b56b2a5 in XREMain::XRE_mainRun() /home/user/code/mozilla-central/toolkit/xre/nsAppRunner.cpp:4541:10
    #14 0x7f643b56c827 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/user/code/mozilla-central/toolkit/xre/nsAppRunner.cpp:4721:8
    #15 0x7f643b56d202 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/user/code/mozilla-central/toolkit/xre/nsAppRunner.cpp:4814:16
    #16 0x4f3953 in do_main(int, char**, char**) /home/user/code/mozilla-central/browser/app/nsBrowserApp.cpp:236:10
    #17 0x4f3270 in main /home/user/code/mozilla-central/browser/app/nsBrowserApp.cpp:307:16
    #18 0x7f644c26782f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
    #19 0x41ea68 in _start (/home/user/code/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x41ea68)

Comment 13

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

Thanks.

Bah, yeah that wasn't very clever. It crashed _with_ my patch too because it lacked the init of the mFormat[] array in the Init() method so it would run with it uninitialized. Here's patch v2 for another attempt.

Comment 14

[Tyson Smith [:tsmith]]

Comment on attachment 8864514 [details] [diff] [review]
v2-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

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

That seems to have fixed the issue and I don't see any new crashes either.

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8864514 [details] [diff] [review]
v2-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

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

::: netwerk/streamconv/converters/nsDirIndexParser.cpp
@@ +126,3 @@
>    // Parse a "200" format line, and remember the fields and their
>    // ordering in mFormat. Multiple 200 lines stomp on each other.
> +  unsigned int formatNum=0;

nit: spacing around =

Comment 16

[Daniel Stenberg [:bagder]]

Attachment: v3-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

v3, fixed nit.

Comment 17

[Daniel Stenberg [:bagder]]

Comment on attachment 8865011 [details] [diff] [review]
v3-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Fairly easy. The patch is small and reveals it being a buffer overflow/overread.

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 of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I'm pretty sure the same patch will work unaltered on most if not all of them.

How likely is this patch to cause regressions; how much testing does it need?

Low risk. It is a small patch for a rarely-used feature.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 8865011 [details] [diff] [review]
v3-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

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

sec-approval=dveditz, but please wait to check this in until closer to release, like the week of May 22. Then we'll also want to land this on the Beta (54) and esr-52 branches.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/550c4bd5695ab4cbe48285d0b4ecf2151c5986b2

I've confirmed that this grafts cleanly to Beta and ESR52. Please nominate the patch for approval when you get a chance.

Comment 20

[Daniel Stenberg [:bagder]]

Comment on attachment 8865011 [details] [diff] [review]
v3-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: a buffer out of bounds access from C++ code remains
Fix Landed on Version: it was just merged in nightly 55
Risk to taking this patch (and alternatives if risky): minor risk due to small path in a feature area that is little used
String or UUID changes made by this patch: none

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/550c4bd5695a

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8865011 [details] [diff] [review]
v3-0001-bug-1359639-ensure-a-final-1-in-mFormat-r-valenti.patch

Fix a sec-critical. Beta54+ & ESR52+. Should be in 54 beta 11.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2be2b90e9c07
https://hg.mozilla.org/releases/mozilla-esr52/rev/a4f8d8a12afa