Heap-buffer-overflow in nsCString::ReplaceSubstring

RESOLVED FIXED in Firefox 37, Firefox OS v2.2

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: Abhishek Arya, Assigned: Ehsan)

Tracking

(Depends on: 1 bug, {csectype-bounds, regression, sec-critical})

Trunk
mozilla37
x86
All
csectype-bounds, regression, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox35 unaffected, firefox36 unaffected, firefox37 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8538265 [details]
hbf.html

=================================================================
==30902==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6030000f8f40 at pc 0x00000048cc64 bp 0x7fff81ff09b0 sp 0x7fff81ff0170
READ of size 33 at 0x6030000f8f40 thread T0 (Web Content)
    #0 0x48cc63 in __asan_memcpy _asan_rtl_
    #1 0x7f663e45a7a8 in nsCString::ReplaceSubstring(nsCString const&, nsCString const&) xpcom/string/nsCharTraits.h:381:36
    #2 0x7f664220b158 in nsEncodingFormSubmission::EncodeVal(nsAString_internal const&, nsCString&, bool) dom/html/nsFormSubmission.cpp:761:5
    #3 0x7f664220cde1 in nsFSMultipartFormData::AddNameValuePair(nsAString_internal const&, nsAString_internal const&) dom/html/nsFormSubmission.cpp:425:8
    #4 0x7f66421c936d in mozilla::dom::HTMLTextAreaElement::SubmitNamesValues(nsFormSubmission*) dom/html/HTMLTextAreaElement.cpp:1058:10
    #5 0x7f66420a3a9a in mozilla::dom::HTMLFormElement::WalkFormElements(nsFormSubmission*) dom/html/HTMLFormElement.cpp:960:5
    #6 0x7f66420a2321 in mozilla::dom::HTMLFormElement::BuildSubmission(nsFormSubmission**, mozilla::WidgetEvent*) dom/html/HTMLFormElement.cpp:721:8
    #7 0x7f66420a1ba2 in mozilla::dom::HTMLFormElement::DoSubmit(mozilla::WidgetEvent*) dom/html/HTMLFormElement.cpp:655:17
    #8 0x7f664209ece8 in mozilla::dom::HTMLFormElement::Submit(mozilla::ErrorResult&) dom/html/HTMLFormElement.cpp:606:12
    #9 0x7f6640fbde83 in mozilla::dom::HTMLFormElementBinding::submit(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLFormElement*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/HTMLFormElementBinding.cpp:625:3
    #10 0x7f6641ce1012 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:2434:13
    #11 0x7f66469afe47 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:231:15
    #12 0x7f66469f5ac0 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2539:18
    #13 0x7f66469db2f0 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:432:12
    #14 0x7f66469a3922 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:641:15
    #15 0x7f6646a071db in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:677:12
    #16 0x7f6646645517 in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4788:19
    #17 0x7f6646645c28 in JS::Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4815:12
    #18 0x7f66406127bd in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) dom/base/nsJSUtils.cpp:256:12
    #19 0x7f6640613868 in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) dom/base/nsJSUtils.cpp:328:10
    #20 0x7f66406877d7 in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&, void**) dom/base/nsScriptLoader.cpp:1140:10
    #21 0x7f6640684e1c in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*, void**) dom/base/nsScriptLoader.cpp:970:10
    #22 0x7f664067e853 in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) dom/base/nsScriptLoader.cpp:783:10
    #23 0x7f664067ac94 in nsScriptElement::MaybeProcessScript() dom/base/nsScriptElement.cpp:140:10
    #24 0x7f663fb9aac5 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) dom/base/nsIScriptElement.h:220:18
    #25 0x7f663fb98f1a in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:488:7
    #26 0x7f663fb9f88b in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:127:9
    #27 0x7f663e57b0d6 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:835:7
    #28 0x7f663e5d1d06 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #29 0x7f663edf443f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:99:21
    #30 0x7f663eda6431 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #31 0x7f6642fd998f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:164:3
    #32 0x7f6644a8a762 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:731:12
    #33 0x7f663eda6431 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:233:3
    #34 0x7f6644a89ba4 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:568:7
    #35 0x4bb3be in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:211:19
    #36 0x7f663bbfdec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
