1497141 - FeaturePolicy: camera and microphone

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

FeaturePolicy support for camera and microphone

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - camera

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - microphone

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - WPTs

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9015198 [details] [diff] [review]
part 1 - camera

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

I'd like to see it again over the doc check.

::: dom/media/MediaManager.cpp
@@ +2891,5 @@
> +        if (Preferences::GetBool("media.getusermedia.camera.deny", false)) {
> +          videoPerm = nsIPermissionManager::DENY_ACTION;
> +        }
> +
> +        if (videoPerm == nsIPermissionManager::UNKNOWN_ACTION) {

Nit: any reason not to prefer a simpler `} else {` here?

@@ +2894,5 @@
> +
> +        if (videoPerm == nsIPermissionManager::UNKNOWN_ACTION) {
> +          nsIDocument* doc = aWindow->GetExtantDoc();
> +          if (!doc ||
> +              !dom::FeaturePolicyUtils::IsFeatureAllowed(doc, NS_LITERAL_STRING("camera"))) {

If doc is nullptr here I think we want to err on the side of DENY_ACTION, to avoid potential timing exploits.

Or better, don't check doc at all and crash instead, since there's some precedence here for that [1]

Or if you prefer, make `doc` invariant by creating it earlier in this function and bail there if it's nullptr (or MOZ_RELEASE_ASSERT).

[1] https://searchfox.org/mozilla-central/rev/5786b9be9f887ed371c804e786081b476a403104/dom/media/MediaManager.cpp#3320

@@ +2899,5 @@
> +            videoPerm = nsIPermissionManager::DENY_ACTION;
> +          }
> +        }
> +
> +        if (videoPerm == nsIPermissionManager::UNKNOWN_ACTION) {

...then we could use `} else {` here too.

@@ +2908,3 @@
>        } else {
>          rv = permManager->TestExactPermissionFromPrincipal(
> +          principal, "screen", &videoPerm);

Ideally, we'd want feature policy to control screen sharing as well:

    dom::FeaturePolicyUtils::IsFeatureAllowed(doc, NS_LITERAL_STRING("display")

...if that's ready? https://github.com/w3c/permissions/issues/182

We can add that later too I suppose.

@@ +4141,5 @@
>      }
> +
> +    nsIDocument* doc = window->GetExtantDoc();
> +    if (!doc ||
> +        !dom::FeaturePolicyUtils::IsFeatureAllowed(doc, NS_LITERAL_STRING("camera"))) {

Same here, we should either crash, bail early (with false), or err on the side of DENY if `doc` is null.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9015199 [details] [diff] [review]
part 2 - microphone

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

Ah, in this patch you use the pattern I meant, but would like it in GetUserMedia() also.

::: dom/media/MediaManager.cpp
@@ +2879,5 @@
>            Preferences::GetBool("media.getusermedia.microphone.deny", false)) {
>          audioPerm = nsIPermissionManager::DENY_ACTION;
>        } else {
> +        nsIDocument* doc = aWindow->GetExtantDoc();
> +        if (!doc ||

same here.

@@ +4146,5 @@
> +
> +  auto* principal = window->GetPrincipal();
> +  if (NS_WARN_IF(!principal)) {
> +    return false;
> +  }

Like this!

@@ +4154,5 @@
>    {
> +    if (!dom::FeaturePolicyUtils::IsFeatureAllowed(doc, NS_LITERAL_STRING("microphone"))) {
> +      audio = nsIPermissionManager::DENY_ACTION;
> +    } else {
> +      rv = mgr->TestExactPermissionFromPrincipal(principal, "microphone", &audio);

Just curious, did you consider putting the feature policy check inside TestExactPermissionFromPrincipal? Maybe with doc as required arg?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9015200 [details] [diff] [review]
part 3 - WPTs

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

Awesome!

Comment 7

[Andrea Marchesini [:baku]]

> ...if that's ready? https://github.com/w3c/permissions/issues/182
> 
> We can add that later too I suppose.

Right. Another one we should add is 'speaker'. Maybe I'll ping you on IRC to know what's the best way to do it. AudioChannelServie maybe?

Comment 8

[Andrea Marchesini [:baku]]

> Just curious, did you consider putting the feature policy check inside
> TestExactPermissionFromPrincipal? Maybe with doc as required arg?

This is a very good idea! I'll think about it as a follow up.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 1 - camera

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - microphone

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9015448 [details] [diff] [review]
part 2 - microphone

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

::: dom/media/MediaManager.cpp
@@ +2887,5 @@
> +          audioPerm = nsIPermissionManager::DENY_ACTION;
> +        } else {
> +          rv = permManager->TestExactPermissionFromPrincipal(
> +            principal, "microphone", &audioPerm);
> +          NS_ENSURE_SUCCESS(rv, rv);

Looks good, just noting a behavior change here.

Before this patch, the MediaSourceEnum::Microphone test applied only to the deny pref, and we ran the "microphone" persisted permission test for all audio types.

This patch changes that behavior to skip the "microphone" persisted permission test for the behind-pref MediaSourceEnum::AudioCapture. 

This is good. If ever re-enabled, new thinking is MediaSourceEnum::AudioCapture would be covered under getDisplayMedia where audio is covered by the same permission as video. [1]

I can either open a new bug on this, or feel free to add the test for "screen" similar to how you do it in the other patch (we should rename "screen" to "display" at some point I suppose, but that might require some more thought).

[1] https://github.com/w3c/permissions/issues/182

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7757f7200917
FeaturePolicy: camera, r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f04193768a
FeaturePolicy: microphone, r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8121fadb5b8
FeaturePolicy: camera, microphone - WPTs, r=jib

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/7757f7200917
https://hg.mozilla.org/mozilla-central/rev/a3f04193768a
https://hg.mozilla.org/mozilla-central/rev/e8121fadb5b8

Comment 14

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/camera
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Feature-Policy/microphone