1230341 - potential UAF in service worker use of NS_NewRunnableMethodWithArg

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Consider this code from ServiceWorkerManager.cpp:

    // Step 8 "Queue a task..." for updatefound.
    nsCOMPtr<nsIRunnable> upr =
      NS_NewRunnableMethodWithArg<ServiceWorkerRegistrationInfo*>(
        swm,
        &ServiceWorkerManager::FireUpdateFoundOnServiceWorkerRegistrations,
        mRegistration);

    NS_DispatchToMainThread(upr);

This dispatches a runnable without holding a strong ref to the mRegistration argument.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Marking sec-high, because I don't think this is easily exploitable.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Hold a strong ref in service worker NS_NewRunnableMethodWithArg uses. r=ehsan

Fixed another case I found as well.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Passes tests locally.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8696031 [details] [diff] [review]
Hold a strong ref in service worker NS_NewRunnableMethodWithArg uses. r=ehsan

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I think difficult.  Its hard to directly control the life cycle of these objects from script.  Even if this was possible, it would then need to force the race to fail.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Well, its obviously a potential UAF, but again I don't think its obviously exploitable.

Which older supported branches are affected by this flaw?

I believe we should fix this in 44.  Its probably been in the service worker code the entire time, but its disabled behind a pref on earlier branches.

If not all supported branches, which bug introduced the flaw?

Probably there from the first service worker manager impl.  Again, disabled on earlier branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should uplift cleanly to aurora 44.

How likely is this patch to cause regressions; how much testing does it need?

No risk.  Its simply holding a strong ref where the code expects the object to stay alive.  Tested locally using existing mochitests and wpt tests.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8696031 [details] [diff] [review]
Hold a strong ref in service worker NS_NewRunnableMethodWithArg uses. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: Probably minimal, since this seems hard to exploit.  It would be nice to close the UAF race, though, since we are shipping SW on 44.
[Describe test coverage new/current, TreeHerder]: Existing mochitests and wpt tests.
[Risks and why]: Minimal.  Service workers only.
[String/UUID change made/needed]: None.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8696031 [details] [diff] [review]
Hold a strong ref in service worker NS_NewRunnableMethodWithArg uses. r=ehsan

Approvals given.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

m-i:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6c582cc65dc9

m-a:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/eeaa77b2a1e0

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c582cc65dc9