1297687 - With "never remember history" enabled, postMessage() is rejected despite origins matching

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew Zitnay]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160817112116

Steps to reproduce:

1) Run Firefox 49+ (currently beta channel or higher)

2) Go to menu -> Preferences -> Privacy, change to "never remember history", and restart Firefox

3) Install LastPass 4.1.26a from https://addons.mozilla.org/en-us/firefox/addon/lastpass-password-manager/versions/beta

4) Browse to resource://support-at-lastpass-dot-com/data/about.html?lplanguage=

5) Open the browser console


Actual results:

You'll see the following in the browser console:

Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘resource://support-at-lastpass-dot-com’) does not match the recipient window’s origin (‘resource://support-at-lastpass-dot-com’).


Expected results:

The postMessage() should have succeeded, since the origins match.

Comment 1

[Andrew Zitnay]

The postMessage() call in question is called from data/message_shim.js within the extension.

Comment 2

[Hal Wine [:hwine] use NI!]

Impact note: this prevents the Last Pass extension from working in 49, which is used by many companies including Mozilla.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 49, seems likely to be a recent regression. We could still potentially uplift a fix for this for 49. 
Andrew, can you help find an owner for this issue ?

Comment 4

[Andrew Overholt [:overholt]]

baku seems best but he's on PTO a bit this week. Let's needinfo him and see what comes of it.

Comment 5

[roxleitan]

Based on this regression range I will change the flags:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16b4946069a9470a94f33791de7440966844747d&tochange=ff298d2993a32aeaca0eacac32d8e673298cf5c5

Comment 6

[Andrea Marchesini [:baku]]

The problem here is that, when "never remember history" setting is used, we force mPrivateBrowsingId to be 1 but not all principals are correctly set. ExpandedPrincipal, in this scenario, is created from a systemPrincipal; this means that the mPrivateBrowsingId is always 0 and the postMessage security check fails.

I suspect we should force mPrivateBrosingId = 1 also in netwerk/base/LoadInfo.cpp CTOR if the pref is set.
Before writing a patch, I would like to have a feedback from smaug.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

How was this case handled before we added pb flag to origin attributes?

Comment 8

[Andrea Marchesini [:baku]]

OriginAttributes didn't contain privateBrowsingId, so the postMessageEvent::Run check passed:

https://dxr.mozilla.org/mozilla-central/source/dom/base/PostMessageEvent.cpp#110

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Ok, so why did the code work earlier? Equals shouldn't be true for ExpandedPrincipal and some random other principal.

What are the principals we're dealing with there? Some principal from a web page and some expanded principal? Who is calling postMessage on what kind of window object?
Why does one have the pb flag and one doesn't?

Comment 10

[(no longer active)]

I would also like to understand what the answers to questions in comment 9 are.  I still don't understand the reason why this bug occurs.