0x6030000f8f40 is located 0 bytes to the right of 32-byte region [0x6030000f8f20,0x6030000f8f40)
allocated by thread T0 (Web Content) here:
    #0 0x499a39 in __interceptor_malloc _asan_rtl_
    #1 0x7f664a97fb1d in moz_xmalloc memory/mozalloc/mozalloc.cpp:52:17
    #2 0x7f663e51c1fe in char* ConvertUnknownBreaks<char>(char const*, int&, char const*) objdir-ff-asan/dist/include/nsMemory.h:38:12
    #3 0x7f663e51c04b in nsLinebreakConverter::ConvertLineBreaks(char const*, nsLinebreakConverter::ELinebreakType, nsLinebreakConverter::ELinebreakType, int, int*) xpcom/io/nsLinebreakConverter.cpp:292:20
    #4 0x7f664220b071 in nsEncodingFormSubmission::EncodeVal(nsAString_internal const&, nsCString&, bool) dom/html/nsFormSubmission.cpp:757:16
    #5 0x7f664220cde1 in nsFSMultipartFormData::AddNameValuePair(nsAString_internal const&, nsAString_internal const&) dom/html/nsFormSubmission.cpp:425:8
    #6 0x7f66421c936d in mozilla::dom::HTMLTextAreaElement::SubmitNamesValues(nsFormSubmission*) dom/html/HTMLTextAreaElement.cpp:1058:10
    #7 0x7f66420a3a9a in mozilla::dom::HTMLFormElement::WalkFormElements(nsFormSubmission*) dom/html/HTMLFormElement.cpp:960:5
    #8 0x7f66420a2321 in mozilla::dom::HTMLFormElement::BuildSubmission(nsFormSubmission**, mozilla::WidgetEvent*) dom/html/HTMLFormElement.cpp:721:8
    #9 0x7f66420a1ba2 in mozilla::dom::HTMLFormElement::DoSubmit(mozilla::WidgetEvent*) dom/html/HTMLFormElement.cpp:655:17
    #10 0x7f664209ece8 in mozilla::dom::HTMLFormElement::Submit(mozilla::ErrorResult&) dom/html/HTMLFormElement.cpp:606:12
    #11 0x7f6640fbde83 in mozilla::dom::HTMLFormElementBinding::submit(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLFormElement*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/HTMLFormElementBinding.cpp:625:3
    #12 0x7f6641ce1012 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:2434:13
    #13 0x7f66469afe47 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:231:15
    #14 0x7f66469f5ac0 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2539:18
    #15 0x7f66469db2f0 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:432:12
    #16 0x7f66469a3922 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:641:15
    #17 0x7f6646a071db in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:677:12
    #18 0x7f6646645517 in Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4788:19
    #19 0x7f6646645c28 in JS::Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4815:12
    #20 0x7f66406127bd in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) dom/base/nsJSUtils.cpp:256:12
    #21 0x7f6640613868 in nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) dom/base/nsJSUtils.cpp:328:10
    #22 0x7f66406877d7 in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&, void**) dom/base/nsScriptLoader.cpp:1140:10
    #23 0x7f6640684e1c in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*, void**) dom/base/nsScriptLoader.cpp:970:10
    #24 0x7f664067e853 in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) dom/base/nsScriptLoader.cpp:783:10
    #25 0x7f664067ac94 in nsScriptElement::MaybeProcessScript() dom/base/nsScriptElement.cpp:140:10
    #26 0x7f663fb9aac5 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) dom/base/nsIScriptElement.h:220:18
    #27 0x7f663fb98f1a in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:488:7
    #28 0x7f663fb9f88b in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:127:9
    #29 0x7f663e57b0d6 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:835:7

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x0c0680017190: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800171a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800171b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800171c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800171d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c06800171e0: fa fa fa fa 00 00 00 00[fa]fa 00 00 00 00 fa fa
  0x0c06800171f0: 00 00 00 00 fa fa 00 00 00 00 fa fa fd fd fd fa
  0x0c0680017200: fa fa 00 00 05 fa fa fa 00 00 00 fa fa fa 00 00
  0x0c0680017210: 00 00 fa fa fd fd fd fa fa fa fd fd fd fa fa fa
  0x0c0680017220: 00 00 00 fa fa fa 00 00 00 fa fa fa 00 00 00 06
  0x0c0680017230: fa fa 00 00 00 00 fa fa 00 00 00 fa fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  ASan internal:           fe
