Closed Bug 1223773 Opened 7 years ago Closed 2 years ago

Limit pre-grant deviceIds to [BROWSINGCONTEXT]

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1528042
Tracking Status
firefox45 --- affected

People

(Reporter: jib, Assigned: jib)

References

()

Details

(Keywords: sec-low, Whiteboard: [fission-])

Attachments

(2 files, 4 obsolete files)

The language we introduced to the spec on deviceIds is narrower than our current implementation:

"As long as no local device has been attached to an active MediaStreamTrack in a page from this origin, and no persistent permission to access local devices has been granted to this origin, then this identifier MUST NOT be reusable after the end of the current browsing session." [1]

A misunderstanding, we currently persist these pre-grant deviceIds until browser close [2], when they should be cleared at the end of the *browsing session*, by which was meant "the time when the browsing context is navigated to another URL", which is earlier. This was lacking a good definition until somewhat recently [3].

This is a great improvement and we should implement it. Marking as hidden to get a head start on this, as it's not obvious that it isn't working like this already, which could be exploited.

---
[1] http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaDeviceInfo-deviceId

[2] except in Private Browsing mode where deviceIds do get cleared when the last Private Browsing window is closed at least (bug 1182354).

[3] https://github.com/w3c/mediacapture-main/issues/222#issuecomment-132578437
Duplicate of this bug: 1223793
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Assignee: nobody → jib
I don't know which mistake this really is: either in using browsing session, or in implementing something that persists on a longer timescale.  I think that I'm OK with this as it has been implemented, probably more so than as it has been defined. I think the definition we arrived at might be more accident than anything else, that text likely being mine and likely being proposed on the (mistaken) understanding that a browser session ends when the browser closes, not on navigation.

Having a new value for every navigation has a usability hazard: page asks for device X, and now can't ask for it again after navigation because the ID changed.  That's not perfect: we don't persist identifiers after closing the browser, even though we do persist cookies.  However, I'm prepared to accept that limitation.
I like that the spec definition is narrower, and that it's not tied to the arbitrary event of closing the browser. Feels more modern somehow, with today's hibernating laptops and app-lingering mobile OSes, where most people probably leave their browser running forever (I know I do), so from a privacy angle, doing this seems like a win.

Reacquiring deviceIds is cheap, and I'd argue persistence carries little meaning until a user has made a choice anyway.

The only tricky bit is that it's per origin, so I need to keep an open-count of windowIds, to not wipe deviceIds in use by a different open tab from the same origin, but it should work.
I don't see this as a privacy win: cookies can be used to track users over the same period.
I'll note however that you can drop all non-persistent storage and just feed the window ID into the HMAC inputs, if you want to fix this.
Resolved on irc. We're unlikely to reopen spec at this point. I can't use the windowId as input, since there may be more than one tab open from the same domain, so each one needs to be given the same temporary token in case one of them persists, so they can all keep using them without confusion. Patch coming up.
Attachment #8687394 - Flags: review?(rjesup)
Comment on attachment 8687394 [details] [diff] [review]
Refresh pre-grant deviceIds more often.

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

r+, though one question: how will this "last tab from origin" stuff going to interact with multi-content-process?
Attachment #8687394 - Flags: review?(rjesup) → review+
Good point, there's more than one MediaManager in that case.

This is already observable by opening an e10s window and a non-e10s window, and running enumerateDevices from both.

I tried it, and as you'd expect, deviceIds get reset too soon e.g. pages in the other process may still be relying on them not to change.

