Closed Bug 1113005 Opened 5 years ago Closed 5 years ago

Heap-buffer-overflow in nsCString::ReplaceSubstring

Categories

(Core :: XPCOM, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
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

People

(Reporter: inferno, Assigned: ehsan)

References

(Depends on 1 open bug)

Details

(Keywords: csectype-bounds, regression, sec-critical)

Attachments

(2 files, 1 obsolete file)

Attached file 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
Flags: sec-bounty?
Regression from bug 1101337?
Component: General → XPCOM
Flags: needinfo?(ehsan.akhgari)
(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)
Blocks: 1101337
[Tracking Requested - why for this release]:
Attachment #8538785 - Flags: review?(nfroyd)
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+
Attachment #8538785 - Attachment is obsolete: true
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+
(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-highsec-critical
Ehsan, this is review+. Can we get this checked in?
Flags: needinfo?(ehsan)
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
Closed: 5 years ago
Flags: needinfo?(ehsan)
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.