Closed Bug 1268726 Opened 4 years ago Closed 3 years ago

isolate shared worker by first party domain (Tor 15564)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: huseby, Assigned: huseby)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [tor][domsecurity-active][ETA 10/10][OA])

Attachments

(1 file, 5 obsolete files)

the tor browser does isolation of shared workers.  this bug is to implement the same isolation, starting with the Tor Browser patch and refactoring it to use origin attributes.
Attached patch the Tor Browser patch (WIP) (obsolete) — Splinter Review
Here's a link that tracks Tor Browser's latest version of the patch:
https://torpat.ch/15564
Dave, since you already uploaded a patch for that I am assigning this bug to.
Assignee: nobody → huseby
Whiteboard: [tor][OA] → [tor][OA][domsecurity-active]
Status: NEW → ASSIGNED
Per baku, Shared Workers already take OriginAttributes into account.
Priority: -- → P1
Whiteboard: [tor][OA][domsecurity-active] → [tor][domsecurity-active]
(In reply to Tanvi Vyas [:tanvi] from comment #4)
> Per baku, Shared Workers already take OriginAttributes into account.

I have tested shared workers in the Bug 1264593, it shows that shared workers are not segregated by the OriginAttributes. I think the reason is that the key of the shared worker does not reference originAttributes [1]. This could be fixed by adding originAttributes as a part of the key.

[1] http://searchfox.org/mozilla-central/source/dom/workers/RuntimeService.cpp#241
Whiteboard: [tor][domsecurity-active] → [tor][domsecurity-active][ETA 10/10]
See Also: → 1294336
Summary: isolate shared worker by first party domain → isolate shared worker by first party domain (Tor 15564)
Duplicate of this bug: 1294336
No longer blocks: meta_tor
Blocks: 1264593
Attached patch WIP Bug_1268726.patch (obsolete) — Splinter Review
WIP
Attachment #8746855 - Attachment is obsolete: true
changing to HG patch format.
Attachment #8789090 - Attachment is obsolete: true
(In reply to Dave Huseby [:huseby] from comment #9)
> Created attachment 8789102 [details] [diff] [review]
> WIP Bug_1268726.patch
> 
> changing to HG patch format.

Is this patch on top of the patches that landed in Bug 1294336 by Tim?  Was the work in that bug not sufficient to isolate shared workers by all Origin Attributes?
Flags: needinfo?(tihuang)
Flags: needinfo?(huseby)
Hi Tanvi,

I took over the bug that Tim had and closed it as a duplicate.  His patch, though r+, was not landed and was dropped in favor of my implementation for various reasons.  After fighting with mercurial and review board, I'm back to git and raw patches.  That's why this seems like it is a patch that is showing up late to the show.  Sorry for the confusion.

--dave
Flags: needinfo?(tihuang)
Flags: needinfo?(huseby)
Attachment #8789102 - Attachment description: WIP Bug_1268726.patch → Isolates shared workers by origin attributes.
Attachment #8789102 - Flags: review?(amarchesini)
Comment on attachment 8789102 [details] [diff] [review]
Isolates shared workers by origin attributes.

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

::: dom/workers/WorkerPrivate.cpp
@@ +4122,5 @@
>      loadInfo.mDomain = aParent->Domain();
>      loadInfo.mFromWindow = aParent->IsFromWindow();
>      loadInfo.mWindowID = aParent->WindowID();
>      loadInfo.mStorageAllowed = aParent->IsStorageAllowed();
> +    loadInfo.mPrivateBrowsing = aParent->IsInPrivateBrowsing(); // looks at origin attriubtes

1. attributes
2. can we remove this completely? Probably yes. Do you mind to file a follow up?

::: dom/workers/WorkerPrivate.h
@@ +787,5 @@
> +  }
> +
> +  const PrincipalOriginAttributes&
> +  OriginAttributesRef() const
> +  {

MOZ_ASSERT(NS_IsMainThread());
Attachment #8789102 - Flags: review?(amarchesini) → review+
Blocks: 1302566
Filed a follow up bug to remove the IsInPrivateBrowsing: Bug 1302566

Fixed the nit to add MOZ_ASSERT(NS_IsMainThread());

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=890e5405e361
Attached patch Bug_1268726.patch (obsolete) — Splinter Review
r+ :baku already.  fixed the nit.
Attachment #8789102 - Attachment is obsolete: true
Attachment #8791020 - Flags: review+
Keywords: checkin-needed
That new assert you added is firing like crazy on the last Try push. Please verify that the run is green before requesting checkin.
Keywords: checkin-needed
MOZ_ASSERT(NS_IsMainThread()); This assert?
Then we have a problem! mPrincipal can not be touched outside the main-thread. I guess you need to cache the full OriginAttributes in the loadInfo.
Adding OA tag since this is for Origin Attributes in general, and not just first party isolation.
Whiteboard: [tor][domsecurity-active][ETA 10/10] → [tor][domsecurity-active][ETA 10/10][OA]
Hrm...will modify the patch and do a new try ASAP.
This patch includes my first pass at removing IsInPrivateBrowsing functions from the shared worker state and adds access to origin attributes of a doc/channel via UserContentUtils.  This also adds caching of origin attributes in the working load info and the worker private structs to avoid accessing the principal so the code works in both parent and child processes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31eace6141b
Attachment #8791020 - Attachment is obsolete: true
Comment on attachment 8792011 [details] [diff] [review]
patch that includes removing IsInPrivateBrowsing functions

Baku,

I've decided to stop here in this bug so that this can get landed before the fork.  I left the IsInPrivateBrowsing in nsContentUtils because there are other subsystems that rely on it.  I will remove that and refactor the other dependent systems in Bug 1302566
Attachment #8792011 - Flags: review?(amarchesini)
Attachment #8792011 - Attachment description: WIP patch that includes removing IsInPrivateBrowsing functions → patch that includes removing IsInPrivateBrowsing functions
Comment on attachment 8792011 [details] [diff] [review]
patch that includes removing IsInPrivateBrowsing functions

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

::: dom/base/nsContentUtils.h
@@ +788,5 @@
>    /**
> +   * Returns origin attributes of the document.
> +   **/
> +  static mozilla::PrincipalOriginAttributes
> +  OriginAttributesRef(nsIDocument* aDoc);

This is not a reference. Call it GetOriginAttributes or OriginAttributes.

@@ +794,5 @@
> +  /**
> +   * Returns origin attributes of the load group.
> +   **/
> +  static mozilla::PrincipalOriginAttributes
> +  OriginAttributesRef(nsILoadGroup* aLoadGroup);

same here.

::: dom/workers/ScriptLoader.cpp
@@ +410,5 @@
>    nsCOMPtr<nsIGlobalObject> mSandboxGlobalObject;
>    nsTArray<RefPtr<CacheScriptLoader>> mLoaders;
>  
>    nsString mCacheName;
> +  PrincipalOriginAttributes mAttrs;

mOriginAttributes;

@@ +1478,5 @@
>    mCacheStorage =
>      CacheStorage::CreateOnMainThread(mozilla::dom::cache::CHROME_ONLY_NAMESPACE,
>                                       mSandboxGlobalObject,
> +                                     aPrincipal,
> +                                     false, /* can't be true, won't get here. */

/* privateBrowsing cannot be true here */ or... I mean, write 'privateBrowsing'

::: dom/workers/ServiceWorkerInfo.h
@@ +30,5 @@
>    const nsCString mScope;
>    const nsCString mScriptSpec;
>    const nsString mCacheName;
>    ServiceWorkerState mState;
> +  PrincipalOriginAttributes mAttrs;

also here. mOriginAttributes;

@@ +105,5 @@
>      return mState;
>    }
>  
> +  const PrincipalOriginAttributes&
> +  OriginAttributesRef() const

OriginAttributes() const

::: dom/workers/WorkerPrivate.cpp
@@ +1742,2 @@
>    , mServiceWorkersTestingInWindow(false)
> +  , mAttrs()

This should be needed, right?

::: dom/workers/WorkerPrivate.h
@@ +783,5 @@
>      return mLoadInfo.mStorageAllowed;
>    }
>  
> +  const PrincipalOriginAttributes&
> +  OriginAttributesRef() const

Nit: up to you, but maybe OriginAttributes() ?

@@ +788,2 @@
>    {
> +    return mLoadInfo.mAttrs;

mOriginAttributes

::: dom/workers/Workers.h
@@ +272,2 @@
>    bool mServiceWorkersTestingInWindow;
> +  PrincipalOriginAttributes mAttrs;

call it mOriginAttributes
Attachment #8792011 - Flags: review?(amarchesini) → review+
r+ baku, updated patch with review fixes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a6d87500be
Attachment #8792011 - Attachment is obsolete: true
Attachment #8792751 - Flags: review+
The try looks good.  Flagging for check in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b278f56017
isolate shared worker by first party domain. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39b278f56017
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.