Closed
Bug 1359639
Opened 7 years ago
Closed 7 years ago
heap-buffer-overflow READ size 4 in [@ nsDirIndexParser::ParseData]
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
People
(Reporter: tsmith, Assigned: bagder)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [necko-active][post-critsmash-triage][adv-main54+][adv-esr52.2+])
Attachments
(4 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
application/x-zip-compressed
|
Details | |
(deleted),
patch
|
bagder
:
review+
gchang
:
approval-mozilla-beta+
gchang
:
approval-mozilla-esr52+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
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)
Flags: in-testsuite?
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → daniel
Flags: needinfo?(daniel)
Assignee | ||
Comment 4•7 years ago
|
||
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•7 years ago
|
||
Can you comment on the severity of this issue (from a security rating standpoint) and which branches are effected?
Flags: needinfo?(daniel)
Assignee | ||
Comment 6•7 years ago
|
||
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.
Flags: needinfo?(daniel)
Comment 7•7 years ago
|
||
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•7 years ago
|
||
Thanks!
status-firefox53:
--- → wontfix
status-firefox54:
--- → affected
status-firefox-esr45:
--- → wontfix
status-firefox-esr52:
--- → affected
tracking-firefox54:
--- → ?
tracking-firefox55:
--- → ?
tracking-firefox-esr52:
--- → ?
Keywords: sec-critical
Sec-critical, tracking for 54 and 55.
Assignee | ||
Comment 10•7 years ago
|
||
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?
Attachment #8861822 -
Flags: feedback?(twsmith)
Reporter | ||
Comment 11•7 years ago
|
||
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.
Attachment #8861822 -
Flags: feedback?(twsmith) → feedback-
Reporter | ||
Comment 12•7 years ago
|
||
==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)
Updated•7 years ago
|
Assignee | ||
Comment 13•7 years ago
|
||
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.
Attachment #8861822 -
Attachment is obsolete: true
Attachment #8864514 -
Flags: feedback?(twsmith)
Reporter | ||
Comment 14•7 years ago
|
||
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.
Attachment #8864514 -
Flags: feedback?(twsmith) → feedback+
Assignee | ||
Updated•7 years ago
|
Attachment #8864514 -
Flags: review?(valentin.gosu)
Comment 15•7 years ago
|
||
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 =
Attachment #8864514 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 16•7 years ago
|
||
v3, fixed nit.
Attachment #8864514 -
Attachment is obsolete: true
Attachment #8865011 -
Flags: review+
Assignee | ||
Comment 17•7 years ago
|
||
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.
Attachment #8865011 -
Flags: sec-approval?
Comment 18•7 years ago
|
||
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.
Attachment #8865011 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Whiteboard: [wait until 5/22 to land]
Updated•7 years ago
|
Whiteboard: [wait until 5/22 to land] → [wait until 5/22 to land][necko-active]
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 19•7 years ago
|
||
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.
Keywords: checkin-needed
Whiteboard: [wait until 5/22 to land][necko-active] → [necko-active]
Assignee | ||
Comment 20•7 years ago
|
||
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
Attachment #8865011 -
Flags: approval-mozilla-esr52?
Attachment #8865011 -
Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/550c4bd5695a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 22•7 years ago
|
||
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.
Attachment #8865011 -
Flags: approval-mozilla-esr52?
Attachment #8865011 -
Flags: approval-mozilla-esr52+
Attachment #8865011 -
Flags: approval-mozilla-beta?
Attachment #8865011 -
Flags: approval-mozilla-beta+
Comment 23•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2be2b90e9c07 https://hg.mozilla.org/releases/mozilla-esr52/rev/a4f8d8a12afa
Updated•7 years ago
|
Group: network-core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [necko-active] → [necko-active][post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [necko-active][post-critsmash-triage] → [necko-active][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•