(In reply to Andrea Marchesini [:baku] from comment #6)
> I suspect we should force mPrivateBrosingId = 1 also in
> netwerk/base/LoadInfo.cpp CTOR if the pref is set.
> Before writing a patch, I would like to have a feedback from smaug.

No.  The only implication of permanent private browsing is for all of the windows we create to be in PB mode by default.  See the code in nsWindowWatcher as well as nsAppShellService::JustCreateTopWindow which deals with the "browser.privatebrowsing.autostart" pref.  The system principal's OA.mPB flag needs to remain false.  If you have a system principal which has its mPrivateBrowsingId member set to true, that's where the bug is coming from.

Comment 11

[Hal Wine [:hwine] use NI!]

In case this helps isolate issue, in 51.0a1 (2016-08-29), I can get this to occur outside of private browsing mode as follows:
 - launch nightly
 - check last pass toolbar menu display - it displays properly
 - open private browsing window
 - check last pass toolbar menu display - it is blank
 - close private browsing window
 - check last pass toolbar menu display in original window - it is NOW BLANK

Comment 12

[Andrea Marchesini [:baku]]

> principal's OA.mPB flag needs to remain false.  If you have a system
> principal which has its mPrivateBrowsingId member set to true, that's where
> the bug is coming from.

We don't have any systemPrincipal with mPrivateBrowsingID set to true.

> What are the principals we're dealing with there? Some principal from a web
> page and some expanded principal? Who is calling postMessage on what kind of
> window object?

Here is what happens in this scenario:

1. we create a sandbox. The sandbox creates a nsExtendedPrincipal with 3 nsPrincipals. The first 2 are created from strings. So we end up here:

            // We use a default originAttributes here because we don't support
            // passing a userContextId with an array.
            PrincipalOriginAttributes attrs;
            if (!ParsePrincipal(cx, str, attrs, getter_AddRefs(principal)))
                return false;

https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#1340

At this point their mOriginAttributes.mPrivateBrowsingId is 0.
The last principal is created from an object and it's mOriginAttributes.mPrivateBrowsingId is set to 1.

Then we create an nsExtendedPrincipal: 

https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#1371

However, nsExtendedPrincipal.mOriginAttributes doesn't contain the corrent mPrivateBrowsingId value. And the check in PostMessageEvent::Run fails.

Comment 13

[(no longer active)]

I see what's happening now, thanks for the explanation.

First things first, forcing Sandbox objects to be created with OAs without an mPrivateBrowsingId seems generally wrong.  We should probably add something similar to bug 1260917 to allow the caller to override the mPrivateBrowsingId that we will use.  But even with that, we should try to maintain existing code which used to work before bug 1269361.  In order to do that, I think we need to grab the subject principal if one exists, and extract the PB OA attribute from it.  With that change, I think the lastpass add-on should work again without needing to pass in an exising PB sandbox option.

Comment 14

[(no longer active)]

This bug can potentially break any add-on that uses the Sandbox feature.  We should definitely fix it for 49.  Marking the bug as such.

Comment 15

[Marcia Knous [:marcia]]

Tracking 50/51+ for the fact that it could break an any addon that uses the Sandbox.

Comment 16

[(no longer active)]

I'm taking this since baku is away on a work week.

Using the subject principal from comment 13 didn't work, since the code in question runs under the system principal which always has mPrivateBrowsingId set to 0.

Instead, I ended up implementing a different solution.  As we're iterating over the array passed to Sandbox(), we remember a window's principal if one is encountered, and then assign that to the OA for the expanded principal.  Now the obvious question would be, what should we do if we have two windows in the list?  For now I'm letting the last one win...  The nice thing about this solution is that it will _probably_ fix most add-ons that are affected by this, and in case some add-on keeps being broken, it's very likely that it's passing two or more windows of different kinds to Sandbox(), which we can only support properly once we allow the caller to specify an OriginAttributes.

I will also file another bug to add a better API to allow the caller to specify an OriginAttributes to be used here.  Since that part won't be something that we want to backport, I'm not doing the work for that here.

Comment 17

[(no longer active)]

Attachment: Use the OriginAttributes associated with a window principal when creating a Sandbox with an expanded principal

Comment 18

[(no longer active)]

Attachment: Use the OriginAttributes associated with a window principal when creating a Sandbox with an expanded principal

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8786979 [details] [diff] [review]
Use the OriginAttributes associated with a window principal when creating a Sandbox with an expanded principal

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

::: caps/nsIScriptSecurityManager.idl
@@ +254,5 @@
>       */
>      nsIPrincipal getChannelURIPrincipal(in nsIChannel aChannel);
>  
>      /**
> +     * Get the principal for a sandbox object.

do we want to write that this is for testing only?

::: caps/nsPrincipal.cpp
@@ +673,5 @@
>      return a == b;
>    }
>  };
>  
> +nsExpandedPrincipal::nsExpandedPrincipal(nsTArray<nsCOMPtr <nsIPrincipal> > &aWhiteList,

why these spaces? Write nsTArray<nsCOMPtr<nsIPrincipal>>& aWhiteList
& next to >

@@ +675,5 @@
>  };
>  
> +nsExpandedPrincipal::nsExpandedPrincipal(nsTArray<nsCOMPtr <nsIPrincipal> > &aWhiteList,
> +                                         const PrincipalOriginAttributes& aOriginAttributes)
> +  : nsExpandedPrincipal(aWhiteList)

mOriginAttributes(aOriginAttributes) ?

::: caps/nsPrincipal.h
@@ +66,5 @@
>  
>  class nsExpandedPrincipal : public nsIExpandedPrincipal, public mozilla::BasePrincipal
>  {
>  public:
> +  nsExpandedPrincipal(nsTArray< nsCOMPtr<nsIPrincipal> > &aWhiteList,

ditto for spaces and &

::: caps/nsScriptSecurityManager.cpp
@@ +474,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsScriptSecurityManager::GetSandboxPrincipal(JS::HandleValue sandboxArg,
> +                                             JSContext* cx,

aCx and aSandboxArg

::: js/xpconnect/src/Sandbox.cpp
@@ +1358,5 @@
>                  principal = sop->GetPrincipal();
> +
> +            // If a Window object has been passed, the expanded principal will inherit
> +            // its OriginAttributes.  If more than one Window object has been passed, use
> +            // the last one.

do we want to add: if (!windowPrincipal) ?

@@ +1749,5 @@
>      return NS_OK;
>  }
>  
>  nsresult
> +xpc::GetSandboxPrincipal(JSContext* cx, HandleObject sandboxArg,

aCx, aSandboxArg

::: js/xpconnect/src/xpcprivate.h
@@ +3295,5 @@
>  SetSandboxMetadata(JSContext* cx, JS::HandleObject sandboxArg,
>                     JS::HandleValue metadata);
>  
> +nsresult
> +GetSandboxPrincipal(JSContext* cx, JS::HandleObject sandbox,

aCx, aSandboxArg

Comment 20

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #19)
> ::: js/xpconnect/src/Sandbox.cpp
> @@ +1358,5 @@
> >                  principal = sop->GetPrincipal();
> > +
> > +            // If a Window object has been passed, the expanded principal will inherit
> > +            // its OriginAttributes.  If more than one Window object has been passed, use
> > +            // the last one.
> 
> do we want to add: if (!windowPrincipal) ?

That would make it so that the first window passed wins, not the last one.  I don't particularly care much either way, but letting the last window win makes a bit more sense to me.  What do you think?

Comment 21

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #19)
> @@ +675,5 @@
> >  };
> >  
> > +nsExpandedPrincipal::nsExpandedPrincipal(nsTArray<nsCOMPtr <nsIPrincipal> > &aWhiteList,
> > +                                         const PrincipalOriginAttributes& aOriginAttributes)
> > +  : nsExpandedPrincipal(aWhiteList)
> 
> mOriginAttributes(aOriginAttributes) ?

Can't do that.  mOriginAttributes is a base class member, so it cannot be initialized in the initializer list.

Comment 22

[Andrea Marchesini [:baku]]

> That would make it so that the first window passed wins, not the last one. 
> I don't particularly care much either way, but letting the last window win
> makes a bit more sense to me.  What do you think?

Ok. Write a comment about it.

Comment 23

[(no longer active)]

We did some more IRC investigation as to the cause of this bug.  It turns out that the expanded principal in question is the subject principal in nsGlobalWindow::PostMessageMozOuter() which we get the OA off of.  Before my patch, that principal will always have a default initialized OA, hence this bug.

Comment 24

[(no longer active)]

Comment on attachment 8786979 [details] [diff] [review]
Use the OriginAttributes associated with a window principal when creating a Sandbox with an expanded principal

Approval Request Comment
[Feature/regressing bug #]: Bug 1269361 
[User impact if declined]: Add-ons such as LastPass may not work under some circumstances.
[Describe test coverage new/current, TreeHerder]: The patch has been tested manually to verify that it fixes LastPass, and it also includes an automated test.
[Risks and why]: This patch is somewhat risky in that it's changing the OriginAttributes of expanded principals.  We have actually had the underlying bug for a while but this may be the first instance of it where we have observed an actual user facing breakage as a result.  I would normally not advocate for landing this patch all the way to beta but in this case, the alternative (which would be reverting bug 1269361 and everything that has landed on top of it) is a lot scarier.
[String/UUID change made/needed]: None.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7eb0625d3e
Use the OriginAttributes associated with a window principal when creating a Sandbox with an expanded principal; r=baku

Comment 26

[Bobby Holley (:bholley)]

Um, there are some pretty serious issues in this patch. :\

First, why do we need getSandboxPrincipal? I'd think Cu.getObjectPrincipal should work fine here.

Second, expanded principals themselves are not supposed to have OAs, and this "use the first one encountered" strategy breaks the algebra of how expanded principals are supposed to work (that is to say, any checks should be iteratively forwarded to the contained principals).

I appreciate the respect for my low review bandwidth, but changes to core security code still need review from a peer or at least sr from me. I think we need to back this out.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes, can you back this out from inbound or m-c if it ended up there? Thanks. 

Ehsan & others: The RC build will happen after the merge on Monday (talk with Sylvestre as I will be out).
This may be an issue where we end up having to do an extra RC build/release mid next week.

Comment 28

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

It was only on inbound, but I backed it out on the merge to m-c in https://hg.mozilla.org/mozilla-central/rev/63d190efe42e

Comment 29

[(no longer active)]

Bobby and I discussed this on IRC at length.  I'm going to try to capture the results here.

Bobby thinks that instead of fixing things for all nsExtendedPrincipals, we should focus on fixing the consumers who would be affected by reading the OA of an nsEP and expecting it to contain the correct information for private browsing.  He owns this code, so he wins!

But my biggest worry about this bug isn't about the postMessage() consumer at all.  All that code does is get the subject principal (which _could_ be an nsEP) and get its OA.  So, while Bobby disagrees with this to some extent, I'd only be OK with only fixing the postMessage consumer after we gain some evidence that this is the only code affected by this.  This audit is tricky, since we need to care about subject principals, nsIPrincipals* that we don't know the concrete type of, DOM features exposed to sandboxes which may end up with an nsEP somewhere and possibly other things.

Once the said audit is finished, and a patch for postMessage() is written, we should add assertions to ensure that GetOriginAttributes can never be called on an nsEP.  This audit will take some time since it needs to be done quite carefully.  Since I'm very hesitant to hold off the release of the said audit, I'd have to look into backing out all of James' work as an alternative fix for beta.  I have no idea at the moment which approach is less risky, but sadly a backout may end up as the less risky approach, and that is the riskiest thing I've ever proposed to take on beta.  :(

And to make everything trickier, Monday is a holiday in Canada, so I won't be able to start to look at this until the next Tuesday.  I can't predict how long the audit will take, but I'm confident that we should not release 49 with this unfixed.

Comment 30

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #29)
> Bobby and I discussed this on IRC at length.  I'm going to try to capture
> the results here.
> 
> Bobby thinks that instead of fixing things for all nsExtendedPrincipals, we
> should focus on fixing the consumers who would be affected by reading the OA
> of an nsEP and expecting it to contain the correct information for private
> browsing.  He owns this code, so he wins!
> 
> But my biggest worry about this bug isn't about the postMessage() consumer
> at all.  All that code does is get the subject principal (which _could_ be
> an nsEP) and get its OA.

To be clear, I think this case is particularly unique because we're getting the subject principal, taking the OAs on that, _then_ synthesizing a new codebase principal with a different URI, and subsequently calling Equals().

My point is:
* nsEPs mostly appear as subject principals.
* It's mostly nonsensical to ask about any specific OA value of the subject principal (i.e. "what is the private browsing-ness of the subject principal?"), and so callers shouldn't be doing that. 
* Subject principals _are_ used for privileged checks. But the code that considers OAs during privilege checks (i.e. the implementation of Subsumes) is already ns-EP aware, and does the right thing (i.e. checking each member principal and allowing access if any of the member principals allows access).

So my suspicion is that this is a pretty unique problem, and we can fix it at the callsite. I don't agree that we should block 49 on a complete audit, but Ehsan owns PB, so he wins! ;-)

In terms of the specific fix for postMessage: The code in question exists to handle the fact that the 'origin' argument to postMessage is a URI string, but we need to make a principal out of it to do the check (and a principal is URI + OAs). So it tries to do a halfway-sensible thing by taking the OAs of the caller and creating a new codebase principal for the URI string.

However, it seems like we should be fine just using the OAs of the receiver (i.e. only comparing the codebase part of the principal). IIRC, the 'receiver' argument to postMessage is designed to solve a security gotcha where somebody might postMessage to a window and then have it navigate to something else. But given that OAs generally don't change on navigation and OAs are browser-internal anyway, it's hard for me to think of a case where the sender would leak sensitive data to an unexpected recipient by this mechanism.

Boris, what do you think about the specific fix for postMessage?

Comment 31

[Bobby Holley (:bholley)]

Also, Ehsan says that Cu.getObjectPrincipal doesn't give the right answer. I'm quite skeptical of this - the latter doesn't examine the SandboxPrivate directly, but it does use the principal of the compartment, which should always match the principal of the SandboxPrivate (if it doesn't, we have a serious bug!). So hopefully he can investigate that.

Comment 32

[Andrew Zitnay]

Is Hal's case in comment 11 definitely the same bug I submitted?  The postMessage() issue was easy enough to work around on our side, and we've already released a patch.  However, The errors in the console for Hal's comment 11 case look pretty different, and I don't see an easy workaround.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> However, it seems like we should be fine just using the OAs of the receiver

That thought had crossed my mind.  That would incidentally make postMessage from system code to random content windows work when the system code specifies a target origin, right?

Fundamentally, I expect us to make different-OA things unreachable, so if you can reach a window you should be able to postMessage to it assuming the non-OA things match, yes?

Comment 34

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> > However, it seems like we should be fine just using the OAs of the receiver
> 
> That thought had crossed my mind.  That would incidentally make postMessage
> from system code to random content windows work when the system code
> specifies a target origin, right?

Right. Right now I'd imagine it already works if you do '*'.
 
> Fundamentally, I expect us to make different-OA things unreachable, so if
> you can reach a window you should be able to postMessage to it assuming the
> non-OA things match, yes?

Well, I think the issue is more that the 'receiver' argument isn't about protecting the receiver from the caller (since anyone can postMessage cross-origin with '*'), but rather protecting the caller from the receiver. So it's not so much "are you allowed to postmessage to it given your OAs" but more "might you accidentally leak to an unintended recipient if we don't compare the OAs?"

Given that, I don't find the threat model compelling.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8786979 [details] [diff] [review]
Use the OriginAttributes associated with a window principal when creating a Sandbox with an expanded principal

Resetting the flags.
Looks like we will have to do a second rc for that...

Comment 36

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #31)
> Also, Ehsan says that Cu.getObjectPrincipal doesn't give the right answer.
> I'm quite skeptical of this - the latter doesn't examine the SandboxPrivate
> directly, but it does use the principal of the compartment, which should
> always match the principal of the SandboxPrivate (if it doesn't, we have a
> serious bug!). So hopefully he can investigate that.

I haven't investigated, but testing that getObjectPrincipal() and getSandboxPrincipal() return different things is quite simple.  I filed bug 1300800 in case you or someone else wants to investigate.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ehsan, Bobby, this is currently marked as blocking for Fx49. Do you think we are ready with an updated patch to address some of the concerns Bobby had with the original patch? We will gtb RC2 later today/tomm so time is of essence here. Thanks!

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

I already talked with Ehsan this morning and he is auditing now (as in comment 29).  
I am not looking to gtb on RC2 before Wednesday afternoon as we are also waiting on some other fixes.

Comment 39

[(no longer active)]

I'm filing dependencies on this bug for what I'm discovering in my audit...

Comment 40

[(no longer active)]

Attachment: Backout bug 1278664, bug 1280105 and bug 1269361 from Firefox 49; a=lizzard

I have now found and filed 5 dependencies for this bug by auditing everywhere that we currently access the subject principal in Gecko.  Based on the extent of the problem and given how our fundamental assumptions (that we never create DOM windows with expanded principals) proved to be false, I'm not comfortable with fixing anything on beta besides a full backout of the original bug and everything that has landed on top of it.  This patch is the backout patch I have prepared.  I'll need to post it to try to make sure it will stick, but I have already tested it to make sure that lastpass will work after this patch.

Comment 41

[(no longer active)]

Comment on attachment 8788620 [details] [diff] [review]
Backout bug 1278664, bug 1280105 and bug 1269361 from Firefox 49; a=lizzard

Approval Request Comment
[Feature/regressing bug #]: Bug 1269361
[User impact if declined]: Lastpass and perhaps other add-ons stop working in private windows
[Describe test coverage new/current, TreeHerder]: Not tested besides ensuring that it fixes Lastpass.  Try server testing pending.
[Risks and why]: I have tried to find everything that has landed on top of bug 1269361 in the 49 time frame which depends on it.  Needless to say, I may have missed stuff, but I'm not sure where else I would look for more things to back out.  This is still very very risky for a patch to be landed on the last RC, but most of the risk is in me having missed something that needs to be backed out which won't be uncovered by build/test failures, which will hopefully be unlikely.  That all being said, this is pretty much our only realistic option for 49, so it's not like we're picking the least risky alternative or something.
[String/UUID change made/needed]: None

Note that this patch isn't intended to land anywhere but mozilla-beta.  It means that it will skip central and aurora, where we should take the real fixes for this bug and its dependencies.

Comment 42

[(no longer active)]

Comment on attachment 8788620 [details] [diff] [review]
Backout bug 1278664, bug 1280105 and bug 1269361 from Firefox 49; a=lizzard

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b08140ac49d1

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8788620 [details] [diff] [review]
Backout bug 1278664, bug 1280105 and bug 1269361 from Firefox 49; a=lizzard

Backout for 49 blocking issue; we should land this only on m-b and m-r. 
This should make it into tomorrow's RC2 build.

Comment 44

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

https://hg.mozilla.org/releases/mozilla-beta/rev/9f839de0688c9c8b9f4d4c831b599169057aa9e9
https://hg.mozilla.org/releases/mozilla-release/rev/9f839de0688c9c8b9f4d4c831b599169057aa9e9

Comment 45

[(no longer active)]

Bug 1300908 is an example of how terrible it is that we cannot have a useful OA for expanded principals.

Comment 46

[(no longer active)]

(In reply to Wes Kocher (:KWierso) from comment #44)
> https://hg.mozilla.org/releases/mozilla-beta/rev/
> 9f839de0688c9c8b9f4d4c831b599169057aa9e9
> https://hg.mozilla.org/releases/mozilla-release/rev/
> 9f839de0688c9c8b9f4d4c831b599169057aa9e9

Wes, I hope landing a super experimental patch on the beta tree that hasn't even yet passed the try server was actually what you intended to do.  I wasn't really ready for the patch to be checked in, but if it ends up sticking, great!

Comment 47

[Bobby Holley (:bholley)]

(In reply to Ritu Kothari (:ritu) from comment #37)
> Hi Ehsan, Bobby, this is currently marked as blocking for Fx49. Do you think
> we are ready with an updated patch to address some of the concerns Bobby had
> with the original patch? We will gtb RC2 later today/tomm so time is of
> essence here. Thanks!

Just FYI, I'm pretty busy with meetings in Taipei this week, and am on PTO next week. I'll try to respond to email and bugzilla when I can, but I'll be laggy and don't have the cycles to write any code. :\

Comment 48

[Liz Henry (:lizzard) (relman/hg->git project)]

Ehsan, I didn't realize you weren't ready.... What would you like to do here?

Comment 49

[(no longer active)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #48)
> Ehsan, I didn't realize you weren't ready.... What would you like to do here?

Looks like it stuck!  All is fine by me now!

Comment 50

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on 51.0a1 (2016-09-07).
Verified fixed FX 49.0 build 2 on Win 7, Win 10, Ubuntu 16.04, OS X 10.11.

Comment 51

[Andrew Zitnay]

(In reply to Paul Silaghi, QA [:pauly] from comment #50)
> Reproduced the issue on 51.0a1 (2016-09-07).
> Verified fixed FX 49.0 build 2 on Win 7, Win 10, Ubuntu 16.04, OS X 10.11.

Is there a place I can download this "FX 49.0 build 2"?  This issue still reproduces on the latest beta, developer edition, and nightly channels.

Comment 52

[Paul Silaghi, QA [:pauly]]

https://archive.mozilla.org/pub/firefox/candidates/49.0-candidates/build2/

Comment 53

[(no longer active)]

Attachment: Part 1: Ensure that nsIScriptSecurityManager.createExpandedPrincipal() is restricted to tests

Comment 54

[(no longer active)]

Attachment: Part 2: Allow specifying an OriginAttribute when creating an expanded principal

Comment 55

[(no longer active)]

Attachment: Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes

This patch allows specifying an OriginAttributes when creating a sandbox
using Components.utils.Sandbox() by specifying an originAttributes
member on the options dictionary.

If an OA is specified in this way, it is used for creating codebase
principals from the string arguments passed to the function.  Otherwise,
if one or more principals are passed in the array argument to Sandbox(),
the OA of the principal(s) is used to construct codebase principals from
the strings inside the array.  In this case, we check to make sure that
all of the passed principals have the same OA, otherwise we'll throw an
exception.

In case no explicit OA is specified and no principals are passed in the
array argument, we create the codebase principals using a default OA.

Comment 56

[(no longer active)]

Attachment: Part 4: Add an OriginAttributes argument to nsIScriptSecurityManager.createExpandedPrincipal()

Comment 57

[(no longer active)]

Attachment: Part 5: Specify an OriginAttribute for the XBL content's expanded principal

Comment 58

[(no longer active)]

Attachment: Part 6: Require passing an OriginAttribute when constructing an nsExpandedPrincipal

Comment 59

[Bobby Holley (:bholley)]

Comment on attachment 8793402 [details] [diff] [review]
Part 1: Ensure that nsIScriptSecurityManager.createExpandedPrincipal() is restricted to tests

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

I'm not wild about the pref. I'd rather just have the two callers polyfill createExpandedPrincipal with Cu.getObjectPrincipal(new Cu.Sandbox([...])).

Comment 60

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #59)
> I'm not wild about the pref. I'd rather just have the two callers polyfill
> createExpandedPrincipal with Cu.getObjectPrincipal(new Cu.Sandbox([...])).

(And then just remove createExpandedPrincipal).

Comment 61

[Bobby Holley (:bholley)]

Comment on attachment 8793404 [details] [diff] [review]
Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes

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

::: js/xpconnect/src/Sandbox.cpp
@@ +1359,5 @@
> +    //
> +    // The effective OA selected above will also be set as the OA of the
> +    // expanded principal object.
> +    const uint32_t MAX_PASS = 2;
> +    for (uint32_t pass = 0; pass < MAX_PASS; ++pass) {

Per IRC discussion I'd like to separate this into two loops.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +294,5 @@
>        this.sandbox = Cu.Sandbox(contentWindow, {
>          sandboxPrototype: contentWindow,
>          wantXrays: false,
>          isWebExtensionContentScript: true,
> +        originAttributes: attrs,

Given that there's no EP involved here, there's no reason to pass |attrs| AFAICT.

Comment 62

[Bobby Holley (:bholley)]

Comment on attachment 8793405 [details] [diff] [review]
Part 4: Add an OriginAttributes argument to nsIScriptSecurityManager.createExpandedPrincipal()

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

Per above I think we can remove this API.

Comment 63

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #61)
> Per IRC discussion I'd like to separate this into two loops.
> 
> ::: toolkit/components/extensions/ExtensionContent.jsm
> @@ +294,5 @@
> >        this.sandbox = Cu.Sandbox(contentWindow, {
> >          sandboxPrototype: contentWindow,
> >          wantXrays: false,
> >          isWebExtensionContentScript: true,
> > +        originAttributes: attrs,
> 
> Given that there's no EP involved here, there's no reason to pass |attrs|
> AFAICT.

There is, since the OA's addonId has been modified: <http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#273>

Comment 64

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #63)
> There is, since the OA's addonId has been modified:
> <http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionContent.jsm#273>

The originAttributes SandboxOption is entirely ignored in the non-array case.

Comment 65

[(no longer active)]

Attachment: Part 1: Remove nsIScriptSecurityManager.createExpandedPrincipal()

Comment 66

[(no longer active)]

Attachment: Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes

This patch allows specifying an OriginAttributes when creating a sandbox
using Components.utils.Sandbox() by specifying an originAttributes
member on the options dictionary.

If an OA is specified in this way, it is used for creating codebase
principals from the string arguments passed to the function.  Otherwise,
if one or more principals are passed in the array argument to Sandbox(),
the OA of the principal(s) is used to construct codebase principals from
the strings inside the array.  In this case, we check to make sure that
all of the passed principals have the same OA, otherwise we'll throw an
exception.

In case no explicit OA is specified and no principals are passed in the
array argument, we create the codebase principals using a default OA.

Comment 67

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33fa40e847a4

Comment 68

[Bobby Holley (:bholley)]

Comment on attachment 8793525 [details] [diff] [review]
Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes

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

r=me, but please give the big function a look-over before you land it to check sanity and readability. This code is security-sensitive and clarity matters.

::: js/xpconnect/src/Sandbox.cpp
@@ +1335,5 @@
> +    // Otherwise, we will use the origin attributes of the passed object(s). If
> +    // more than one object is specified, we ensure that the OAs match.
> +    bool originAttrsSet = false;
> +    bool originAttrsForced = false;
> +    PrincipalOriginAttributes attrs;

Can you use a Maybe<> for this instead of having the separate originAttrsSet boolean?

@@ +1349,5 @@
> +    }
> +
> +    // Now we go over the array in two passes.  In the first pass, we ignore
> +    // strings, and only process objects.  Assuming that no originAttributes
> +    // option has been passed, if we encounter a principal or SOP  object, we

Nit: extra space.

@@ +1380,5 @@
>              if (sop)
>                  principal = sop->GetPrincipal();
> +            NS_ENSURE_TRUE(principal, false);
> +
> +            const PrincipalOriginAttributes prinOA =

I'd call this prinAttrs or currAttrs or something - it's weird to have some locals named "OA" and some named "attrs".

@@ +1390,5 @@
> +                // If attrs comes from OriginAttributes, we don't need
> +                // this check.
> +                return false;
> +            }
> +            if (!originAttrsSet && !originAttrsForced) {

Can you structure this as:

if (!originAttrsForced) {
  prinOA = ...;
  if (!originAttrsSet) {
     ...
  } else if (prinOA != attrs) {
    return false;
  }
}

@@ +1394,5 @@
> +            if (!originAttrsSet && !originAttrsForced) {
> +                attrs = prinOA;
> +                originAttrsSet = true;
> +            }
> +        } else if (!allowed.isString()) {

I think it would be slightly clear to do:

else if (allowed.isString() {
  // Skip any string arguments - we handle them in the next pass.
} else {
  // Don't know what this is.
  return false;
}

@@ +1409,5 @@
> +                return false;
> +            }
> +            allowedDomains[i] = principal;
> +        }
> +    }

With the Maybe<> implementation, we'll want to check if they've been created yet and, if not, instantiate with the default attributes. This has the side-benefit of making the fallback case more clear.

@@ +1429,5 @@
> +            // the principal found before, so we can use it here.
> +            if (!ParsePrincipal(cx, str, attrs, getter_AddRefs(principal)))
> +                return false;
> +            NS_ENSURE_TRUE(principal, false);
> +        } else if (!allowed.isObject()) {

We should be able to MOZ_ASSERT against this case.

@@ +1434,5 @@
> +            return false;
> +        }
> +
> +        if (principal) {
> +            allowedDomains[i] = principal;

I think it would be clearer to put this at the end of the isString() branch, which would remove the need for the null check.

Comment 69

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adad6bddaa18
Part 1: Remove nsIScriptSecurityManager.createExpandedPrincipal(); r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/52bc3ac33f86
Part 2: Allow specifying an OriginAttribute when creating an expanded principal; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea63f0e0a772
Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/13b7516823e5
Part 4: Specify an OriginAttribute for the XBL content's expanded principal; r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/717d85fc2046
Part 5: Require passing an OriginAttribute when constructing an nsExpandedPrincipal; r=bholley

Comment 70

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/adad6bddaa18
https://hg.mozilla.org/mozilla-central/rev/52bc3ac33f86
https://hg.mozilla.org/mozilla-central/rev/ea63f0e0a772
https://hg.mozilla.org/mozilla-central/rev/13b7516823e5
https://hg.mozilla.org/mozilla-central/rev/717d85fc2046

Comment 71

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/adad6bddaa18
https://hg.mozilla.org/mozilla-central/rev/52bc3ac33f86
https://hg.mozilla.org/mozilla-central/rev/ea63f0e0a772
https://hg.mozilla.org/mozilla-central/rev/13b7516823e5
https://hg.mozilla.org/mozilla-central/rev/717d85fc2046

Comment 72

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ehsan, should we uplift this fix to Aurora51 and Beta50?

Comment 73

[(no longer active)]

(In reply to Ritu Kothari (:ritu) from comment #72)
> Hi Ehsan, should we uplift this fix to Aurora51 and Beta50?

Yes, but I would like to wait for a few days on Nightly to make sure this didn't break anything too severely.  I have setup a reminder for myself next Wednesday to nominate this for approval.

Comment 74

[(no longer active)]

Comment on attachment 8793525 [details] [diff] [review]
Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes

(I'm requesting approval on the main patch in this series, but we need to backport all patches on the bug.)

Approval Request Comment
[Feature/regressing bug #]: Bug 1269361
[User impact if declined]: See comment 0.  Some add-ons may be broken in private windows.
[Describe test coverage new/current, TreeHerder]:  This patch comes with automated tests, and it's been baking on Nightly for a few days.
[Risks and why]: This is mildly risky, in that we're changing the semantics of an API that is used by extensions so in theory it can cause add-on bustage.  I spent some time looking through the addons DXR and didn't manage to find an add-on that may be affected but obviously I didn't examine every single add-on, but it seems like the coding pattern which can potentially cause an add-on to break because of this is extremely rare.  That being said, any add-on that may break because of this is probably already broken in a way similar to LastPass in this bug.  Given the amount of time we spent looking into alternative ways of fixing this issue, I'm fairly convinced that this is the least risky approach.
[String/UUID change made/needed]: None.

Comment 75

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8793525 [details] [diff] [review]
Part 3: Ensure that the expanded principal of a sandbox has a sensible OriginAttributes

Given the severity of this regression, I am ok to take it in Beta50 rather quickly so we can get more folks testing and catch any regressions sooner. FWIW, it has baked on Nightly for a few days. Aurora51+, Beta50+

Comment 76

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/15c5875f5d24
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/14e6fbd7d68a
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5e2e45ed61d7
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f6cf110b2871
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/72f6089e120d

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/023ebfa3cf3d
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/06dd44643f53
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9431fe63e758
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2c494a5c88a4
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/5237c35095f4

Comment 77

[Paul Silaghi, QA [:pauly]]

Removing qe-verify+ since this is already in 49 release.