Closed Bug 1747185 Opened 2 years ago Closed 2 years ago

Assertion failure: aLength <= kMax (string is too large), at /builds/worker/checkouts/gecko/xpcom/string/nsTStringRepr.h:84

Categories

(Core :: DOM: File, defect)

Product:
Core
Component:
DOM: File
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
97 Branch
Tracking Flags:
Tracking Status
firefox-esr91 97+ fixed
firefox95 --- unaffected
firefox96 --- unaffected
firefox97 + verified

People

(Reporter: tsmith, Assigned: mccr8)

References

(Blocks 1 open bug, Regression)

Details

(5 keywords, Whiteboard: [bugmon:bisected,confirmed][adv-esr91.6-])

Attachments

(2 files)

testcase.html
152 bytes, text/html
Details
Bug 1747185 - Reject INT32_MAX length arrays in CreateInputStream.
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-esr91+
Details | Review
Attached file testcase.html —

Found while fuzzing m-c 20211214-51773d1ab7b5 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html --xvfb

Assertion failure: aLength <= kMax (string is too large), at /builds/worker/checkouts/gecko/xpcom/string/nsTStringRepr.h:84

==18863==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7f2a5f3d63da bp 0x7ffc72ca63a0 sp 0x7ffc72ca6380 T0)
==18863==The signal is caused by a WRITE memory access.
==18863==Hint: address points to the zero page.
    #0 0x7f2a5f3d63da in nsTStringLengthStorage /builds/worker/workspace/obj-build/dist/include/nsTStringRepr.h:84:5
    #1 0x7f2a5f3d63da in nsTDependentSubstring<char>::Rebind(char const*, unsigned long) /gecko/xpcom/string/nsTDependentSubstring.cpp:40:26
    #2 0x7f2a5f5841c7 in nsStringInputStream::ShareData(char const*, int) /gecko/xpcom/io/nsStringStream.cpp:230:9
    #3 0x7f2a5f586247 in NS_NewByteInputStream(nsIInputStream**, mozilla::Span<char const, 18446744073709551615ul>, nsAssignmentType) /gecko/xpcom/io/nsStringStream.cpp:513:20
    #4 0x7f2a656bd337 in mozilla::dom::MemoryBlobImpl::DataOwnerAdapter::Create(mozilla::dom::MemoryBlobImpl::DataOwner*, unsigned int, unsigned int, nsIInputStream**) /gecko/dom/file/MemoryBlobImpl.cpp:61:8
    #5 0x7f2a656bd958 in mozilla::dom::MemoryBlobImpl::CreateInputStream(nsIInputStream**, mozilla::ErrorResult&) /gecko/dom/file/MemoryBlobImpl.cpp:88:9
    #6 0x7f2a656be66f in mozilla::dom::MultipartBlobImpl::CreateInputStream(nsIInputStream**, mozilla::ErrorResult&) /gecko/dom/file/MultipartBlobImpl.cpp:93:15
    #7 0x7f2a656a7d20 in CreateInputStream /gecko/dom/file/Blob.cpp:247:10
    #8 0x7f2a656a7d20 in mozilla::dom::Blob::Stream(JSContext*, JS::MutableHandle<JSObject*>, mozilla::ErrorResult&) /gecko/dom/file/Blob.cpp:355:3
    #9 0x7f2a633fb72b in mozilla::dom::Blob_Binding::stream(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/BlobBinding.cpp:726:24
    #10 0x7f2a64cef18d in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /gecko/dom/bindings/BindingUtils.cpp:3306:13
    #11 0x7f2a6c7e8f44 in CallJSNative /gecko/js/src/vm/Interpreter.cpp:388:13
    #12 0x7f2a6c7e8f44 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:475:12
    #13 0x7f2a6c7d540e in CallFromStack /gecko/js/src/vm/Interpreter.cpp:539:10
    #14 0x7f2a6c7d540e in Interpret(JSContext*, js::RunState&) /gecko/js/src/vm/Interpreter.cpp:3243:16
    #15 0x7f2a6c7ba211 in js::RunScript(JSContext*, js::RunState&) /gecko/js/src/vm/Interpreter.cpp:357:13
    #16 0x7f2a6c7e907f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:507:13
    #17 0x7f2a6c7eb1cb in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:552:8
    #18 0x7f2a6ca648dd in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /gecko/js/src/vm/CallAndConstruct.cpp:117:10
    #19 0x7f2a6490a3e9 in mozilla::dom::EventListener::HandleEvent(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/EventListenerBinding.cpp:62:8
    #20 0x7f2a655bf6b4 in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/EventListenerBinding.h:65:12
    #21 0x7f2a655bf170 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /gecko/dom/events/EventListenerManager.cpp:1303:43
    #22 0x7f2a655c081c in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /gecko/dom/events/EventListenerManager.cpp:1500:17
    #23 0x7f2a655aeb0e in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /gecko/dom/events/EventDispatcher.cpp:348:17
    #24 0x7f2a655ad31d in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /gecko/dom/events/EventDispatcher.cpp:550:16
    #25 0x7f2a655b1595 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /gecko/dom/events/EventDispatcher.cpp:1085:11
    #26 0x7f2a67f4d7df in nsDocumentViewer::LoadComplete(nsresult) /gecko/layout/base/nsDocumentViewer.cpp:1085:7
    #27 0x7f2a6b8aca13 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /gecko/docshell/base/nsDocShell.cpp:6338:20
    #28 0x7f2a6b8abd0b in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /gecko/docshell/base/nsDocShell.cpp:5727:7
    #29 0x7f2a6b8adcdf in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /gecko/docshell/base/nsDocShell.cpp
    #30 0x7f2a61c71b80 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /gecko/uriloader/base/nsDocLoader.cpp:1376:3
    #31 0x7f2a61c70794 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /gecko/uriloader/base/nsDocLoader.cpp:974:14
    #32 0x7f2a61c6cfc2 in nsDocLoader::DocLoaderIsEmpty(bool, mozilla::Maybe<nsresult> const&) /gecko/uriloader/base/nsDocLoader.cpp:793:9
    #33 0x7f2a61c6f185 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult) /gecko/uriloader/base/nsDocLoader.cpp:676:5
    #34 0x7f2a6b8e604b in nsDocShell::OnStopRequest(nsIRequest*, nsresult) /gecko/docshell/base/nsDocShell.cpp:13601:23
    #35 0x7f2a5f99bffe in mozilla::net::nsLoadGroup::NotifyRemovalObservers(nsIRequest*, nsresult) /gecko/netwerk/base/nsLoadGroup.cpp:614:22
    #36 0x7f2a5f99ea43 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /gecko/netwerk/base/nsLoadGroup.cpp:518:10
    #37 0x7f2a62ea24a2 in mozilla::dom::Document::DoUnblockOnload() /gecko/dom/base/Document.cpp:11556:18
    #38 0x7f2a62e4f3e7 in mozilla::dom::Document::UnblockOnload(bool) /gecko/dom/base/Document.cpp:11486:9
    #39 0x7f2a62e7a181 in mozilla::dom::Document::DispatchContentLoadedEvents() /gecko/dom/base/Document.cpp:7999:3
    #40 0x7f2a62f6976d in applyImpl<mozilla::dom::Document, void (mozilla::dom::Document::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1147:12
    #41 0x7f2a62f6976d in apply<mozilla::dom::Document, void (mozilla::dom::Document::*)()> /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1153:12
    #42 0x7f2a62f6976d in mozilla::detail::RunnableMethodImpl<mozilla::dom::Document*, void (mozilla::dom::Document::*)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:1200:13
    #43 0x7f2a5f60727f in mozilla::SchedulerGroup::Runnable::Run() /gecko/xpcom/threads/SchedulerGroup.cpp:144:20
    #44 0x7f2a5f653d52 in mozilla::RunnableTask::Run() /gecko/xpcom/threads/TaskController.cpp:468:16
    #45 0x7f2a5f61933d in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /gecko/xpcom/threads/TaskController.cpp:771:26
    #46 0x7f2a5f616898 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /gecko/xpcom/threads/TaskController.cpp:607:15
    #47 0x7f2a5f616fa9 in mozilla::TaskController::ProcessPendingMTTask(bool) /gecko/xpcom/threads/TaskController.cpp:391:36
    #48 0x7f2a5f65d391 in operator() /gecko/xpcom/threads/TaskController.cpp:124:37
    #49 0x7f2a5f65d391 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /gecko/xpcom/threads/nsThreadUtils.h:531:5
    #50 0x7f2a5f639637 in nsThread::ProcessNextEvent(bool, bool*) /gecko/xpcom/threads/nsThread.cpp:1183:16
    #51 0x7f2a5f644b2c in NS_ProcessNextEvent(nsIThread*, bool) /gecko/xpcom/threads/nsThreadUtils.cpp:467:10
    #52 0x7f2a60b51cbf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /gecko/ipc/glue/MessagePump.cpp:85:21
    #53 0x7f2a609d1631 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:331:10
    #54 0x7f2a609d1631 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:324:3
    #55 0x7f2a609d1631 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:306:3
    #56 0x7f2a678c9a67 in nsBaseAppShell::Run() /gecko/widget/nsBaseAppShell.cpp:137:27
    #57 0x7f2a6c506aef in XRE_RunAppShell() /gecko/toolkit/xre/nsEmbedFunctions.cpp:864:20
    #58 0x7f2a609d1631 in RunInternal /gecko/ipc/chromium/src/base/message_loop.cc:331:10
    #59 0x7f2a609d1631 in RunHandler /gecko/ipc/chromium/src/base/message_loop.cc:324:3
    #60 0x7f2a609d1631 in MessageLoop::Run() /gecko/ipc/chromium/src/base/message_loop.cc:306:3
    #61 0x7f2a6c505d22 in XRE_InitChildProcess(int, char**, XREChildData const*) /gecko/toolkit/xre/nsEmbedFunctions.cpp:701:34
    #62 0x556358190e9d in content_process_main(mozilla::Bootstrap*, int, char**) /gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #63 0x5563581912c8 in main /gecko/browser/app/nsBrowserApp.cpp:327:18
    #64 0x7f2a8374d0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #65 0x5563580dff69 in _start (/home/worker/builds/m-c-20211214042524-fuzzing-asan-opt/firefox+0x5cf69)
Flags: in-testsuite?
Group: dom-core-security

Tyson, could you please check what the behavior is on this test case from a build before bug 1741665 landed? That's not on beta or release so it would be good to know what happens there. I think the precise boundary changed a bit so it might not be bad. Thanks.

Flags: needinfo?(twsmith)

(In reply to Andrew McCreight [:mccr8] from comment #1)

Tyson, could you please check what the behavior is on this test case from a build before bug 1741665 landed?

It just seems to allocate the memory and then nothing.

Is it worth getting a Pernosco session for this?

Flags: needinfo?(twsmith)

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20211221214151-f16661bf49d3.
The bug appears to have been introduced in the following build range:

Start: 6c0d753b10f45d377d10f02992567641e9526fa9 (20211213215115)
End: 69575514d07e77820a6200835383fcad637bba57 (20211213231848)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6c0d753b10f45d377d10f02992567641e9526fa9&tochange=69575514d07e77820a6200835383fcad637bba57

Whiteboard: [bugmon:bisected,confirmed]
Has Regression Range: --- → yes

The array being passed in by the test case has length 2147483647, which is INT32_MAX.

MemoryBlobImpl::CreateInputStream() checks that the length is not greater than INT32_MAX, so that is okay. It then passes the data in to NS_NewByteInputStream() as a Span. That ends up in nsStringInputStream::ShareData(), which also does a sort of 32 bit signed int overflow check, but we pass that again. Then it tries to make a string out of it, but nsTStringLengthStorage is INT32_MAX - 1 (because it has to reserve a byte for a trailing null) so we crash.

I think MemoryBlobImpl::CreateInputStream() should throw on mLength >= INT32_MAX instead of mLength > INT32_MAX, and the check in nsStringInputStream::ShareData() should probably also throw if the length is too large. The other nsStringInputStream methods should probably also be checked, because they may have similar issues.

I guess this was okay before bug 1741665. nsTDependentSubString can represent strings of length INT32_MAX because it isn't null terminated. Maybe there's some issue elsewhere dealing with those strings, but that would be a bug in that other code, and hopefully we'd have already caught it if copying a max length dependent string to a non-dependent string went awry.

I guess the basic issue that this test case highlights is that while bug 1741665 makes sure that string lengths are compatible with the length of JS strings, apparently JS arrays can be a little bigger.

I think this isn't a security issue, but I'll keep it hidden because bug 1741665 is hidden.

Assignee: nobody → continuation
Keywords: sec-other
Component: XPCOM → DOM: File
Flags: in-testsuite? → in-testsuite+

Weird. Maybe allocating a giant array doesn't work well on Android.

I've disabled the test on Android. Hopefully it won't also fail on Win32, as I'm not sure how to skip a reftest there.

Flags: needinfo?(continuation)

It also looks like the chunk that probably would have been running the test is failing with TSan on my push, so to be safe I'll just skip it there.

Reject INT32_MAX length arrays in CreateInputStream. r=dom-workers-and-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/c6d5a42e4a5becf988e5905f31b183a0358fe326
https://hg.mozilla.org/mozilla-central/rev/c6d5a42e4a5b

Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox97: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20211224095019-6d1e19449b74.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
status-firefox97: fixed → verified
Keywords: bugmon

We should uplift this to ESR so that we can uplift bug 1741665.

status-firefox-esr91: unaffected → affected

Comment on attachment 9256714 [details]
Bug 1747185 - Reject INT32_MAX length arrays in CreateInputStream.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed to fix a crash regression in bug 1741665, which we want to eventually uplift to ESR. It doesn't need to go in an earlier release.
  • User impact if declined: Crash in a very specific scenario.
  • Fix Landed on Version: 97
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This changes the behavior of at least some DOM File APIs when you pass in a typed array with INT32_MAX elements, from it working to it not working, so that's a risk, but if somebody is doing that anyways then when we uplift bug 1741665 it'll turn into a crash anyways.
Attachment #9256714 - Flags: approval-mozilla-esr91?

Comment on attachment 9256714 [details]
Bug 1747185 - Reject INT32_MAX length arrays in CreateInputStream.

Approved for 91.6esr.

Attachment #9256714 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed][adv-esr91.6-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: