Closed Bug 1439326 Opened 6 years ago Closed 6 years ago

Keep a strong reference to gInstance when dispatching a runnable in U2F callback functions

Categories

(Core :: DOM: Device Interfaces, enhancement)

59 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- disabled
firefox59 --- disabled
firefox60 + fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

(Keywords: csectype-uaf, sec-high)

Attachments

(1 file, 1 obsolete file)

We currently use NewNonOwningRunnableMethod() to dispatch runnables from e.g. u2f_register_callback():

https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/dom/webauthn/U2FHIDTokenManager.cpp#26

That's bad, if the website cancels the request in the meantime, or it times out, that would be a UAF.

It's relatively hard to trigger as it requires user interaction, i.e. a user touching a USB U2F token. OTOH, it might be easier to trigger by waiting for a timeout.

The simple solution here is to just use NewRunnableMethod() and thus keep a strong reference. We still need to make sure to destroy the underlying u2f-hid-rs manager at the right time, so we can start a new request while the old instance waits to be destroyed in the background.
There's some more cleanup in this patch, but the main changes are:

1) Use NewRunnableMethod() to keep a strong ref to gInstance, a U2FHIDTokenManager.

2) Call U2FHIDTokenManager::Drop() before releasing the instance in the U2FTokenManager so that we can destroy the u2f-hid-rs manager. All queued runnables from u2f-hid-rs callbacks will be no-ops after that. The former gInstance will be destroyed when the Handle{Register,Sign}Result function called by the runnable finishes.
Attachment #8952106 - Flags: review?(jjones)
Unfortunately, I think this rather meets the sec-high criteria.
Keywords: sec-moderatesec-high
Note: This code is disabled behind a pref in pre-60 versions.

Will review ASAP
Comment on attachment 8952106 [details] [diff] [review]
0001-Bug-1439326-Add-U2FTokenTransport-Drop-to-handle-U2F.patch

Review of attachment 8952106 [details] [diff] [review]:
-----------------------------------------------------------------

Per IRC, quite the footgun here. Patch looks good, but would expect that since several reviewers before me. IMO we should address the nit, but don't have to do that in this patch. r+ as-is if we address the nit in a future edit (or here). Great sleuthing.

::: dom/webauthn/U2FHIDTokenManager.cpp
@@ +81,1 @@
>    // Release gInstanceMutex before we call U2FManager::drop(). It will wait

Nit: This comment is now less obvious, since we reset mTransactionId first. Perhaps rewrite this to explain why we're releasing the mutex before resetting mTransactionId, freeing the manager, and dropping U2FManager?
Attachment #8952106 - Flags: review?(jjones) → review+
r=jcj

Moved `mTransactionId = 0` to the end of the function, so the comment above isn't confusing.
Attachment #8952106 - Attachment is obsolete: true
Attachment #8952171 - Flags: review+
Comment on attachment 8952171 [details] [diff] [review]
0001-Bug-1439326-Add-U2FTokenTransport-Drop-to-handle-U2F.patch, v2

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

I assume that s/NewNonOwningRunnableMetho/NewRunnableMethod is relatively easy to spot...

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

I tried not to do that. The only comment added here isn't referencing the actual problem.

Which older supported branches are affected by this flaw?

Only Nightly 60. WebAuthn is disabled in earlier versions.

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

Technically, it was introduced by bug 1388851 in 57, but we only enabled WebAuthn by default in 60.

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

Backporting would be easy, if we think it's worth doing.

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

Unlikely to cause any regressions.
Attachment #8952171 - Flags: sec-approval?
Comment on attachment 8952171 [details] [diff] [review]
0001-Bug-1439326-Add-U2FTokenTransport-Drop-to-handle-U2F.patch, v2

sec-approval+ for trunk.
Will we want to backport this at all to 59, even if it is disabled by default there?
Attachment #8952171 - Flags: sec-approval? → sec-approval+
Group: core-security → dom-core-security
(In reply to Al Billings [:abillings] from comment #7)
> Will we want to backport this at all to 59, even if it is disabled by
> default there?

I think it's not worth the effort.
https://hg.mozilla.org/mozilla-central/rev/443bc564024c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Group: dom-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.

Attachment

General

Created:
Updated:
Size: