make security fixes and add assertions to cover previous conversions from private browsing boolean to origin attribute

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 6 obsolete attachments)

This bug covers adding assertions to code that we previously converted from the private browser boolean to the origin attributes.  Specifically the work done in Bug 1276328 and in this changeset: https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4
Assignee: nobody → huseby
Status: NEW → ASSIGNED
Summary: add assertions to cover previous conversions from private browsing boolean to origin attribute → make security fixes and add assertions to cover previous conversions from private browsing boolean to origin attribute
So I went through the patch for Bug 1276328 and found what I think is a very serious security issue that I documented in that bug.  I also went through the changeset linked to above on the DOMStorage.  That whole subsystem has been converted to origin attributes and there is no boolean to compare to and not places that are obvious places to add assertions.  The only thing I can think of is adding assertions to ensure that the principal pointers are not null since we're now relying on the principal's origin attributes for the private browsing state.  Tanvi, Ehsan, Baku, do you have any other ideas on what to add to the the changeset linked to above?
Flags: needinfo?(tanvi)
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
(In reply to Dave Huseby [:huseby] from comment #1)
> So I went through the patch for Bug 1276328 and found what I think is a very
> serious security issue that I documented in that bug.

Correction: as discussed there, there's no security issue.

> I also went through
> the changeset linked to above on the DOMStorage.  That whole subsystem has
> been converted to origin attributes and there is no boolean to compare to
> and not places that are obvious places to add assertions.  The only thing I
> can think of is adding assertions to ensure that the principal pointers are
> not null since we're now relying on the principal's origin attributes for
> the private browsing state.

Those assertions seem pointless, since we'd just crash if the principal pointers are null, right?

> Tanvi, Ehsan, Baku, do you have any other ideas
> on what to add to the the changeset linked to above?

I think we can close this bug and call it a day.
Flags: needinfo?(ehsan)
I agree with Ehsan, I think we can close this.  Do you concur?
Flags: needinfo?(amarchesini)
(In reply to :Ehsan Akhgari from comment #2)
> (In reply to Dave Huseby [:huseby] from comment #1) 
> > I also went through
> > the changeset linked to above on the DOMStorage.  That whole subsystem has
> > been converted to origin attributes and there is no boolean to compare to
> > and not places that are obvious places to add assertions.  The only thing I
> > can think of is adding assertions to ensure that the principal pointers are
> > not null since we're now relying on the principal's origin attributes for
> > the private browsing state.
> 
> Those assertions seem pointless, since we'd just crash if the principal
> pointers are null, right?
> 
How do you know we would always crash?  Are the principal arguments required in all of the methods called in the changeset?

Looking at https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4, it seems like assertions can be added to most of these places.

For example, before this line of code, you could compare the subjectPrincipal's PB OA to isPrivateBrowsingWindow:
http://searchfox.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1247
https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4#l13.1

Right before http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10723, you could add an assert comparing IsPrivateBrowsing() with the principal's Origin Attributes.
https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4#l6.1

Remember we said that sometimes, even if we were correctly pulling the PB boolean off of the OA associated with the docshell/loadContext, we might be using the wrong principal.
Flags: needinfo?(tanvi)
Tanvi,

Considering the two examples you cited, the first one, the patch removes isPrivateBrowsingWindow because it is initialized from the PB autostart pref and the chrome flags.  If the user doesn't have the PB autostart pref set to true, then it propagates the value of the parent's load context which is the parent's origin attributes.  I suppose we could assert there, but it would be initialized by the parent OA which is the source of truth.  If anything, we should probably just add a function like this:

http://searchfox.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#451

but checks compatibility between the subject principal private browing id and the parent OA.

The second instance would be asserting that the loadContext OA is the same as the principal's OA.  IIRC the principal is the one from the loadContext so it would be redundant to assert.

--dave
Flags: needinfo?(tanvi)
(In reply to Dave Huseby [:huseby] from comment #5)
> Tanvi,
> 
> Considering the two examples you cited, the first one, the patch removes
> isPrivateBrowsingWindow because it is initialized from the PB autostart pref
> and the chrome flags.  If the user doesn't have the PB autostart pref set to
> true, then it propagates the value of the parent's load context which is the
> parent's origin attributes.  I suppose we could assert there, but it would
> be initialized by the parent OA which is the source of truth.  If anything,
> we should probably just add a function like this:
> 
> http://searchfox.org/mozilla-central/source/embedding/components/
> windowwatcher/nsWindowWatcher.cpp#451
> 
> but checks compatibility between the subject principal private browing id
> and the parent OA.
> 
> The second instance would be asserting that the loadContext OA is the same
> as the principal's OA.  IIRC the principal is the one from the loadContext
> so it would be redundant to assert.
> 
> --dave

In cases where you know the PB flag came from OA and you know that the Principal came from OA, you don't need the assert.  So if you have audited all callsites and know that the principal came from OA (it sounds like you already know that the PB flag came from it), then you are fine.  But in some cases (like maybe favicons), it is not always clear that the principal came from OA.  Particularly because the principal may have been set somewhere 5 function calls back.
Flags: needinfo?(tanvi)
I think some of the links above aren't pointing to the correct code any more, but in general I agree that if we don't know where the principal is coming from then we should assert.
Posted patch Bug_1316075.patch (obsolete) — Splinter Review
I went back to double checked this code and decided that it couldn't hurt to add some asserts.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e6b8a53ff0d4abafee5b4d9984499ae6a6d4e5
Attachment #8812952 - Flags: review?(ehsan)
Comment on attachment 8812952 [details] [diff] [review]
Bug_1316075.patch

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

::: docshell/base/nsDocShell.cpp
@@ +3021,5 @@
>    nsCOMPtr<nsPIDOMWindowOuter> domWin = GetWindow();
>  
>    AssertOriginAttributesMatchPrivateBrowsing();
> +  MOZ_DIAGNOSTIC_ASSERT(
> +    (BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 

Nit: please remove the trailing space.

@@ +3022,5 @@
>  
>    AssertOriginAttributesMatchPrivateBrowsing();
> +  MOZ_DIAGNOSTIC_ASSERT(
> +    (BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 
> +    (mPrivateBrowsingId > 0));

The best code is code that we can delete.  Nobody calls this function any more, including add-ons even.  So instead of adding assertions to it, let's just delete this function (both from here and nsIDocShell).

::: dom/base/nsGlobalWindow.cpp
@@ +10790,5 @@
>  
> +    MOZ_DIAGNOSTIC_ASSERT(
> +      (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 
> +      IsPrivateBrowsing());
> +      

Lots of trailing spaces here too.

@@ +10858,5 @@
>  
> +    MOZ_DIAGNOSTIC_ASSERT(
> +      (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 
> +      IsPrivateBrowsing());
> + 

Here too.

::: dom/storage/DOMStorageManager.cpp
@@ +374,5 @@
> +  //browsing flag. adding this assert so that we can check correctness of the
> +  //origin attributes hanging off of the principal.
> +  MOZ_DIAGNOSTIC_ASSERT(
> +    (BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 
> +    false);

Instead of this just test that mPrivateBrowsingId is 0.

@@ +387,5 @@
>                                   nsIDOMStorage** aRetval)
>  {
> +  MOZ_DIAGNOSTIC_ASSERT(
> +    (BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 
> +    false);

Ditto.

::: embedding/components/windowwatcher/nsWindowWatcher.cpp
@@ +1245,5 @@
>        nsCOMPtr<nsPIDOMWindowInner> pInnerWin = parentWindow->GetCurrentInnerWindow();
>  
> +      MOZ_DIAGNOSTIC_ASSERT(
> +        (BasePrincipal::Cast(subjectPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) == 
> +        isPrivateBrowsingWindow);

More trailing spaces.
Attachment #8812952 - Flags: review?(ehsan) → review+
Posted patch Bug_1316075.patch (obsolete) — Splinter Review
r+ :Ehsan already.  this incorporates the feedback changes.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca7c9865c1f643c1ad7da9c7438bff6eb8f9ee81
Attachment #8812952 - Attachment is obsolete: true
Attachment #8813316 - Flags: review+
oh wow.  look at the results of the try push for this patch.  assertion failures all over the place.  i'm looking at them now trying to figure out if these are false positives or if we should really be worried.
Flags: needinfo?(tanvi)
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
Looking at a couple of these assertion failures, the assertion you added to PrecacheStorage() is bogus (and so is the comment there which is why I mistakenly r+ed the patch.)

nsGlobalWindow::PreloadLocalStorage() tries to preload the local storage for a given page in order to hopefully have it loaded and ready by the time the page accesses window.localStorage.  Private windows do not persist their local storage and do not use the on-disk stored storage database.  The reason the code used to pass false there was to preload the storage for non-PB mode, and it was doing extra unneeded work for PB pages (e.g., it would preload the localStorage for the site but not use it.)

I think what you want is to remove that assertion and only call PrecacheStorage() if the current page isn't in PB mode.
Flags: needinfo?(ehsan)
Thanks Ehsan, I was seeing the same thing.  I'm fixing the patch now.
Posted patch Bug_1316075.patch (obsolete) — Splinter Review
incorporates review feedback.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3498d1818b9995aa5893648809b589419eb004ec
Attachment #8813316 - Attachment is obsolete: true
Flags: needinfo?(tanvi)
Flags: needinfo?(amarchesini)
Attachment #8813476 - Flags: review+
I'm still seeing this assertion failing: embedding/components/windowwatcher/nsWindowWatcher.cpp:1249

The original patch removed the code to pass isPrivateBrowsingWindow to DOMStorageManager::GetStorage():
https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4#l13.12

So I thought that the appropriate assertion here is to make sure that the origin attribute matched the isPrivateBrowsingWindow value:

> MOZ_DIAGNOSTIC_ASSERT(
>   (BasePrincipal::Cast(subjectPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==
>   isPrivateBrowsingWindow);

This assert seems correct to me.  I'm not sure what the correct path forward is on this.
Flags: needinfo?(ehsan)
(In reply to Dave Huseby [:huseby] from comment #15)
> I'm still seeing this assertion failing:
> embedding/components/windowwatcher/nsWindowWatcher.cpp:1249
> 
> The original patch removed the code to pass isPrivateBrowsingWindow to
> DOMStorageManager::GetStorage():
> https://hg.mozilla.org/mozilla-central/rev/34acf7fe34f4#l13.12
> 
> So I thought that the appropriate assertion here is to make sure that the
> origin attribute matched the isPrivateBrowsingWindow value:
> 
> > MOZ_DIAGNOSTIC_ASSERT(
> >   (BasePrincipal::Cast(subjectPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==
> >   isPrivateBrowsingWindow);
> 
> This assert seems correct to me.  I'm not sure what the correct path forward
> is on this.

Two points:

1. Note the condition here: <http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/embedding/components/windowwatcher/nsWindowWatcher.cpp#1109>.  If the execution doesn't get to the body of this conditional before hitting the assertion, then the assertion is being over-zealous.  I think we need to adjust the assertion anyways.  (Also note that in the case of the new window, we adjust the docshell flag here: <http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/embedding/components/windowwatcher/nsWindowWatcher.cpp#1175> but this doesn't change the OA flag in the case of a chrome docshell.)  This logic is pretty twisted... :/

2. I think bug 1283281 slightly changed our behavior here.  The subject principal may be system, in which case the storage is never marked as private.  That being said, this is session storage, and session storage is already not persisted to disk <http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/dom/storage/DOMStorageCache.cpp#161>, so in practice the storage that is being cloned here never ends up being persisted to disk, which is good.  The bad part is that we'd end up computing the wrong index here <http://searchfox.org/mozilla-central/rev/feef954874af9a18168e61a75629a9406b847c53/dom/storage/DOMStorageCache.cpp#53>, however the implications of that aren't 100% clear to me.  But at any rate I'd rather be safe than sorry.  I think we need to introduce a version of this API which accepts a boolean flag and pass isPrivateBrowsingWindow explicitly as before.  That part needs to be filed and fixed separately in a new bug I'd think because we should backport it all the way to beta...
Flags: needinfo?(ehsan)
ehsan, here's an updated patch with the assert restricted to the only case where I think it is valid.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f37ce5b5cb193382ab43976a0427b0bec8fc4c
Attachment #8813476 - Attachment is obsolete: true
Attachment #8813520 - Attachment is obsolete: true
Attachment #8813878 - Flags: review?(ehsan)
Blocks: 1319951
I filed Bug 1319951 to cover point #2 in comment 17.
Priority: -- → P1
(In reply to Dave Huseby [:huseby] from comment #18)
> The call to
> http://searchfox.org/mozilla-central/rev/
> feef954874af9a18168e61a75629a9406b847c53/embedding/components/windowwatcher/
> nsWindowWatcher.cpp#1175 actually doesn't do anything.  See here:
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/LoadContext.
> cpp#133

The nsILoadContext object here is an nsDocShell.  LoadContext is a parent process only class.
Comment on attachment 8813878 [details] [diff] [review]
Bug_1316075.3.patch (with assert restricted to valid case)

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

I'd like to see another version of the patch even though all of the comments below are pretty minor.

Out of curiosity, what specific condition was causing the assertion in nsWindowWatcher to go off?

::: dom/base/nsGlobalWindow.cpp
@@ +3031,5 @@
>    }
>  
> +  // private browsing windows do not persist local storage to disk so we should
> +  // only try to precache storage when we're not a private browsing window.
> +  if (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId == 0) {

Instead of this, please just do:

  if (principal->GetPrivateBrowsingId() == 0)

@@ +10792,5 @@
>        return nullptr;
>      }
>  
> +    MOZ_DIAGNOSTIC_ASSERT(
> +      (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==

Similar to the above, you can use nsIPrincipal::GetPrivateBrowsingId() directly here.

@@ +10794,5 @@
>  
> +    MOZ_DIAGNOSTIC_ASSERT(
> +      (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==
> +      IsPrivateBrowsing());
> +      

Nit: trailing space

@@ +10860,5 @@
>        }
>      }
>  
> +    MOZ_DIAGNOSTIC_ASSERT(
> +      (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==

Here too.

@@ +10862,5 @@
>  
> +    MOZ_DIAGNOSTIC_ASSERT(
> +      (BasePrincipal::Cast(principal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==
> +      IsPrivateBrowsing());
> + 

And here!

::: embedding/components/windowwatcher/nsWindowWatcher.cpp
@@ +1240,5 @@
>      nsCOMPtr<nsIDOMStorageManager> newStorageManager =
>        do_QueryInterface(newDocShell);
>  
>      if (parentStorageManager && newStorageManager) {
> +			if (windowIsNew && 

Please avoid using tabs.  :-)

@@ +1241,5 @@
>        do_QueryInterface(newDocShell);
>  
>      if (parentStorageManager && newStorageManager) {
> +			if (windowIsNew && 
> +          !nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal)) {

Instead of replicating that condition here, you can set a boolean to true inside the if block above and check for that here.

@@ +1245,5 @@
> +          !nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal)) {
> +        auto* docShell = static_cast<nsDocShell*>(newDocShell.get());
> +        if (docShell->ItemType() != nsIDocShellTreeItem::typeChrome) {
> +          MOZ_DIAGNOSTIC_ASSERT(
> +            (BasePrincipal::Cast(subjectPrincipal)->OriginAttributesRef().mPrivateBrowsingId > 0) ==

Same comment about nsIPrincipal::GetPrivateBrowsingId().
Attachment #8813878 - Flags: review?(ehsan)
I'm embarrassed about the whitespace and tabs.  I apologize.  I've got my vim environment straightened out now.  As for penance, I've also included a second patch that fixes all of the trailing whitespace and tabs I found in the files I touched.

To answer your question in comment 22, there were two asserts that were hitting in the unrestricted version:

> nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal) == false

and

> docShell->ItemType() != nsIDocShellTreeItem::typeChrome

So this code gets called with system/expanded principals as well as chrome docshells.  Anyway, the restricted case seems to do fine.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e012f53eaba143d6163d0c4c4cc5b858d62a557f
Attachment #8813878 - Attachment is obsolete: true
Attachment #8814998 - Flags: review?(ehsan)
Separate patch that cleans up trailing whitespace and tabs (not mine) in the files I touched. Penance for being dumb and wasting your time with whitespace in a patch sent for review. :)
Attachment #8814999 - Flags: review?(ehsan)
Comment on attachment 8814998 [details] [diff] [review]
Bug_1316075.4.patch (review feedback incorporated, with assert restricted to valid case)

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

::: embedding/components/windowwatcher/nsWindowWatcher.cpp
@@ +1243,5 @@
>        do_QueryInterface(newDocShell);
>  
> +    if (parentStorageManager && newStorageManager && shouldCheckPrivateBrowsingId) {
> +      if ((subjectPrincipal->GetPrivateBrowsingId() > 0) != isPrivateBrowsingWindow) {
> +        MOZ_DIAGNOSTIC_ASSERT(windowIsNew == true);

This follows from setting shouldCheckPrivateBrowsingId under an |if (windowIsNew)| so the assertion isn't doing anything useful, please remove it.

(If you really want to keep it, please use MOZ_ASSERT instead -- there's no good reason to check this under non-debug builds...)

@@ +1244,5 @@
>  
> +    if (parentStorageManager && newStorageManager && shouldCheckPrivateBrowsingId) {
> +      if ((subjectPrincipal->GetPrivateBrowsingId() > 0) != isPrivateBrowsingWindow) {
> +        MOZ_DIAGNOSTIC_ASSERT(windowIsNew == true);
> +        MOZ_DIAGNOSTIC_ASSERT(nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal) == false);

Same.

@@ +1246,5 @@
> +      if ((subjectPrincipal->GetPrivateBrowsingId() > 0) != isPrivateBrowsingWindow) {
> +        MOZ_DIAGNOSTIC_ASSERT(windowIsNew == true);
> +        MOZ_DIAGNOSTIC_ASSERT(nsContentUtils::IsSystemOrExpandedPrincipal(subjectPrincipal) == false);
> +        auto* docShell = static_cast<nsDocShell*>(newDocShell.get());
> +        MOZ_DIAGNOSTIC_ASSERT(docShell->ItemType() != nsIDocShellTreeItem::typeChrome);

This one too.
Attachment #8814998 - Flags: review?(ehsan) → review+
Comment on attachment 8814999 [details] [diff] [review]
Bug_1316075.whitespace.patch

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

Thanks!
Attachment #8814999 - Flags: review?(ehsan) → review+
The try looks good.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1386238da4
Add asserts for the places where we've gotten rid of the private browsing boolean. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8857178de40
Additional patch to clean up tabs and trailing whitespace in files touched. r=ehsan
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1321969
You need to log in before you can comment on or make changes to this bug.