1264567 - Tests for first party isolation of localStorage (Tor 13749.1)

Status: RESOLVED FIXED

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

Description

[Jonathan Hao (inactive) [:jhao]]

Attachment: the Tor Browser patch (WIP)

Tor Browser tests for first party isolation of localStorage.
https://torpat.ch/13749

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: regression tests for first party isolation of localStorage

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?

Comment 2

[Jonathan Hao (inactive) [:jhao]]

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?

Comment 3

[Jonathan Hao (inactive) [:jhao]]

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

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tor's original tests (for comparison)

Previous patch has a wrong diff.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

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.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

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.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Attachment: Set firstPartyDomain in MaybeCreateDocShell.

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.

Comment 8

[Dave Huseby [:huseby]]

Yoshi has the firstPartyDomain bug now.  Do you still need anything from me?

Comment 9

[Ethan Tseng [:ethan]]

(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.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

(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.

Comment 11

[Jonathan Hao (inactive) [:jhao]]

(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 12

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8777781 [details] [diff] [review]
Set firstPartyDomain in MaybeCreateDocShell.

No longer need this patch since Yoshi's patches work.

Comment 13

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

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.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

Comment 15

[Arthur Edelstein [:arthur]]

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.

Comment 16

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

Hi Arthur,

I did what you suggested.

Hi Baku,

Could you also review this, please?

Thanks.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

Forgot to remove the changes to head.js in the previous patch.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

Sorry, this one should be correct.

Comment 19

[Arthur Edelstein [:arthur]]

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?

Comment 20

[Jonathan Hao (inactive) [:jhao]]

Attachment: 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.

Is this what you want, Arthur?

Comment 21

[Arthur Edelstein [:arthur]]

(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.

Comment 22

[Jonathan Hao (inactive) [:jhao]]

Attachment: 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.

Comment 23

[Arthur Edelstein [:arthur]]

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?

Comment 24

[Jonathan Hao (inactive) [:jhao]]

Attachment: Isolation tests of localStorage.

Waiting for try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e269b0994c

Comment 25

[Jonathan Hao (inactive) [:jhao]]

Try looks good. Thanks, Arthur and Baku.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace20b16c454
Isolation tests of localStorage. r=baku, r=arthuredelstein

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ace20b16c454