1811181 - (CVE-2023-28161) Giving the camera permission to a local .html file means giving this permission to all the local .html files opened in the same tab

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P2)

Description

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/109.0.0.0 Safari/537.36

Steps to reproduce:

Create two local .html files with two different names and the same code lines:

<div id="container">
    <h1><a href="//webrtc.github.io/samples/" title="WebRTC samples homepage">WebRTC samples</a>
        <span>getUserMedia</span></h1>

    <video id="gum-local" autoplay playsinline></video>
    <button id="showVideo">Open camera</button>

</div>

<script src="https://webrtc.github.io/samples/src/content/getusermedia/gum/js/main.js"></script>

Open the first .html file on Firefox, give the camera permission to the file, open the second .html file, the second .html file is granted the camera permission automatically.

Video of demonstration: please download from this link to watch a high-definition video: https://drive.google.com/file/d/1YSdgDrxYTQZm2xLn86Q2cLTfdvtgZXKe/view?usp=sharing.

Actual results:

Giving the camera permission to a local .html file means giving this permission to all the local .html files.

Expected results:

Should ask for giving the camera permission on each local .html file.

Comment 1

[Daniel Veditz [:dveditz]]

Paul: I may be misreading bug 1627210, but do we really support different permission settings for individual files? If not, wouldn't granting permanent permission lead to this issue permanently? Or is this something gUM is doing specific incorrectly?

Comment 2

[Emma Zühlcke [:emz]]

There are two permission managers involved here, SitePermissions(.jsm) and nsIPermissionManager.

nsIPermissionManager accepts file principals, it will key permissions by origin which means two different files will have different permission keys isolating the permissions between the files.

SitePermissions is a layer on top of nsIPermissionManager which has an extra in-memory store for temporary permissions. Temporary block permissions are keyed by base-domain while temporary allow permissions are keyed by pre-path.
Two different local file URIs still share the same pre-path file:// thus temporary allow grants are shared across all file URIs. I'm wondering if we should change this to use origin instead.

This bug should not affect permanent permissions only temporary ones. Those are stored per-browser (tab). That's why the bug STR does not work when you open the second local page in a new tab.

AFAIC the temporary allows are currently only used for WebRTC grace periods (Bug 1693677).

Comment 3

[Emma Zühlcke [:emz]]

Attachment: Bug 1811181 - Switch SitePermissions TemporaryPermissions and GloballyBlockedPermissions from prePath to origin key. r=prathiksha!

Comment 4

[Emma Zühlcke [:emz]]

I've submitted a WIP patch for how switching to origin scope for temporary ALLOW permissions could look like. There are probably some details we would still need to figure out, like checking for non content principals etc.
It would be nice to share some permission key logic with the permission manager, e.g. code aroundPermissionManager::PermissionKey::CreateFromPrincipal. This would make sure we align behavior.

Overall I'm not sure how critical this issue is given that it only affects temporary permissions and only local files. You could make the argument that it is unexpected for users to grant permission to one file and then have it apply to all local files. But then this would also be easy to be bypassed if the user saves a new file in the same location?
Dan, what do you think?

Also I'm looking at https://phabricator.services.mozilla.com/D167888. On fist glance this is probably fine given that persistent permissions end up in the permission manager anyway where they're handled correctly (key will include the file path).

Comment 5

[Daniel Veditz [:dveditz]]

So this bug could be re-titled "Giving the camera permission to a local .html file means giving this permission to any other local .html file you open in the same tab for an hour"?

If one page links to the next then we've got some kind of "local-disk" application (old-style web development not using a localhost server?) and that's not a terrible result at all--it's kind of acting like a "website". Or the user has a bookmark... those are not likely to be suspicious. One questionable case is if the first file was opened from a (download?) directory listing, and then the user hits back and picks another unrelated file on that list. It could happen, but would someone code a file to spy on you just hoping for that very narrow circumstance to happen?

That didn't seem like much of a problem with the original 50s timeout from bug 1693677, but privacy.webrtc.deviceGracePeriodTimeoutMs was bumped to an hour in bug 1697284. That's definitely long enough to give a user a nasty surprise, although the this still seems like a very unlikely scenario for an intentional attack.

I like where your patch is going. Using prePath is definitely problematic for anything security related like permissions. Does your patch need to deal with the use of prePath in the GloballyBlockedPermissions section? Or is that something for the bug 1627210 patch to deal with (the D167888 you linked above) since that's the one removing the checks that prevent using file:// urls there?

Comment 6

[Emma Zühlcke [:emz]]

Thanks!

GloballyblockedPermissions should probably be switched to origin too. I don't think bug 1627210 is really problematic because enabling that checkbox makes us store the permissions in the permission manager which keys correctly. A contributor has already submitted a patch for enabling the checkbox and I think this can land separately.

