1317927 - Media caching needs to use origin attributes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Dave Huseby [:huseby]]

in dom/media/MediaManager.cpp around #2519, it calls media::GetOriginKey to get a pledge with an origin "key" based off of a few parameters passed in.  One of them is whether or not we're in private browsing mode.

dom/media/MediaChild.cpp's GetOriginKey method needs to be updated to take origin attributes instead of flags.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - OA in MediaManager

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - GMP

Comment 3

[Andrea Marchesini [:baku]]

Attachment: EME

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8811334 [details] [diff] [review]
part 2 - GMP

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

Thanks for the patch. Please use MozReview for requesting review. It makes reviewing code easier.

::: dom/media/eme/MediaKeys.cpp
@@ +409,5 @@
>      return promise.forget();
>    }
>  
>    nsIDocument* doc = window->GetExtantDoc();
> +  PrincipalOriginAttributes attrs = nsContentUtils::GetOriginAttributes(doc);

Can this be const? We shouldn't be mutating the origin, right?

::: dom/media/gmp/GMPServiceParent.cpp
@@ +1420,5 @@
>    }
>  
> +  nsAutoString origin;
> +  origin.Assign(aOrigin);
> +  

Trailing whitespace.

@@ +1425,5 @@
> +  nsAutoCString suffix;
> +  aAttrs->CreateSuffix(suffix);
> +  origin.Append(NS_ConvertUTF8toUTF16(suffix));
> +
> +  const uint32_t hash = AddToHash(HashString(origin),

I assume this means that the value we hash to will change, and so existing storage records will now not be able to be accessed? Or is that only for records not in the default container?

If the generated hashes will change for origins in the default container, please increment the value of pref media.gmp.storage.version.expected so that we'll clear old orphaned storage records under the old hashing scheme.

::: dom/media/gmp/GMPServiceParent.h
@@ +43,5 @@
>                               bool *aRetVal) override;
>    NS_IMETHOD GetNodeId(const nsAString& aOrigin,
>                         const nsAString& aTopLevelOrigin,
>                         const nsAString& aGMPName,
> +                       PrincipalOriginAttributes* aOriginAttributes,

Can aOriginAttributes be const?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8811335 [details] [diff] [review]
EME

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

Please use MozReview for requesting reviews. It makes reviews easier, and means I don't have to go find the original code that's being modified before I can review changes to it, and it ensures the version I compare against is the same one you made changes against.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8811250 [details] [diff] [review]
part 1 - OA in MediaManager

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

This is WebRTC code, so Jesup should review or find a reviewer.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8811250 [details] [diff] [review]
part 1 - OA in MediaManager

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

r+ from me, forwarding to jib for a look

Comment 8

[Tanvi Vyas[:tanvi]]

Is this related to https://bugzilla.mozilla.org/show_bug.cgi?id=1283325?

Comment 9

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

Comment on attachment 8811250 [details] [diff] [review]
part 1 - OA in MediaManager

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

Thanks! Some questions...

::: dom/media/systemservices/MediaParent.cpp
@@ +405,5 @@
> +    origin.Assign(aOrigin);
> +
> +    nsAutoCString suffix;
> +    aAttrs.CreateSuffix(suffix);
> +    origin.Append(suffix);

This makes a deviceId unstable across realms of the same origin, unless ALL origin attributes match as well, is that the intent?

We have a spec to follow [1] but it is quite vague here, so anything that boosts privacy seems good to me in principle.

However, I think we'd naively expect a deviceId to be stable across the same scope that we persist gUM permissions to, and I notice in bug 1302047 we currently ignore userContextId and firstPartyDomain when matching permissions, though I see bug 1308607 comment 0 filed to change that. Can you comment on our short-term and long-term plans here?

Also, can we reuse any of this for bug 1299577? The mediacapture spec wants us to double-key origin for permissions even without "privacy.firstparty.isolate", because of the ease with which malicious sites can embed url-based rooms from popular webrtc sites that people tend to grant permission to.

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadeviceinfo-deviceid

Comment 10

[Ethan Tseng [:ethan]]

Baku, your patch uses Origin Attributes in MediaManager so I assume it also separates media cache by firstPartyDomain, is that correct?

Comment 11

[Andrea Marchesini [:baku]]

Yes it is. I want to apply the comments and land these patches for today.

Comment 12

[Ethan Tseng [:ethan]]

(In reply to Andrea Marchesini [:baku] from comment #11)
> Yes it is. I want to apply the comments and land these patches for today.

Thanks for confirmation!  :)

Comment 13

[Andrea Marchesini [:baku]]

Attachment: oa_1.patch

Fixing bug 1319045, all these 3 patches can be merged in 1 simpler patch.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8813217 [details] [diff] [review]
oa_1.patch

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

r?cpearce for the GMP/EME parts (look sane to me)

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8813217 [details] [diff] [review]
oa_1.patch

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

Please use mozreview for review requests. It makes developing patches and reviewing patches easier.

::: dom/media/gmp/mozIGeckoMediaPluginService.idl
@@ +160,5 @@
>                         in ACString nodeId,
>                         in GetGMPDecryptorCallback callback);
>  
>    /**
> +   * Gets the NodeId for a (origin, urlbarOrigin) duple.

s/duple/pair/ right?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed75a24f8b5
Media caching needs to use origin attributes, r=cpearce, r=jesup

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ed75a24f8b5