The solution is probably to move the opencount logic to the chrome process as well (though it still needs to be kept separate from the stored originKeys, so open-counts don't get out of sync when someone clears cookies).

Since this errs on the other side, most people wont be messing with e10s/non-e10s combinations, and the symptom is mild and non-privacy related, so I vote we land what we have here and file a follow-up non-security bug.

Does that sound good?
Flags: needinfo?(rjesup)
... I think that you are putting in quite a bit of effort here to fix something that doesn't actually need to be fixed.  I can propose a change to the spec that would be in place much faster.
I agree with MT. The intention when we discussed this originally was that the lifetime would semi-approximate cookie lifetime, which generally persists till browser close (if not longer). It's not clear to me what the benefit of clearing this out on page nav brings.
The benefit would be not introducing a new tracking concept that ATM is slightly superior to cookies.

deviceIds are not cookies, which makes them:

- Invisible to users and unaffected by built-in and any Add-on cookie management and/or policy tools.
- They have no timed expiry, once persisted.
- They don't currently follow third-party-cookie rules.
- "Forget about this site" doesn't clear them yet (Bug 1152448)

Yes, they're cleared by "Clear cookies" (since x), the "Forget" button, and "Clear cookies on shutdown".

On the spec, comment 10 seems optimistic to me, given that the github issue in comment 0 shows input from other members and chairs that demonstrate understanding of [BROWSINGCONTEXT] when talking about deviceIds. We're way past last-call, and this kind of thing seems like the last thing anyone wants to re-open.

I certainly wouldn't want anyone to reopen this on account of effort on my part, as this patch seems landable to me.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #12)
> The benefit would be not introducing a new tracking concept that ATM is
> slightly superior to cookies.
> 
> deviceIds are not cookies, which makes them:
> 
> - Invisible to users and unaffected by built-in and any Add-on cookie
> management and/or policy tools.

You are familiar with IndexedDB, right?


> - They have no timed expiry, once persisted.

And cookies can be made to live essentially forever.


> - They don't currently follow third-party-cookie rules.
> - "Forget about this site" doesn't clear them yet (Bug 1152448)

Well *these* seems like the relevant defects.

The easy design choice (and indeed the one we discussed a number of times)
is to simply have a hidden cookie that's hashed against the true device
ID, thus making it generally fate share against cookies.


> On the spec, comment 10 seems optimistic to me, given that the github issue
> in comment 0 shows input from other members and chairs that demonstrate
> understanding of [BROWSINGCONTEXT] when talking about deviceIds. We're way
> past last-call, and this kind of thing seems like the last thing anyone
> wants to re-open.
> 
> I certainly wouldn't want anyone to reopen this on account of effort on my
> part, as this patch seems landable to me.

As MT suggested, this patch has a number of negative side effects. More important
to just get it right.
I'm happy to argue that [BROWSINGCONTEXT] was an accident, and in error.
My main argument against this is that there is no need for deviceIds to have any broader scope.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> My main argument against this is that there is no need for deviceIds to have
> any broader scope.

But MT already gave you a reason:

"Having a new value for every navigation has a usability hazard: page asks for device X, and now can't ask for it again after navigation because the ID changed."
> "Having a new value for every navigation has a usability hazard: page asks
> for device X, and now can't ask for it again after navigation because the ID
> changed."

We're only talking about "pre-grant" deviceIds, i.e. sites for which the user has never granted gUM access.

Given this, what's an example where X has value that cannot be replicated by simply calling enumerateDevices again?
(In reply to Eric Rescorla (:ekr) from comment #13)
> IndexedDB

The spec on IndexedDB appears to have different privacy trade-offs, e.g. saying "User agents MAY require the user to authorize access to databases before a site can use the feature."

Mediacapture in contrast requires deviceIds to be "Without authorization (to the "drive-by web")".
I'm not aware of any limits on access to IndexedDB, localStorage, or HTTP cache existing in practice.
Group: core-security → media-core-security
Fixed the multi-process case.
Flags: needinfo?(rjesup)
Attachment #8688183 - Flags: review?(rjesup)
Comment on attachment 8688183 [details] [diff] [review]
Move tracking of same-origin windowIds to chrome process.

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

::: dom/media/systemservices/MediaParent.cpp
@@ +64,5 @@
> +      windowIds->AppendElement(aWindowId);
> +      MOZ_ASSERT(!mWindowIdToOrigin.Contains(aWindowId));
> +      mWindowIdToOrigin.Put(aWindowId, aOrigin);
> +    }
> +    

trailing spaces in a few spots
Attachment #8688183 - Flags: review?(rjesup) → review+
Rebased, and removed litter from last patch (two new unused declarations). Carrying forward r=jesup.
Attachment #8687394 - Attachment is obsolete: true
Attachment #8688485 - Flags: review+
Rebased, and removed trailing whitespace. Carrying forward r=jesup.
Attachment #8688183 - Attachment is obsolete: true
Attachment #8688486 - Flags: review+
Rebased.
Attachment #8688485 - Attachment is obsolete: true
Attachment #8691429 - Flags: review+
Should be good to land. Planning to mark checkin-needed by end of day.
Keywords: sec-low
Jib -- Can we land this?
Flags: needinfo?(jib)
Last I spoke with mt, we were not in sync on this being an improvement. I'll ask him to summarize the reasons here.
Flags: needinfo?(jib) → needinfo?(martin.thomson)
The claim is that this improves privacy.  I'm contesting that claim.  The change alters the lifetime of this one piece of origin-specific state in a way that isn't consistent and is therefore likely to cause issues.

First, privacy.  The set of deviceId values that are returned by enumerateDevices are a means of tracking a user agent over multiple visits.  This would, if it were the only thing that we persisted, represent a privacy hazard.  But we have cookies, localStorage, indexedDB, service workers, and much more.

The web relies on the stability of that state.  At the most recent TPAC meeting it was very much the consensus of the group that partial clearing of origin-specific state was a hazard.  On the one hand, it doesn't provide any protection against tracking, since a site can just shift data around.  But it can cause serious integrity issues with applications.  For an extreme example of the sort of integrity issues I'm talking about, read [1].

[1] <https://www.usenix.org/system/files/conference/usenixsecurity15/sec15-paper-zheng.pdf>

In this case, I think that the integrity issues are minor.  But so is the tracking potential.

If the intent is to have an optional feature where subsequent visits to a page appear to be from a completely different user agent (in the absence of fingerprinting), then we should talk about providing a control that allows that.  A partial fix for this one tiny piece of state isn't worthwhile in the absence of that plan.
Flags: needinfo?(martin.thomson)
(In reply to Martin Thomson [:mt:] from comment #10)
> I can propose a change to the spec that would be in place much faster.

(Cleaning out my P1s) Martin, do you want to propose fixing the spec here, and then we can close this as WONTFIX?
Flags: needinfo?(martin.thomson)
That would be nice.

I don't have the bandwidth for the PR, but I will open an issue.  I don't know who can un-sec this bug, but it might be helpful to have this discussion available when the spec change is proposed.
Flags: needinfo?(martin.thomson)
Group: media-core-security
Looks like this will be resolved in the spec instead. See comment 32.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Reopening as https://github.com/w3c/mediacapture-main/issues/322 was closed with no major change.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(In reply to Jan-Ivar Bruaroey [:jib] from comment #34)
> Reopening as https://github.com/w3c/mediacapture-main/issues/322 was closed
> with no major change.

Looks like the GH issue is closed. Is this still relevant?
Flags: needinfo?(jib)
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → ?
Fission Milestone: ? → ---

jib, can you please do something with this?

Flags: needinfo?(jib)
Flags: needinfo?(jib)
Whiteboard: [fission-]
Severity: normal → S2

Bug 1528042 should hopefully take care of this problem.

See Also: → 1528042
Severity: S2 → S3
Status: REOPENED → RESOLVED
Closed: 7 years ago2 years ago
Flags: needinfo?(jib)
Resolution: --- → DUPLICATE
Duplicate of bug: 1528042
You need to log in before you can comment on or make changes to this bug.