Closed Bug 1230341 Opened 4 years ago Closed 4 years ago

potential UAF in service worker use of NS_NewRunnableMethodWithArg

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- disabled
firefox44 + fixed
firefox45 + fixed
firefox-esr38 --- unaffected

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage])

Attachments

(1 file)

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.
Group: core-security → dom-core-security
Marking sec-high, because I don't think this is easily exploitable.
Keywords: sec-high
Fixed another case I found as well.
Attachment #8696031 - Flags: review?(ehsan)
Passes tests locally.
Attachment #8696031 - Flags: review?(ehsan) → review+
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.
Attachment #8696031 - Flags: sec-approval?
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.
Attachment #8696031 - Flags: approval-mozilla-aurora?
Comment on attachment 8696031 [details] [diff] [review]
Hold a strong ref in service worker NS_NewRunnableMethodWithArg uses. r=ehsan

Approvals given.
Attachment #8696031 - Flags: sec-approval?
Attachment #8696031 - Flags: sec-approval+
Attachment #8696031 - Flags: approval-mozilla-aurora?
Attachment #8696031 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/6c582cc65dc9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: dom-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.