==30902==ABORTING
status-firefox37: --- → affected
Flags: sec-bounty?

Comment 1

3 years ago
Regression from bug 1101337?
Component: General → XPCOM
Flags: needinfo?(ehsan.akhgari)
(Assignee)

Comment 2

3 years ago
(In reply to Olli Pettay [:smaug] from comment #1)
> Regression from bug 1101337?

Yes.  I'll take a look.
Assignee: nobody → ehsan.akhgari
Flags: needinfo?(ehsan.akhgari)
(Assignee)

Updated

3 years ago
Blocks: 1101337
[Tracking Requested - why for this release]:
status-firefox36: --- → unaffected
Keywords: csectype-bounds, regression
status-firefox-esr31: --- → unaffected
(Assignee)

Comment 4

3 years ago
Created attachment 8538785 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer
Attachment #8538785 - Flags: review?(nfroyd)
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → affected
status-firefox35: --- → unaffected
(Assignee)

Comment 5

3 years ago
Comment on attachment 8538785 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer

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

The sec-approval text injection stuff seems to be broken, but here's the interesting bits:

This bug only affects nightly as of a few days ago.  It was regressed by bug 1101337.  This doesn't need backport.  It should be fairly difficult to go from the patch to an exploit, and the patch doesn't include a test case.
Attachment #8538785 - Flags: sec-approval?
Comment on attachment 8538785 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer

Clearing sec-approval. Since this is trunk only, it doesn't need sec-approval.

See https://wiki.mozilla.org/Security/Bug_Approval_Process for details.
Attachment #8538785 - Flags: sec-approval?
Comment on attachment 8538785 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer

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

Doh.
Attachment #8538785 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

3 years ago
Attachment #8538785 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8539466 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer
Attachment #8539466 - Flags: review?(nfroyd)
Comment on attachment 8539466 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer

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

Hooray, tests!
Comment on attachment 8539466 [details] [diff] [review]
Copy only the necessary number of bytes from the source buffer

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

Hooray, actually r+'ing patches!

::: xpcom/string/nsTStringObsolete.cpp
@@ +504,5 @@
>    if (!MutatePrep(XPCOM_MAX(mLength, newLength), &oldData, &oldFlags))
>      return;
>    if (oldData) {
>      // Copy all of the old data to the new buffer.
> +    char_traits::copy(mData, oldData, mLength);

All right, so this is legitimate only because we may have over-allocated in the MutatePrep() call above; we always allocate at least mLength, even if we are shrinking the buffer.
Attachment #8539466 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 12

3 years ago
(In reply to Nathan Froyd (:froydnj) from comment #11)
> ::: xpcom/string/nsTStringObsolete.cpp
> @@ +504,5 @@
> >    if (!MutatePrep(XPCOM_MAX(mLength, newLength), &oldData, &oldFlags))
> >      return;
> >    if (oldData) {
> >      // Copy all of the old data to the new buffer.
> > +    char_traits::copy(mData, oldData, mLength);
> 
> All right, so this is legitimate only because we may have over-allocated in
> the MutatePrep() call above; we always allocate at least mLength, even if we
> are shrinking the buffer.

That's correct, the rest of the algorithm relies on having access to all of the string.  I will clarify in a comment.  Thanks for the review!
It looks like we're writing to data off the end of an array, maybe by a fairly arbitrary amount, so I'm going to mark this sec-high.
Keywords: sec-high
Well, I guess critical is more appropriate.  It doesn't look very hard to trigger, based on the test case.
Keywords: sec-high → sec-critical
Ehsan, this is review+. Can we get this checked in?
Flags: needinfo?(ehsan)
(Assignee)

Comment 16

3 years ago
I landed this a while ago, not sure why the bug was not marked.

http://hg.mozilla.org/mozilla-central/rev/663e8640e196
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(ehsan)
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
status-firefox37: affected → fixed
status-b2g-v2.2: affected → fixed
Flags: sec-bounty? → sec-bounty+
Group: core-security
Depends on: 1306804
You need to log in before you can comment on or make changes to this bug.