Since it's a bigger change perhaps we can land the SItePermissions origin key change in a separate non-sec bug. We don't have to mention local files explicitly. Would that work for you?

Comment 7

[Daniel Veditz [:dveditz]]

Since it's a bigger change perhaps we can land the SItePermissions origin key change in a separate non-sec bug.

A bigger change than what? You're talking about the patch in this bug (for temporary SitePermissions), right? By "bigger" do you mean it will affect more than just file:/// URLs? The "more" seems pretty edge-casey, and then wouldn't it be odd to land a public change without any tests for it?

Other cases that might be similarly broken but fixed by switching to origin: about:blank' documents, frames with about:srcdocand non-sandboxeddata:contents,blob:` documents. Unless those are currently prevented from setting permissions, the prepath on those is also problematic.

I'm not seeing what you'd gain from checking this in under a public bug (which is why I wonder if I'm misunderstanding the question), but it should be fine if you want to.

Comment 8

[Emma Zühlcke [:emz]]

Hmm, fair, sounds like we should keep it in this bug then. I'll look into getting the draft production ready.

Comment 9

[Emma Zühlcke [:emz]]

Moving to SitePermissions since that component is where we need to change the keying.

Comment 10

[Emma Zühlcke [:emz]]

Attachment: Bug 1811181 - Add a test case for temporary permissions for file URIs. r=prathiksha!

Depends on D167988

Comment 11

[Phabricator Automation]

Comment on attachment 9316624 [details]
Bug 1811181 - Add a test case for temporary permissions for file URIs. r=prathiksha!

Revision D169233 was moved to bug 1816420. Setting attachment 9316624 [details] to obsolete.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Switch SitePermissions TemporaryPermissions and GloballyBlockedPermissions from prePath to origin key. r=prathiksha
https://hg.mozilla.org/integration/autoland/rev/2c2060b124711a821522638fef996e436e572958
https://hg.mozilla.org/mozilla-central/rev/2c2060b12471

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:pbz, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[Emma Zühlcke [:emz]]

Comment on attachment 9314350 [details]
Bug 1811181 - Switch SitePermissions TemporaryPermissions and GloballyBlockedPermissions from prePath to origin key. r=prathiksha!

Beta/Release Uplift Approval Request

• User impact if declined: sec-moderate: Users granting a site permission (camera, microphone, geolocation etc) to a local file grant it to all local files temporarily. This may be unexpected.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See https://bugzilla.mozilla.org/show_bug.cgi?id=1811181#c0
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium risk because we change permission keying in SitePermissions which may have unintended side effects. However this aligns with how we already key in nsIPermissionManager so it's well understood.
• String changes made/needed:
• Is Android affected?: No

Comment 16

[Donal Meehan [:dmeehan]]

:pbz what's the impact on this for esr102?
It's a medium risk and doesn't graft cleanly - should we be looking to uplift this to esr102 after your consideration or should it be a wontfix?

Comment 17

[Ciprian Georgiu, Desktop QA]

I have reproduced this bug with STR from comment 0 on Win 10 x64, with an affected Nightly build (2023-01-25).

The issue is verified as fixed on latest Nightly 112.0a1, under Win 10 x64, macOS 11 and Ubuntu 18.04 x64.

Comment 18

[Emma Zühlcke [:emz]]

Same impact on ESR. Do we usually uplift sec-moderate? I'm asking because the uplift request has a field asking for special consideration if not sec-high or sec-crit. I don't have such a reason 🙂.

Comment 19

[Donal Meehan [:dmeehan]]

(In reply to Paul Zühlcke [:pbz] from comment #18)

Same impact on ESR. Do we usually uplift sec-moderate? I'm asking because the uplift request has a field asking for special consideration if not sec-high or sec-crit. I don't have such a reason 🙂.

Thanks for the response - we usually only aim to uplift sec-high/sec-critical but thought it worth reaching out.
Setting esr-102 to wontfix, and will follow up with the beta uplift approval.

Comment 20

[Khiem Tran]

Dear all,

So, I would ask does it has a reward?

Best regards,

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9314350 [details]
Bug 1811181 - Switch SitePermissions TemporaryPermissions and GloballyBlockedPermissions from prePath to origin key. r=prathiksha!

Approved for 111.0b5

Comment 22

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/755f46abde2c

Comment 23

[Emma Zühlcke [:emz]]

(In reply to Khiem Tran from comment #20)

Dear all,

So, I would ask does it has a reward?

Best regards,

I'm not very familiar with that process, but I've set the flag for someone to take a look.

Comment 24

[Ciprian Georgiu, Desktop QA]

This is also verified as fixed on Beta 111.0b5, across platforms: Win 10 x64, macOS 11 and Ubuntu 18.04 x64.

Comment 25

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt