Valgrind warning about System Messages being used after free on b2g18

RESOLVED WORKSFORME

Status

defect
RESOLVED WORKSFORME
7 years ago
2 years ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

({csectype-uaf, sec-critical})

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [fixed in bug 803688])

==231== Invalid write of size 4
==231==    at 0x5AE679C: mozilla::dom::ContentParent::RecvSystemMessageHandled() (in /system/b2g/libxul.so)
==231==    by 0x5B383CB: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B025CB: mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B07B85: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (in /system/b2g/libxul.so)
==231==    by 0x5AE668F: RunnableMethod<mozilla::dom::TabChild, void (mozilla::dom::TabChild::*)(), Tuple0>::Run() (in /system/b2g/libxul.so)
==231==    by 0x5B063E1: mozilla::ipc::RPCChannel::DequeueTask::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C47EF1: MessageLoop::RunTask(Task*) (in /system/b2g/libxul.so)
==231==    by 0x5C48EEF: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (in /system/b2g/libxul.so)
==231==    by 0x5C49BB1: MessageLoop::DoWork() (in /system/b2g/libxul.so)
==231==    by 0x5B05D19: mozilla::ipc::DoWorkRunnable::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C2352F: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==231==    by 0x5C00977: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)
==231==  Address 0xdd26218 is 8 bytes inside a block of size 28 free'd
==231==    at 0x48062B0: free (vg_replace_malloc.c:446)
==231==    by 0x60E4247: moz_free (in /system/b2g/libxul.so)
==231==    by 0x5AE6965: mozilla::dom::(anonymous namespace)::SystemMessageHandledListener::Release() (in /system/b2g/libxul.so)
==231==    by 0x53D0665: nsRefPtr<imgRequest>::assign_assuming_AddRef(imgRequest*) (in /system/b2g/libxul.so)
==231==    by 0x579E335: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (in /system/b2g/libxul.so)
==231==    by 0x5C25609: nsTimerImpl::Fire() (in /system/b2g/libxul.so)
==231==    by 0x5C2567F: nsTimerEvent::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C2352F: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==231==    by 0x5C00977: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)
==231==    by 0x5B05E3B: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (in /system/b2g/libxul.so)
==231==    by 0x5C47E8D: MessageLoop::RunInternal() (in /system/b2g/libxul.so)
==231==    by 0x5C47F6D: MessageLoop::Run() (in /system/b2g/libxul.so)

This is an optimized build... RecvSystemMessageHandled calls SystemMessageHandledListener::OnSystemMessageHandled() so I think the bad part is somewhere in here:

    class SystemMessageHandledListener
    {
    ...
        static void OnSystemMessageHandled()
        {
            if (!sListeners) {
                return;
            }

            SystemMessageHandledListener* listener = sListeners->popFirst();
            if (!listener) {
                return;
            }

            // Careful: ShutDown() may delete |this|.
            listener->ShutDown();
        }
    ...
    };

It may be that sListeners holds SystemMessageHandledListener objects that have been deleted.

I triggered this by using the notification panel and repeatedly enabling/disabling bluetooth, wifi, and airplane mode in a somewhat random fashion.
blocking-b2g: --- → tef?
Maybe this is somehow related to WakeLock's:

==231== Invalid read of size 1
==231==    at 0x5765506: mozilla::dom::power::WakeLock::Unlock() (in /system/b2g/libxul.so)
==231==    by 0x5AE6755: mozilla::dom::(anonymous namespace)::SystemMessageHandledListener::ShutDown() (in /system/b2g/libxul.so)
==231==    by 0x5AE67A1: mozilla::dom::ContentParent::RecvSystemMessageHandled() (in /system/b2g/libxul.so)
==231==    by 0x5B383CB: mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B025CB: mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const&) (in /system/b2g/libxul.so)
==231==    by 0x5B07B85: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (in /system/b2g/libxul.so)
==231==    by 0x5AE668F: RunnableMethod<mozilla::dom::TabChild, void (mozilla::dom::TabChild::*)(), Tuple0>::Run() (in /system/b2g/libxul.so)
==231==    by 0x5B063E1: mozilla::ipc::RPCChannel::DequeueTask::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C47EF1: MessageLoop::RunTask(Task*) (in /system/b2g/libxul.so)
==231==    by 0x5C48EEF: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (in /system/b2g/libxul.so)
==231==    by 0x5C49BB1: MessageLoop::DoWork() (in /system/b2g/libxul.so)
==231==    by 0x5B05D19: mozilla::ipc::DoWorkRunnable::Run() (in /system/b2g/libxul.so)
==231==  Address 0xdd24050 is 24 bytes inside a block of size 56 free'd
==231==    at 0x48062B0: free (vg_replace_malloc.c:446)
==231==    by 0x60E4247: moz_free (in /system/b2g/libxul.so)
==231==    by 0x57657CB: mozilla::dom::power::WakeLock::~WakeLock() (in /system/b2g/libxul.so)
==231==    by 0x55CECE9: nsDOMParser::Release() (in /system/b2g/libxul.so)
==231==    by 0x538029F: nsCollation::~nsCollation() (in /system/b2g/libxul.so)
==231==    by 0x5AE695F: mozilla::dom::(anonymous namespace)::SystemMessageHandledListener::Release() (in /system/b2g/libxul.so)
==231==    by 0x53D0665: nsRefPtr<imgRequest>::assign_assuming_AddRef(imgRequest*) (in /system/b2g/libxul.so)
==231==    by 0x579E335: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (in /system/b2g/libxul.so)
==231==    by 0x5C25609: nsTimerImpl::Fire() (in /system/b2g/libxul.so)
==231==    by 0x5C2567F: nsTimerEvent::Run() (in /system/b2g/libxul.so)
==231==    by 0x5C2352F: nsThread::ProcessNextEvent(bool, bool*) (in /system/b2g/libxul.so)
==231==    by 0x5C00977: NS_ProcessNextEvent_P(nsIThread*, bool) (in /system/b2g/libxul.so)
I'm pretty sure this will end up calling a virtual function on deleted memory (Shutdown() calls AddRef() immediately). s-s.
Group: core-security
(In reply to ben turner [:bent] from comment #2)
> I'm pretty sure this will end up calling a virtual function on deleted
> memory (Shutdown() calls AddRef() immediately). s-s.

So would we call this an sec-critical bug? Outside of security, what's the user impact here?
bent and I talked about this on irc.  Given that he's not seeing this on m-c, I'm pretty sure the issue is caused by the fact that bug 803688 is not on b2g18.

We can add a remove() call to the destructor here, or backport the fix.
I'd rather backport bug 803688, so that we have consistent semantics across our development branches.
(In reply to Alex Keybl [:akeybl] from comment #3)
> So would we call this an sec-critical bug?

I'm out of the loop here, better to have a security team member answer that.

> Outside of security, what's the user impact here?

Crashes! Potentially any time we handle a system message (receive a call, get an sms, etc.).
This can cause exploitable crashes whenever we receive a call or SMS.  We have a simple, safe fix in bug 803688.

Please a+ that patch ASAP.
Thanks all, we'll take bug 803688 in that case and not tef+ this bug specifically.
blocking-b2g: tef? → -
Whiteboard: [fixed in bug 803688]
Depends on: 803688
This should be fixed now if bug 803688 did the trick on the b2g18 branch. Can you confirm and close the bug, Ben?
Assignee: nobody → bent.mozilla
Flags: needinfo?(bent.mozilla)
Yep, my testing show that this looks good now. Thanks Justin!
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(bent.mozilla)
Resolution: --- → WORKSFORME
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.