1862723 - Intermittent SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:390:5 in strlen | single tracking bug

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

Filed by: ncsoregi [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=434725860&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Ha2ciwZWThqN6zrZDlR-kg/runs/0/artifacts/public/logs/live_backing.log

---

IPDL protocol error: Handler returned error code!
###!!! [Parent][DispatchAsyncMessage] Error: PBackgroundIDBDatabase::Msg_Blocked Processing error: message was deserialized, but the handler returned false (indicating failure)
=================================================================
==22395==ERROR: AddressSanitizer: heap-use-after-free on address 0x5060005aeea8 at pc 0x55708e056089 bp 0x7ffd55f038b0 sp 0x7ffd55f03078
READ of size 57 at 0x5060005aeea8 thread T0
PID 22395
    #0 0x55708e056088 in strlen /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:390:5
    #1 0x7f92505722bb in length /builds/worker/workspace/obj-build/dist/include/nsCharTraits.h:423:56
    #2 0x7f92505722bb in nsTDependentString /builds/worker/workspace/obj-build/dist/include/nsTDependentString.h:75:52
    #3 0x7f92505722bb in mozilla::dom::ContentParent::KillHard(char const*) /builds/worker/checkouts/gecko/dom/ipc/ContentParent.cpp:4535:22
    #4 0x7f9248cd8053 in operator() /builds/worker/checkouts/gecko/ipc/glue/BackgroundImpl.cpp:781:3
    #5 0x7f9248cd8053 in mozilla::detail::RunnableFunction<(anonymous namespace)::ParentImpl::KillHardAsync(mozilla::ipc::PBackgroundParent*, char const*)::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
[...]
freed by thread T8 (IPDL Background) here:
    #0 0x55708e0da9b6 in free /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:52:3
    #1 0x7f9248d37639 in ~nsTSubstring /builds/worker/workspace/obj-build/dist/include/nsTSubstring.h:326:21
    #2 0x7f9248d37639 in mozilla::ipc::IPCResult::FailImpl(mozilla::NotNull<mozilla::ipc::IProtocol*>, char const*, char const*) /builds/worker/checkouts/gecko/ipc/glue/ProtocolUtils.cpp:85:1
    #3 0x7f92503548c5 in mozilla::dom::indexedDB::PBackgroundIDBDatabaseParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundIDBDatabaseParent.cpp:502:52
    #4 0x7f9248ddf96a in mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundParent.cpp:2694:32
[...]
previously allocated by thread T8 (IPDL Background) here:
    #0 0x55708e0dac5e in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x7f9246fcb5eb in Alloc /builds/worker/checkouts/gecko/xpcom/string/nsStringBuffer.cpp:68:32
    #2 0x7f9246fcb5eb in nsTSubstring<char>::StartBulkWriteImpl(unsigned long, unsigned long, bool, unsigned long, unsigned long, unsigned long) /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:233:32
    #3 0x7f9246ff2fa5 in nsTSubstring<char>::AppendASCII(char const*, unsigned long, std::nothrow_t const&) /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:846:12
    #4 0x7f924700df5a in AppendASCII /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:808:7
    #5 0x7f924700df5a in PrintfAppend<char>::append(char const*, unsigned long) /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:1103:14
    #6 0x55708e1f8429 in emit /builds/worker/workspace/obj-build/dist/include/mozilla/Printf.h:110:12
    #7 0x55708e1f8429 in fill2 /builds/worker/checkouts/gecko/mozglue/misc/Printf.cpp:90:8
    #8 0x55708e1f8429 in mozilla::PrintfTarget::cvt_s(char const*, int, int, int) /builds/worker/checkouts/gecko/mozglue/misc/Printf.cpp:395:10
    #9 0x55708e1fb2f5 in mozilla::PrintfTarget::vprint(char const*, __va_list_tag*) /builds/worker/checkouts/gecko/mozglue/misc/Printf.cpp:1012:16
    #10 0x7f9246ff34ad in nsTSubstring<char>::AppendVprintf(char const*, __va_list_tag*) /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:1126:21
    #11 0x7f924702ca24 in nsPrintfCString::nsPrintfCString(char const*, ...) /builds/worker/workspace/obj-build/dist/include/nsPrintfCString.h:31:5
    #12 0x7f9248d375ab in mozilla::ipc::IPCResult::FailImpl(mozilla::NotNull<mozilla::ipc::IProtocol*>, char const*, char const*) /builds/worker/checkouts/gecko/ipc/glue/ProtocolUtils.cpp:67:19
    #13 0x7f92503548c5 in mozilla::dom::indexedDB::PBackgroundIDBDatabaseParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundIDBDatabaseParent.cpp:502:52
    #14 0x7f9248ddf96a in mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundParent.cpp:2694:32

Comment 1

[Andrew McCreight [:mccr8]]

This log is a bit of a mess, but I'm pretty sure this is a use-after-free inside IPC error handling code. Kind of weird!

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: cleaned up log extract

Comment 3

[Andrew McCreight [:mccr8]]

This is a use-after-free of a string being passed around for an IPC error, and the use happens right around when we're killing the process so it doesn't feel super exploitable.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It looks like the thread dispatch is here, and I think it'd be enough to change the capture to something like reason = nsCString(aReason) and adjust the use of it accordingly.

The general problem here, if I understand correctly, is that we have a toplevel actor (PBackground) owned by a thread that's different from the process's main actor (PContent), and it wants to kill/crash the child process on a channel error (IToplevelProtocol::ProcessingError, so it has to do a thread hop, and one of the parameters is a borrowed string which needs to become owned in that case. I don't know if we have any other instances of this pattern currently.

(mccr8 also noticed that we're passing borrowed/dependent strings to AnnotateCrashReport, but that seems to be fine (as long as the pointer is valid during the call), because crashReporterAPIData_Table's elements are nsCString which will copy the string when assigned to).

Comment 5

[Andrew McCreight [:mccr8]]

ParentImpl::KillHardAsync was added in bug 1855138.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1855138

:janv, since you are the author of the regressor, bug 1855138, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 7

[Jan Varga [:janv]]

I guess the string needs to be copied, probably here https://searchfox.org/mozilla-central/rev/02841791400cf7cf5760c0cfaf31f5d772624253/ipc/glue/BackgroundImpl.cpp#783

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1855138

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1862723 - Use nsCString types for BackgroundParent::KillHardAsync, r=#ipc-reviewers

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1862723 - Use nsCString types for BackgroundParent::KillHardAsync, r=#ipc-reviewers

Original Revision: https://phabricator.services.mozilla.com/D194548

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8a1e52bfc49
Use nsCString types for BackgroundParent::KillHardAsync, r=ipc-reviewers,mccr8

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c8a1e52bfc49

Comment 14

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: Potential string UAF when building minidumps for killed processes
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Fix verified in Nightly: yes
• Code covered by automated testing: no
• Explanation of risk level: Low risk code transformation which eliminates a potential UAF.
• Is Android affected?: yes
• String changes made/needed: None
• Needs manual QE test: no

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9365658 [details]
Bug 1862723 - Use nsCString types for BackgroundParent::KillHardAsync, r=#ipc-reviewers

Approved for 121.0b5.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b886155d8f1

Comment 17

[Gabriele Svelto [:gsvelto]]

FYI this was spotted by PHC too, see this crash from yesterday.

Comment 18

[Andrew McCreight [:mccr8]]

Neat! That crash has the comment "Facebook ALWAYS crashes!" which is kind of weird, given the issue.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 21

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter