Closed Bug 1264567 Opened 4 years ago Closed 3 years ago

Tests for first party isolation of localStorage (Tor 13749.1)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jhao, Assigned: jhao)

References

(Blocks 3 open bugs)

Details

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

Attachments

(1 file, 12 obsolete files)

2.60 KB, patch
jhao
: review+
Details | Diff | Splinter Review
Attached patch the Tor Browser patch (WIP) (obsolete) — Splinter Review
Tor Browser tests for first party isolation of localStorage.
https://torpat.ch/13749
Summary: Regression tests for first party isolation of localStorage (Tor 13749) → Tests for first party isolation of localStorage (Tor 13749)
Whiteboard: [tor][OA] → [tor][OA][domsecurity-backlog]
Depends on: containers_testing
No longer depends on: containers_testing
Whiteboard: [tor][OA][domsecurity-backlog] → [tor][OA-testing][domsecurity-backlog]
Whiteboard: [tor][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog]
Priority: -- → P1
Depends on: 1289319
Priority: P1 → P3
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog1]
These tests failed as expected on current mozilla-central, but at least they terminate. When I applied Dave's patch in bug 1260931, these tests won't finish. At its 5th step, the html page doesn't receive the message from its iframe.

Dave, do you have any idea why?
Attachment #8741278 - Attachment is obsolete: true
Flags: needinfo?(huseby)
I traced further and the problem is: postMessage compares the target window's principal with the provided principal.  The provided principal is the caller's provided origin uri plus the subject principal's origin attributes.  When the first party pref is on, the target window's principal has firstPartyDomain attribute, but the subject principal does not.

Dave, it seems your patch in bug 1260931 breaks postMessage. Will you fix that in the bug, or should we open a follow-up bug?
Here[1] it uses the subject principal's origin attributes.  It doesn't have firstPartyDomain, but the target window does.  And in [2], it checks if the principals are equal to each other.

[1] http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8371
[2] http://searchfox.org/mozilla-central/source/dom/base/PostMessageEvent.cpp#107
Previous patch has a wrong diff.
Attachment #8776542 - Attachment is obsolete: true
Attached patch Isolation tests of localStorage. (obsolete) — Splinter Review
I rewrite the test with Tim's framework in bug 1289319, which doesn't use postMessage.  The result shows that the WIP patch in bug 1260931 doesn't isolate localStorage.  I'll test manually later.
I double checked with some manual testing using http://people.mozilla.org/~jhao/localStorageParent.html, and I'm sure that localStorage is not isolated by firstParty.

I also printed the iframes' node principals. Their origin attributes don't have first party domain.
Assignee: nobody → jhao
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1] → [tor-testing][OA-testing][domsecurity-active]
This one-line patch sets the docShell's origin attributes's firstPartyDomain the same as its parent document's.  With this patch, iframes gets the firstPartyDomain, and the above tests will pass.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [tor-testing][OA-testing][domsecurity-active] → [tor-testing][OA-testing][domsecurity-active][ETA 10/10]
Yoshi has the firstPartyDomain bug now.  Do you still need anything from me?
Flags: needinfo?(huseby)
(In reply to Dave Huseby [:huseby] from comment #8)
> Yoshi has the firstPartyDomain bug now.  Do you still need anything from me?

Jonathan should try the test again using Yoshi's patches.
Priority: P1 → P2
(In reply to Ethan Tseng [:ethan] from comment #9)
> (In reply to Dave Huseby [:huseby] from comment #8)
> > Yoshi has the firstPartyDomain bug now.  Do you still need anything from me?
> 
> Jonathan should try the test again using Yoshi's patches.

The tests will pass with Yoshi's patches.
(In reply to Jonathan Hao [:jhao] from comment #2)
> I traced further and the problem is: postMessage compares the target
> window's principal with the provided principal.  The provided principal is
> the caller's provided origin uri plus the subject principal's origin
> attributes.  When the first party pref is on, the target window's principal
> has firstPartyDomain attribute, but the subject principal does not.
> 
> Dave, it seems your patch in bug 1260931 breaks postMessage. Will you fix
> that in the bug, or should we open a follow-up bug?

I also tried Tor's original tests with Yoshi's patches.  After the 1st party pref is turned on, the iframe can postMessage to its parent, but the parent page cannot postMessage to its opener. That's because the opener, test_localStorageByFirstParty.html, was opened before the 1st party pref is on, and hence it doesn't have the firstPartyDomain attribute.

Yoshi says there will be a bug that reopens every tab when the 1st party pref is turned on.  That'll probably fix this issue, or we could use browser test to turn on the pref before the test page is loaded.
Comment on attachment 8777781 [details] [diff] [review]
Set firstPartyDomain in MaybeCreateDocShell.

No longer need this patch since Yoshi's patches work.
Attachment #8777781 - Attachment is obsolete: true
Attachment #8777191 - Attachment description: regression tests for first party isolation of localStorage → Tor's original tests (for comparison)
Attached patch Isolation tests of localStorage. (obsolete) — Splinter Review
I rewrote the tests using Tim's latest framework.

Arthur, would you please take a look?  It only does one setItem and one getItem instead of setting items back and forth between those two pages like the original patch.  But essentially they're the same.
Attachment #8784791 - Flags: review?(arthuredelstein)
Attachment #8777310 - Attachment is obsolete: true
Attached patch Isolation tests of localStorage. (obsolete) — Splinter Review
Attachment #8784792 - Flags: review?(arthuredelstein)
Attachment #8784791 - Attachment is obsolete: true
Attachment #8784791 - Flags: review?(arthuredelstein)
Comment on attachment 8784792 [details] [diff] [review]
Isolation tests of localStorage.

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

::: browser/components/originattributes/test/browser/browser_localStorageIsolation.js
@@ +27,5 @@
> +  } else {
> +    return yield* getItem(aBrowser);
> +  }
> +}
> +

I think you can simplify here. What about something like:

// Use a random key so we don't access it in later tests.
const key = Math.random().toString();

// Define the testing function
function* doTest(aBrowser) {
  return yield ContentTask.spawn(aBrowser, null, function () {
    let value = content.localStorage.getItem(key);
    if (value === null) {
      // No value is found, so we create one.
      value = Math.random().toString();
      content.localStorage.setItem(key, value);
    }
    return yield value;
  });
}

Then you can drop the setItem/getItem functions, and also you don't need to add the additional aTabNum argument to the framework.
Attachment #8784792 - Flags: review?(arthuredelstein)
Attached patch Isolation tests of localStorage. (obsolete) — Splinter Review
Hi Arthur,

I did what you suggested.

Hi Baku,

Could you also review this, please?

Thanks.
Attachment #8785782 - Flags: review?(arthuredelstein)
Attachment #8785782 - Flags: review?(amarchesini)
Attachment #8784792 - Attachment is obsolete: true
Attached patch Isolation tests of localStorage. (obsolete) — Splinter Review
Forgot to remove the changes to head.js in the previous patch.
Attachment #8785784 - Flags: review?(arthuredelstein)
Attachment #8785784 - Flags: review?(amarchesini)
Attachment #8785782 - Attachment is obsolete: true
Attachment #8785782 - Flags: review?(arthuredelstein)
Attachment #8785782 - Flags: review?(amarchesini)
Attached patch Isolation tests of localStorage. (obsolete) — Splinter Review
Sorry, this one should be correct.
Attachment #8785823 - Flags: review?(arthuredelstein)
Attachment #8785823 - Flags: review?(amarchesini)
Attachment #8785784 - Attachment is obsolete: true
Attachment #8785784 - Flags: review?(arthuredelstein)
Attachment #8785784 - Flags: review?(amarchesini)
Comment on attachment 8785823 [details] [diff] [review]
Isolation tests of localStorage.

Thanks! :) I guess we need `skip-if = true` unless localStorage is already properly isolated, right?

Also, it occurs to me that we should perhaps test sessionStorage here as well?
Attachment #8785823 - Flags: review?(arthuredelstein) → review+
Attachment #8785823 - Flags: review?(amarchesini) → review+
Attached patch Isolation tests of DOM Storage. (obsolete) — Splinter Review
Session storage is already isolated across open tabs regardless of their origins and first party domains, so I passed in a comapare function that makes sure results are always different.

Is this what you want, Arthur?
Attachment #8786184 - Flags: review?(arthuredelstein)
Attachment #8785823 - Attachment is obsolete: true
(In reply to Jonathan Hao [:jhao] from comment #20)
> Created attachment 8786184 [details] [diff] [review]
> Isolation tests of DOM Storage.
> 
> Session storage is already isolated across open tabs regardless of their
> origins and first party domains, so I passed in a comapare function that
> makes sure results are always different.

I'm sorry, my mistake. I didn't realize that sessionStorage is already isolated across tabs. I tend to think for the sake of clarity that the previous version of your patch is better.
Attachment #8786184 - Flags: review?(arthuredelstein)
Attached patch Isolation tests of DOM Storage. (obsolete) — Splinter Review
How about we only skip first party isolation tests and keep the container ones?  We can turn it back on after bug 1260931 is landed.
Attachment #8786199 - Flags: review?(arthuredelstein)
Attachment #8786184 - Attachment is obsolete: true
Comment on attachment 8786199 [details] [diff] [review]
Isolation tests of DOM Storage.

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

(In reply to Jonathan Hao [:jhao] from comment #22)
> Created attachment 8786199 [details] [diff] [review]
> Isolation tests of DOM Storage.
> 
> How about we only skip first party isolation tests and keep the container
> ones?  We can turn it back on after bug 1260931 is landed.

That sounds like a good idea to me. Does everything pass on the try server?
Attachment #8786199 - Flags: review?(arthuredelstein) → review+
Attachment #8786199 - Attachment is obsolete: true
Try looks good. Thanks, Arthur and Baku.
Keywords: checkin-needed
Attachment #8777191 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace20b16c454
Isolation tests of localStorage. r=baku, r=arthuredelstein
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ace20b16c454
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1264560
Blocks: meta_tor
Summary: Tests for first party isolation of localStorage (Tor 13749) → Tests for first party isolation of localStorage (Tor 13749.1)
You need to log in before you can comment on or make changes to this bug.