Closed Bug 1317927 Opened 3 years ago Closed 3 years ago

Media caching needs to use origin attributes

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: huseby, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [OA][tor][domsecurity-active])

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → amarchesini
Attached patch part 1 - OA in MediaManager (obsolete) — Splinter Review
Attachment #8811250 - Flags: review?(ajones)
Attached patch part 2 - GMP (obsolete) — Splinter Review
Attachment #8811334 - Flags: review?(ajones)
Attached patch EME (obsolete) — Splinter Review
Attachment #8811335 - Flags: review?(ajones)
Priority: -- → P1
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?
Attachment #8811334 - Flags: review?(ajones) → review+
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.
Attachment #8811335 - Flags: review?(ajones) → review+
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.
Attachment #8811250 - Flags: review?(ajones) → review?(rjesup)
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
Attachment #8811250 - Flags: review?(rjesup)
Attachment #8811250 - Flags: review?(jib)
Attachment #8811250 - Flags: review+
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
Attachment #8811250 - Flags: review?(jib) → review+
Baku, your patch uses Origin Attributes in MediaManager so I assume it also separates media cache by firstPartyDomain, is that correct?
Flags: needinfo?(amarchesini)
Yes it is. I want to apply the comments and land these patches for today.
Flags: needinfo?(amarchesini)
(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!  :)
Whiteboard: [OA] → [OA] [tor]
Whiteboard: [OA] [tor] → [OA][tor][domsecurity-active]
Attached patch oa_1.patchSplinter Review
Fixing bug 1319045, all these 3 patches can be merged in 1 simpler patch.
Attachment #8811250 - Attachment is obsolete: true
Attachment #8811334 - Attachment is obsolete: true
Attachment #8811335 - Attachment is obsolete: true
Attachment #8813217 - Flags: review?(rjesup)
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)
Attachment #8813217 - Flags: review?(rjesup)
Attachment #8813217 - Flags: review?(cpearce)
Attachment #8813217 - Flags: review+
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?
Attachment #8813217 - Flags: review?(cpearce) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed75a24f8b5
Media caching needs to use origin attributes, r=cpearce, r=jesup
https://hg.mozilla.org/mozilla-central/rev/4ed75a24f8b5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.