1396399 - (CVE-2018-5116) An extension can XSS any domain with only the ActiveTab permission using frames

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, defect, P2)

Description

[Ronen Zilberman]

Attachment: poc.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622

Steps to reproduce:

The ActiveTab permission allows an extension to only access the current tab (after the user interacted with the extension). This is to allow "extensions to fulfill a common use case, without having to give them very powerful permissions" like <all_urls> (quote from the documentation).

However, it was found that this permission also allows access to frames within the tab that can be of a different domain. This allows a malicious extension, that requested only a very strict permission set, to still interact with any domain:
1. The extension injects code into the tab that creates a hidden iframe to an arbitrary domain.
2. The extension then injects code into that frame creating an XSS situation. This obviously allows for many kinds of nefarious actions such as theft of information, request forgery etc.


--------------------------------------------------------------------------
To recreate:
1. Inject code that creates a hidden iframe
2. Wait for the frame to load
3. inject any script into that frame

I provide a POC extension that shows a browser action (button). When clicked, a hidden iframe to "https://example.com" is created in the current tab and the cookies for that URL are console.log()ed. 
The extension only requests activeTab yet this works regardless of the URL of the active tab.


Actual results:

The malicious code runs in all the frames in the current tab, including the iframe that was just created.


Expected results:

The activeTab should allow access to the parent frame only, and not to any inner frames. This is the behaviour in the Chrome browser.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So what you are saying is that you expect 'activeTab' to only grant a permission to the origin of the top level frame? What happens if the page actually contains cross-domain iframes? The add-on can not interact with them? That's not very intuitive to me, but its interesting if that is the way chrome has implemented it.  Our model might have been a deliberate design decision, I dont know.

(NB We haven't documented this behavior either way; I know this as I am currently writing the security model documentation at the moment for Firefox's implementation of browser extensions).

Kris, Andrew - what do you think about this? Firstly is this something we would like to change? On one hand I think it makes the security model more consistent (i.e. you explicitly need the host permission to in order to executeScript in a frame) but the flipside is that might break extensions which rely on the currently behavior (and it might also result in all extensions requesting the <all_urls> permission. I tend to agree with the reporter here though - while activeTab isn't quite as permissive as <all_urls> 

If we are going to change this, then I assume we would also need to change the behavior of the tabs and activeTab APIs, to grant a permission to the current frame. (and then I wonder, would that be scoped to the actual page or the origin, ie can you inject script into same origin iframes with different URLs. Origin would probably make the most sense). 

If we DON'T want to make this change, then we should at least ensure that AMO and reviewers treat tab & activeTab with the same level of caution. (in line with <all_urls> )

[Tracking Requested - why for this release]:
This seems like an important incompatibility with chrome if the behavior is as described - if we didn't do this deliberately, its probably something we want to address.

Comment 3

[Kris Maglione [:kmag]]

This behavior is intentional, yes. If Chrome behaves differently, that's certainly not the way it was documented when we implemented this. If we were to implement different restrictions, we'd probably still want to allow access to sub-frames that are same-origin with the root.

That said, I'm not sure that restricting access to subframes is what we want. Unless perhaps we only include subframes that were already present when the activeTab permission was granted.

But, really, any domain that's worth trying to hijack this way should set appropriate X-Frame-Options headers to prevent it being loaded in a subframe to begin with.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Honestly the documentation is all over the shop on this topic(both ours and theirs). But I think there is no point implementing restrictions if we allow subframes (there is no way to tell between legit sub-frames and injected sub-frames, right?). I'd agree with you x-frame-options, except that most sites don't set this even though they should.

From digging a little more into the chrome documentation, I don't think we are meeting the intention of the activeTab permission[1]. IIUC The design goals were to provide a permission which was NOT as dangerous as <all_urls>, and hence they only allow scripting to the current page, until the page has been navigated. If don't restrict the sub-frame scripting then activeTab is as dangerous as <all_urls> and in fact worse, since the implication isn't clear.

[1] https://developer.chrome.com/extensions/activeTab#motivation

And what our own documentation says is quoted above already ( "The intention of this permission is to enable extensions to fulfill a common use case, without having to give them very powerful permissions.")

Currently activeTab effectively as dangerous as <all_urls>, just with the added restriction of requiring user interaction prior to execution. Of course, the other part of the story is that chrome doesn't show the activeTab permission to the user. Which seems deceptive to me. (I guess their assumption is that if the user is interacting with addon via a browser/page action, then this signals intent for the permission grant - and as such it would make sense to block sub-frames since the user cant be aware of the other origins that might be framed.)

So I actually think that we should 'fix' this, and block script execution in subframes, unless the extension has the correct permission.

Comment 5

[Ronen Zilberman]

I agree that it's confusing. Being called activeTab (and not activeTopFrameOfTab) it implies the entire tab. My own extension (Check4Change) would have wanted a "real" activeTab but would regrettably need to request <all_urls> in order to access subframes in the current tab.

However, leaving this unfixed is too dangerous (IMO) as, like was said above, it is "effectively as dangerous as <all_urls>" but without letting the user know.

One can overcome the X-Frame-Options restriction by loading URLs like these:
https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png
https://www.facebook.com/ajax/bz

The header doesn't mitigate the attack as it is intended to stop clickjacking and therefor only needed in the top-level pages (where there are actions to be done). It is not present (nor needs to be) in inner elements (in the examples about: The google logo, or a facebook ajax point). But of course an iframe can load an image and once it does it is within the Same-Origin of its domain, allowing execution of client side scripts.

Comment 6

[Frederik Braun [:freddy]]

We'll want to fix this (i.e., align it with Chrome) for various reasons. Security and web-extension compatibility.

Comment 7

[Andy McKay]

After fiddling with the poc a little I can't reproduce in Chrome. I agree with Paul and Frederik and think we should fix this in the 57 timeline to be consistent with Chrome if we can.

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Attachment: PoC modified to work in both chrome and firefox

This should work in both Chrome and Firefox.

STR:

1. Install addon as temporary addon
2. navigate to webpage that isnt exmaple.com (e.g. https://misuse.co)
3. press the browser action button installed by the extension

Results:

In chrome, one alert fires (for the top level domain)
In Firefox, two alerts fire (for both domains)

Comment 9

[Shane Caraveo (:mixedpuppy)]

My 2c.  I wonder why a malicious addon wouldn't just request the permissions it needs to access subframes anyway.  Maybe a few less people will install, but they wouldn't have to deal with limitations.

Comment 10

[Andy McKay]

Its a fair point and why it's a P2 and not a blocker in my opinion.

Legacy add-ons could of course just do all this with no prompting at all, but hey we should probably get the permissions matching expectations.

Comment 11

[Ronen Zilberman]

My 2c reply. One of the majors reasons moving away from legacy add-ons in the first place was security. The idea that add-ons couldn't just do anything they wanted anymore but rather had to ask for granular permissions and thus giving control back to the users to make an informed decision. 

It's definitely arguable if this security mechanism (permissions) is broken by design because most users just click "ok" anyway (similar to Android). However it is still *a* security mechanism of the browser. It would be much simpler to do away with it entirely and just grant all permissions to all add-ons, but I assume no one would advocate for that as the simplicity doesn't outweigh the security benefits.

----------

I've noticed the bug was labeled "sec-moderate". It is my feeling that according to the guidelines (https://wiki.mozilla.org/Security_Severity_Ratings) the bug doesn't adhere to the description of "sec-moderate", yet fits well as "sec-high". From the doc "sec-high" allows "Obtaining confidential data from other sites the user is visiting, or inject data or code into those sites, requiring no more than normal browsing actions." (assuming installing an add-on is normal browsing actions).

Disclaimer: While I genuinely feel all the above is true, I wouldn't be arguing over semantics if I wasn't excited about the possibility of my bug achieving bug-bounty fame. :)

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Not tracking for 57 (because of comment #10 & sec-moderate)

Comment 13

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> My 2c.  I wonder why a malicious addon wouldn't just request the permissions
> it needs to access subframes anyway.  Maybe a few less people will install,
> but they wouldn't have to deal with limitations.

I probably agree with you there - would the less scary warning be worth it for a malicious add-on writer? I don't know, but it's clear we need to fix this as currently the activeTab permission doesn't fulfill the purpose of its existence.

The whole reason for the existence of "activeTab" permission is that its supposed to be a less dangerous permission, and that's the justification for why we _show nothing_ in the install permission prompt when a add-on has this permission. The idea with ActiveTab is that since the user has interacted with the tab (browser/page action, context menu etc) [1] then there is an implicit permission grant to interact with that page, if the extension has asked that it needs such permission.

So I thinks an important bug to fix, as currently our permission warnings (or lack of) are somewhat misleading imho. 


(In reply to Ronen Zilberman from comment #11)
> My 2c reply....

Totally agree with you here that it needs to be fixed. Security is definitely a motivation for web extensions, but its more about removing the all-powerful nature of legacy extensions than actively trying to giving the user more control.

I do think however that sec-moderate is appropriate though both because the limited value this represents for attacker and inherent limitations of permission granting as a security control (i.e. users don't read prompts). Also activeTab requires user interaction in the form clicking a browser action, page action or context menu button. 

Regardless of the rating, I do think this is worth bounty consideration, and I have flagged it as such. I've been explicitly testing our web extension implementation and I hadn't noticed this bug, so I'm a definitely very grateful here for the bug report here.


[1] https://developer.chrome.com/extensions/activeTab#invoking-activeTab

Comment 14

[Andrew Swan [:aswan]]

I did a little more testing on this in Chrome and empirically, it seems that Chrome distinguishes script-injected content and does not grant access to that content via activeTab.  When this bug was originally filed, it wasn't feasible to implement that behavior in Firefox.  But Kris, with your recent work on bug 1406278 would it be feasible to do that now?

Comment 15

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #14)
> I did a little more testing on this in Chrome and empirically, it seems that
> Chrome distinguishes script-injected content and does not grant access to
> that content via activeTab.  When this bug was originally filed, it wasn't
> feasible to implement that behavior in Firefox.  But Kris, with your recent
> work on bug 1406278 would it be feasible to do that now?

Yeah, to some extent. Script-injected frames should have a triggeringPrincipal set for their src attribute (but not for their srcdoc attribute). But there are other ways to navigate a frame than setting its src property.

Comment 16

[Andrew Swan [:aswan]]

So the rule can be: an extension can only use the activeTab permission to inject a script into a frame if the frame's current location matches its src attribute and its triggeringPrincipal is not an extension principal (or should this check be even more specific?)

If I can get consensus on this, I can put together a patch.  Shotgunning some needinfos...

Comment 17

[Kris Maglione [:kmag]]

The extension can set the src using the page's principal, and we don't currently record any principal for parser-created nodes. We need to actually record when the src was set using a non-script-created parser.

Comment 18

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Andrew Swan [:aswan] from comment #14)
> I did a little more testing on this in Chrome and empirically, it seems that
> Chrome distinguishes script-injected content and does not grant access to
> that content via activeTab.  When this bug was originally filed, it wasn't
> feasible to implement that behavior in Firefox.  But Kris, with your recent
> work on bug 1406278 would it be feasible to do that now?

When you say "script-injected" content - does that mean "any frames that were injected by a script (as opposed to declared in html)"? IE that if a webpage dynamically adds frames to itself with a script, Web Extensions will not have permission to inject into these with only the activetab permission?

Just making sure I understand this clearly (I should test this I suppose)

Comment 19

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Andrew Swan [:aswan] from comment #16)
> So the rule can be: an extension can only use the activeTab permission to
> inject a script into a frame if the frame's current location matches its src
> attribute and its triggeringPrincipal is not an extension principal (or
> should this check be even more specific?)
> 
> If I can get consensus on this, I can put together a patch.  Shotgunning
> some needinfos...

That sounds great to me. Again just to make sure I understand your goal here, the effective permission of activeTab permission would then to allow script injection into:
 a) the top-level window of the current tab
 b) Any iframes which are declared in the HTML of the document

I was expecting just (a), but if you would prefer (a)& (b) for compat reasons, that seems to make sense too. My goal here is to ensure that activeTab is significantly less privileged than <all_urls>.

Comment 20

[Andrew Swan [:aswan]]

(In reply to Paul Theriault [:pauljt] from comment #18)
> (In reply to Andrew Swan [:aswan] from comment #14)
> > I did a little more testing on this in Chrome and empirically, it seems that
> > Chrome distinguishes script-injected content and does not grant access to
> > that content via activeTab.  When this bug was originally filed, it wasn't
> > feasible to implement that behavior in Firefox.  But Kris, with your recent
> > work on bug 1406278 would it be feasible to do that now?
> 
> When you say "script-injected" content - does that mean "any frames that
> were injected by a script (as opposed to declared in html)"? IE that if a
> webpage dynamically adds frames to itself with a script, Web Extensions will
> not have permission to inject into these with only the activetab permission?

Yes, that is the proposal.  We can't reliably distinguish script-injected frames that are created by the page itself from those created by an extension so its really the only safe thing to do.


(In reply to Paul Theriault [:pauljt] from comment #19)
> That sounds great to me. Again just to make sure I understand your goal
> here, the effective permission of activeTab permission would then to allow
> script injection into:
>  a) the top-level window of the current tab
>  b) Any iframes which are declared in the HTML of the document
> 
> I was expecting just (a), but if you would prefer (a)& (b) for compat
> reasons, that seems to make sense too. My goal here is to ensure that
> activeTab is significantly less privileged than <all_urls>.

Right a&b is what Chrome does and is what is proposed here.

Comment 21

[Andrew Swan [:aswan]]

Attachment: wip

This will need reviews from a dom peer and from security but
lets start with telling me how badly I butchered the C++ bits.

MozReview-Commit-ID: ES3OcA1C4Zz

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8925147 [details] [diff] [review]
wip

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

Good start, but I think the original URL tracking needs some work.

::: dom/base/nsContentUtils.cpp
@@ +4758,5 @@
>  /* static */
> +bool
> +nsContentUtils::IsOriginalParserCreatedFrame(nsPIDOMWindowOuter* aWin)
> +{
> +  if (aWin == nullptr) {

Can make this an assertion.

@@ +4768,5 @@
> +    return false;
> +  }
> +
> +  bool dynamic = true;
> +  docshell->GetCreatedDynamically(&dynamic);

We should mark this infallible in the IDL.

@@ +4775,5 @@
> +  }
> +
> +  nsIURI* currentURI = aWin->GetDocumentURI();
> +
> +  nsGenericHTMLFrameElement* frame = static_cast<nsGenericHTMLFrameElement*>(aWin->GetFrameElementInternal());

This should probably be something like `RefPtr<nsGenericHTMLFrameElement> frame = do_QueryObject(...)`, or we should implement nsGenericHTMLFrameElement::FromContent.

@@ +4779,5 @@
> +  nsGenericHTMLFrameElement* frame = static_cast<nsGenericHTMLFrameElement*>(aWin->GetFrameElementInternal());
> +  nsCOMPtr<nsIURI> originalURI;
> +  nsCOMPtr<nsIPrincipal> unusedPrincipal;
> +  nsresult rv = frame->GetResolvedSrcURI(getter_AddRefs(originalURI), getter_AddRefs(unusedPrincipal));
> +  if (rv != NS_OK) {

You should always check NS_SUCCEEDED(rv) rather than comparing against NS_OK.

And you could use something like NS_ENSURE_SUCCESS(rv, true) here, but at this point I'd probably just have this return Result<bool, nsresult> and use MOZ_TRY

@@ +4784,5 @@
> +    return false;
> +  }
> +
> +  bool equal = false;
> +  rv = (originalURI->Equals(currentURI, &equal));

I don't think this will work. It will just tell us the resolved URL of the current src attribute, but that could have changed since the frame was created.

I think we probably need to change the frame element code to clear the mNetworkCreated flag when the src/srcset attributes are changed, rather than when it's inserted into the document.

::: dom/base/nsFrameLoader.cpp
@@ +2774,5 @@
> +    // XBL bindings doc shouldn't load sub-documents.
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIURI> base_uri = mOwnerContent->GetBaseURI();

Urgh. I know you didn't write this, but underscored variable names violate our style guide. If you're changing this anyway, it would be nice to fix that while you're here.

::: dom/base/nsFrameLoader.h
@@ +337,5 @@
>    void ApplySandboxFlags(uint32_t sandboxFlags);
>  
>    void GetURL(nsString& aURL, nsIPrincipal** aTriggeringPrincipal);
>  
> +  nsresult GetResolvedURI(nsIURI** aURI, nsIPrincipal** aTriggeringPrincipal);

Nit: Please document.

::: toolkit/components/extensions/WebExtensionContentScript.h
@@ +49,5 @@
>    // be called for codebase principals.
>    const URLInfo& PrincipalURL() const;
>  
>    bool IsTopLevel() const;
> +  bool HasActiveTabPermission() const;

This name is a bit misleading. It seems like it should be something like IsNetworkCreated()

::: toolkit/components/extensions/WebExtensionPolicy.cpp
@@ +303,5 @@
>  WebExtensionContentScript::WebExtensionContentScript(WebExtensionPolicy& aExtension,
>                                                       const ContentScriptInit& aInit,
>                                                       ErrorResult& aRv)
>    : mExtension(&aExtension)
> +  , mHasActiveTabPermission(aInit.mHasActiveTabPermission)

Can we add an assertion that this is always false for manifest-registered content scripts?

@@ +360,5 @@
> +  if (AddonManagerWebAPI::IsValidSite(aDoc.PrincipalURL().URI())) {
> +    return false;
> +  }
> +
> +  if (mHasActiveTabPermission && aDoc.HasActiveTabPermission()) {

I don't think this is right. I think we probably want a `requireParserCreated` flag, or something, and to return false if it's not set, rather than ignoring the URL match patterns in the opposite case.

Comment 23

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #22)
> @@ +360,5 @@
> > +  if (AddonManagerWebAPI::IsValidSite(aDoc.PrincipalURL().URI())) {
> > +    return false;
> > +  }
> > +
> > +  if (mHasActiveTabPermission && aDoc.HasActiveTabPermission()) {
> 
> I don't think this is right. I think we probably want a
> `requireParserCreated` flag, or something, and to return false if it's not
> set, rather than ignoring the URL match patterns in the opposite case.

Actually, ignore this. Your way is better. If an extension has permissions for a
frame, it should always be able to inject into it. If it doesn't, we should fall
back to active tab permissions.

Comment 24

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

Still needs a test which is going to be tricky since all the apis that grant
the activeTab permission are browser- or mobile-specific.  But for now here's
the patch to look at.

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8925431 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

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

r=me for the extension parts, but they still needs tests (in a separate patch).

The docShell changes need DOM peer review.

::: docshell/base/nsDocShell.cpp
@@ +2029,5 @@
>  
> +  if (mHasLoadedNonBlankURI) {
> +    bool equal;
> +    if (NS_SUCCEEDED(mCurrentURI->Equals(aURI, &equal))) {
> +      mHasNavigatedFromInitialPage = !equal;

I think this should be something like `if (!succeeded || !equal) mHasNavigated = true`

Otherwise, it should be possible to reset the flag.

Also, we probably want to skip this if LOCATION_CHANGE_SAME_DOCUMENT is set.

::: toolkit/components/extensions/WebExtensionContentScript.h
@@ +49,5 @@
>    // be called for codebase principals.
>    const URLInfo& PrincipalURL() const;
>  
>    bool IsTopLevel() const;
> +  bool ShouldGetActiveTabPermission() const;

Maybe s/Get/Match/?

::: toolkit/components/extensions/WebExtensionPolicy.cpp
@@ +88,5 @@
>    }
>  
>    mContentScripts.SetCapacity(aInit.mContentScripts.Length());
>    for (const auto& scriptInit : aInit.mContentScripts) {
> +    MOZ_ASSERT(!scriptInit.mHasActiveTabPermission);

Maybe make this a diagnostic assert? And set it to false, after the check.

@@ +369,1 @@
>    return MatchesURI(aDoc.PrincipalURL());

Maybe make this an else, just to make it clear that the last `if` breaks the pattern above of returning false on failure, but never returning true.

@@ +447,5 @@
> +  if (mShouldGetActiveTabPermission.isNothing()) {
> +    struct Matcher
> +    {
> +      bool match(Window aWin) {
> +        // XXX ASSERT aWin not null

Why not just add the assertion?

@@ +461,5 @@
> +
> +        if (docshell->GetCreatedDynamically() ||
> +            docshell->GetHasNavigatedFromInitialPage()) {
> +          return false;
> +        }

Maybe something like:

   if (nsIDocShell* docShell = aWin->GetDocShell()) {
     return !docShell->GetCreatedDynamically() && !docShell->GetHasNavigetedFromInitialPage();
   }
   return false;

Comment 26

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25)
> ::: toolkit/components/extensions/WebExtensionPolicy.cpp
> @@ +88,5 @@
> >    }
> >  
> >    mContentScripts.SetCapacity(aInit.mContentScripts.Length());
> >    for (const auto& scriptInit : aInit.mContentScripts) {
> > +    MOZ_ASSERT(!scriptInit.mHasActiveTabPermission);
> 
> Maybe make this a diagnostic assert? And set it to false, after the check.

aInit is const, do you suggest making a copy?  That seems like small overhead but totally unnecessary in most (hopefully all) cases.

> @@ +447,5 @@
> > +  if (mShouldGetActiveTabPermission.isNothing()) {
> > +    struct Matcher
> > +    {
> > +      bool match(Window aWin) {
> > +        // XXX ASSERT aWin not null
> 
> Why not just add the assertion?

Whoops, i didn't mean to leave that in, I just removed the comment.

Comment 27

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #26)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #25)
> > ::: toolkit/components/extensions/WebExtensionPolicy.cpp
> > @@ +88,5 @@
> > >    }
> > >  
> > >    mContentScripts.SetCapacity(aInit.mContentScripts.Length());
> > >    for (const auto& scriptInit : aInit.mContentScripts) {
> > > +    MOZ_ASSERT(!scriptInit.mHasActiveTabPermission);
> > 
> > Maybe make this a diagnostic assert? And set it to false, after the check.
> 
> aInit is const, do you suggest making a copy?  That seems like small
> overhead but totally unnecessary in most (hopefully all) cases.

Hm. Fair enough. Maybe just throw and return rather than asserting, then.

Comment 28

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 29

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 30

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8926757 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

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

I'm just going to assume you intended to r? me on this.

::: docshell/base/nsDocShell.cpp
@@ +2035,5 @@
>  
> +  if (mHasLoadedNonBlankURI && !(aLocationFlags & LOCATION_CHANGE_SAME_DOCUMENT)) {
> +    bool equal;
> +    nsresult rv = mCurrentURI->Equals(aURI, &equal);
> +    if (!NS_SUCCEEDED(rv) || !equal) {

No need for the `rv` here. This would normally be written as `if (NS_FAILED(Equal(&equal)) || !equal)`

::: toolkit/components/extensions/WebExtensionPolicy.cpp
@@ -378,5 @@
>    }
>  
> -  if (AddonManagerWebAPI::IsValidSite(aURL.URI())) {
> -    return false;
> -  }

The more I think about this, the more I think we should also keep this check in `MatchesURI`, even if it is redundant. We don't currently call this function anywhere else, but we do expose it to JS, and it might lead to a future footgun.

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8926760 [details] [diff] [review]
Test for content scripts and activeTab permission

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

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ +643,5 @@
>          extension.testMessage(...aMessage.data.args);
>          return undefined;
>        }
>  
> +      case "SPExtensionGrantActiveTab": {

SpecialPowers APIs are so ugly... *sigh*

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_activeTab.html
@@ +31,5 @@
> +      let frame = document.getElementById("frame");
> +      frame.addEventListener("load", () => {
> +        browser.test.sendMessage("frame-navigated");
> +      }, {once: true});
> +      frame.src = "http://test2.example.com";

Nit: Please add a trailing / to the URL. It's technically illegal without one, but it gets fixed-up during parsing.

@@ +41,5 @@
> +        try {
> +          result = await browser.tabs.executeScript(tab.id, {
> +            allFrames: true,
> +            code: "window.location.href;",
> +          });

Maybe something like `let result = await executeScript(...).catch(err => ({error: err.message}))` ?

@@ +87,5 @@
> +
> +  extension.sendMessage("execute");
> +  result = await extension.awaitMessage("result");
> +  is(result.length, 2, "Script was injected into 2 frames");
> +  let hosts = result.map(href => new URL(href).hostname);

Nit: Maybe just send back `location.origin` from the content script rather than parsing the URLs for hostnames in every test?

@@ +89,5 @@
> +  result = await extension.awaitMessage("result");
> +  is(result.length, 2, "Script was injected into 2 frames");
> +  let hosts = result.map(href => new URL(href).hostname);
> +  ok(hosts.includes(BASE_HOST), "Script was injected into base page");
> +  ok(hosts.includes(FRAME_HOST), "Script was injected into original frame");

Maybe just .sort() and use `Assert.deepEqual()` rather than checking the length and then doing two .include() checks? Would make it a bit clearer exactly what's going on...

Comment 33

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 34

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 35

[Andrew Swan [:aswan]]

Comment on attachment 8927170 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

carry over r+ from kmag from the previous patch after addressing his comments

Comment 36

[Andrew Swan [:aswan]]

Comment on attachment 8927171 [details] [diff] [review]
Test for content scripts and activeTab permission

carry over r+ from kmag from the previous patch after addressing his comments

Comment 37

[Andrew Swan [:aswan]]

Comment on attachment 8927170 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Relatively easily by somebody who understands the code

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The tests are in a separate patch that can be landed separately.  I've tried to avoid any comments in the code or the checkin summary that point toward the problem.

Which older supported branches are affected by this flaw?
I believe all supported branches (release, beta, esr) are affected.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This should backport easily to 57.  A backport to ESR would be rather difficult.

How likely is this patch to cause regressions; how much testing does it need?
The patch touches docshell which is heavily used, but it is small and follows our established practices so the risk should be low.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on November 28, two weeks into the new cycle. Please do not check in before that date.

We'll want this on other branches so branch patches should be made and nominated at that time as well. If you don't think we can easily do ESR52 (as mentioned in comment 37) then we can go without it since it isn't that long until the next ESR branch.

Comment 39

[Andrew Swan [:aswan]]

bz: this patch needs review from a DOM peer, I'm sure you're busy, should I redirect to somebody else?
This is still a week and a half from landing but I'm going to be on PTO most of next week and was hoping to get the review done before leaving.  If you're pinched for time, focusing on the docshell bits and ignoring the webextensions bits (which kmag reviwed) should be fine.

And abillings: should the patch and the tests be checked in on 11/28 or should the tests be held for some time?

Comment 40

[Boris Zbarsky [:bzbarsky]]

I'm working on this as we speak.  I've been trying to make sure I actually think through the threat model here...

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8927170 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

> Bug 1396399 Clarify rules for applying activeTab permission to content scripts

This needs a much better commit message, unless that would expose the security issue.  It should describe what the new rules are and why the particular rules we're changing to are the ones we want.  This is part of what made this review so slow.  I've had to reverse-engineer the intent from the patch.  I probably should have just brought this up immediately instead.  :(

>+  , mHasNavigatedFromInitialPage(false)

I don't understand the desired semantics of mHasNavigatedFromInitialPage.

If I do window.open(url), should that really have different semantics from:

  var w = window.open();
  setTimeout(function() { w.location.href = url }, 100);

?  It looks to me like the former would have mHasNavigatedFromInitialPage false while the latter has it true.

Same thing for <iframe> created without src and then getting it src attribute set.

>+  NS_ENSURE_ARG_POINTER(aResult);

Please don't.  Callers just shouldn't do that.

>+    if (scriptInit.mHasActiveTabPermission) {
>+      aRv.Throw(NS_ERROR_INVALID_ARG);

I'm not sure I follow.  At the very least, this needs a comment explaining why.

Comment 42

[Andrew Swan [:aswan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #41)
> > Bug 1396399 Clarify rules for applying activeTab permission to content scripts
> 
> This needs a much better commit message, unless that would expose the
> security issue.

That (exposing the issue) is what I was concerned about.  I'm happy to expand on the commit message, but could use some feedback on how much is okay (ie from security).
Regardless, sorry for the confusion :/

> >+  , mHasNavigatedFromInitialPage(false)
> 
> I don't understand the desired semantics of mHasNavigatedFromInitialPage.
> 
> If I do window.open(url), should that really have different semantics from:
> 
>   var w = window.open();
>   setTimeout(function() { w.location.href = url }, 100);
> 
> ?  It looks to me like the former would have mHasNavigatedFromInitialPage
> false while the latter has it true.

Yes, that is desired for this particular scenario.  This case may be rather narrow but consider a page that has a blank iframe and an extension that has the activeTab permission (but no specific host permissions).  The extension can navigate the frame to some new URL, then inject a content script with allFrames: true and now they've managed to inject a script at a site that they should not have access to.  If we could distinguish when the extension is the one navigating the frame perhaps we could use that, but I don't think we actually can make that distinction in a foolproof way.
(If the above has too much WebExtensions jargon please ask for clarification, I'd be glad to clear it up here or on IRC or whatever)

Comment 43

[Boris Zbarsky [:bzbarsky]]

> I'm happy to expand on the commit message, but could use some feedback on how much is okay (ie from security).

Can we start with a writeup in this bug of what the commit message would be if we ignore security considerations?

So if I understand correctly, if an extension has activeTab permissions we _do_ want to allow it to interact with cross-origin iframes the page itself loads, but not ones the extension loads?

Can the extension not inject the blank iframe itself?  Can it not inject an iframe with src already set?

Comment 44

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #43)
> So if I understand correctly, if an extension has activeTab permissions we
> _do_ want to allow it to interact with cross-origin iframes the page itself
> loads, but not ones the extension loads?

Correct, but only frames created by the network parser. It's too easy for the extensions to create frames from the context of content JS.

> Can the extension not inject the blank iframe itself?  Can it not inject an
> iframe with src already set?

It can, but we're only allowing access to frames that were network-created.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  So the key part is that we're restricting this to <iframe> elements that have an src when they come out of the HTML parser, _and_ excluding cases when that parser is document.write() or innerHTML or whatever.

What happens if the extension injects something like:

  <iframe srcdoc="<iframe src=whatever></iframe>"></iframe>

Does that inner iframe get treated as "created by the network parser"?  If so, how is it handled by this setup?

Comment 46

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #45)
> What happens if the extension injects something like:
> 
>   <iframe srcdoc="<iframe src=whatever></iframe>"></iframe>
> 
> Does that inner iframe get treated as "created by the network parser"?  If
> so, how is it handled by this setup?

That's a good question. I suspect that it does, but I'm not sure. We should test it.

I think we can solve that problem by checking the activeTab permission for parent frames when we do the matching.

Comment 47

[Boris Zbarsky [:bzbarsky]]

I assume we can't easily leverage the "use the extension as the triggering principal for loads it starts" work, because that doesn't actually happen in all the cases we care about?

(Plus of course it doesn't handle confused-deputy issues...)

Comment 48

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #47)
> I assume we can't easily leverage the "use the extension as the triggering
> principal for loads it starts" work, because that doesn't actually happen in
> all the cases we care about?
> 
> (Plus of course it doesn't handle confused-deputy issues...)

At this point, it's more that the extension can always do things using the page principal if it wants, using wrappedJSObject, eval, or injecting script nodes. I don't think there's really any reasonable way we can prevent that. I wish we could, though...

Comment 49

[Andrew Swan [:aswan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #45)
> What happens if the extension injects something like:
> 
>   <iframe srcdoc="<iframe src=whatever></iframe>"></iframe>
> 
> Does that inner iframe get treated as "created by the network parser"?  If
> so, how is it handled by this setup?

I tried this out and in my test, we never get around to checking hasNavigated.. since Matches() returns false here:
https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/components/extensions/WebExtensionPolicy.cpp#338-340

This is over my head, I defer to kmag and bz to evaluate whether this is sufficient.

Comment 50

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #49)
> I tried this out and in my test, we never get around to checking
> hasNavigated.. since Matches() returns false here:
> https://searchfox.org/mozilla-central/rev/
> 9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/components/extensions/
> WebExtensionPolicy.cpp#338-340

That will return false for the srcdoc iframe, but not for the iframe loaded from the parsed srcdoc attribute. Which may not actually exist in bz's actual example, depending on how the HTML parser deals with the invalid syntax from a > being present in an attribute value, but, e.g.:

  <iframe srcdoc="<iframe src='http://example.com/'&gt;</iframe&gt>"></iframe>

for instance, should still wind up loading a content script into the example.com iframe.

Comment 51

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #50)
> (In reply to Andrew Swan [:aswan] from comment #49)
> > I tried this out and in my test, we never get around to checking
> > hasNavigated.. since Matches() returns false here:
> > https://searchfox.org/mozilla-central/rev/
> > 9bab9dc5a9472e3c163ab279847d2249322c206e/toolkit/components/extensions/
> > WebExtensionPolicy.cpp#338-340
> 
> That will return false for the srcdoc iframe, but not for the iframe loaded
> from the parsed srcdoc attribute. Which may not actually exist in bz's
> actual example, depending on how the HTML parser deals with the invalid
> syntax from a > being present in an attribute value, but, e.g.:
> 
>   <iframe srcdoc="<iframe
> src='http://example.com/'&gt;</iframe&gt>"></iframe>
> 
> for instance, should still wind up loading a content script into the
> example.com iframe.

Yes, I fixed that in my test.  What I actually ended up with in the top-level doc is (as copied from devtools Inspector):
<iframe style="display: block;" srcdoc="<iframe src=&quot;http://example.com/&quot;></iframe>"></iframe>

The inner iframe didn't actually load anything from example.com though...

Comment 52

[Kris Maglione [:kmag]]

`>` ends an element declaration, even when it appears in an attribute value. At least in theory. But it looks like our tag soup parser currently accepts it, and:

  document.body.innerHTML=`<iframe srcdoc="<iframe src='http://example.com/'></iframe>"></iframe>`

does appear to actually create an iframe with an example.com iframe loaded into it.

Comment 53

[Andrew Swan [:aswan]]

Bleah, never mind comment 49, the inner frame was blocked by the CSP of the outer frame in my test.
With that removed, the script is indeed injected into the inner frame.

Comment 54

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

Addressed the srcdoc case that :bz raised, also addressed his other
review comments.  I haven't updated the commit summary or description,
I think recent comments on the bug have cleared up any oustanding
questions for people who have access to the bug.  If more detail in
the commit message is wanted, I'm happy to revise it.
Additional test case to come shortly.

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 55

[Boris Zbarsky [:bzbarsky]]

So I'm still a bit confused by the semantics of mHasNavigatedFromInitialPage.  If the page has:

  <iframe></iframe>

(no src attribute) and then the extension navigates the iframe to a url of its choosing, what will mHasNavigatedFromInitialPage be set to?  Will the extension get access to the resulting page?

Comment 56

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #55)
> So I'm still a bit confused by the semantics of
> mHasNavigatedFromInitialPage.  If the page has:
> 
>   <iframe></iframe>
> 
> (no src attribute) and then the extension navigates the iframe to a url of
> its choosing, what will mHasNavigatedFromInitialPage be set to?  Will the
> extension get access to the resulting page?

It shouldn't, but we should make sure that's tested.

Comment 57

[Andrew Swan [:aswan]]

Yep, that case isn't handled correctly.
Perhaps mHasNavigatedFromInitialPage is poorly named but the desired semantics are something like "false if this page/frame is what was originally loaded from the network, true if it is possible that a script has caused this page/frame to load something from a different source".
My first pass at this involved monkeying around with nsGenericHTMLFrameElement and that got kind of messy but it seems like this particular case isn't going to be detectable from the docshell and we'll have to go back to the frame element and track whether any of the frame's attributes have been changed?  Any suggestions for simpler alternatives would be gratefully welcomed...

Comment 58

[Boris Zbarsky [:bzbarsky]]

I agree that it's not clear how to detect this stuff from the docshell right now.

So one option is that the first time nsGenericHTMLFrameElement::LoadSrc reaches the mFrameLoader->LoadFrame() line for a given element that came from network, we indicate that to nsFrameLoader::LoadFrame somehow.  This will, in practice, mean the load when we get inserted into the document.

I agree that threading this information through LoadFrame, LoadURI, frameloader initialization, etc, is a pain.  I'm not seeing an obvious better option, though...

Comment 59

[Al Billings [:abillings - ex-MoCo]]

(In reply to Andrew Swan [:aswan] from comment #39)
 
> And abillings: should the patch and the tests be checked in on 11/28 or
> should the tests be held for some time?

No tests. No bug that needs sec-approval should ever land with tests. If it is nightly and beta only, we can check in tests right after it lands on both. Otherwise, tests need to wait until about a month after public release.

Comment 60

[Daniel Veditz [:dveditz]]

(In reply to Andrew Swan [:aswan] from comment #20)
> (In reply to Paul Theriault [:pauljt] from comment #19)
> >  a) the top-level window of the current tab
> >  b) Any iframes which are declared in the HTML of the document
> > 
> > I was expecting just (a), but if you would prefer (a)& (b) for compat
> > reasons, that seems to make sense too. My goal here is to ensure that
> > activeTab is significantly less privileged than <all_urls>.
> 
> Right a&b is what Chrome does and is what is proposed here.

In theory a&b matches a user's expectation and Chrome compat is important, so I'm OK with it in theory. But implementing (b) raises all kinds of edge cases (some of which bz notes in comments above) that I worry in practice we won't be plugging the hole.

Comment 61

[Daniel Veditz [:dveditz]]

(In reply to Ronen Zilberman from comment #11)
> "sec-high" allows "Obtaining confidential data from other sites the user is
> visiting, or inject data or code into those sites, requiring no more than normal
> browsing actions." (assuming installing an add-on is normal browsing actions).

"normal browsing" means with an unmodified browser; an attack like that can affect all our users. Installing an extension of any kind is already granting extra power that would be considered a security hole if a web page suddenly started doing it.

Comment 62

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8929679 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

Clearing review request until comment 55 is addressed.

Comment 63

[Ronen Zilberman]

I haven't followed this thread for a while, so I wanted to chip in.
If I understand your proposed solution correctly, the idea is that with ActiveTab an extension can interact with iframes that were loaded by the original page, but not with iframes that were loaded by the extension itself.
I'd like to point out that many pages might have "high-value" iframes originally: Facebook Like buttons for example, are implemented as an iframe pointing to facebook.com. This means that if a page has a Like button, an extension with "ActiveTab" can steal that user's cookies and therefor identity. Google ads might be the same case.

Comment 64

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

Here's an update based on comment 58.  But you lost me with the bit about
connecting this logic to nsFrameLoader.  If there's something I'm missing
here can you help me connect the dots?

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8933408 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

I _think_ there's a hole here, but please correct me if I'm wrong.

Say the page has <iframe src="foo">.  We go through nsGenericHTMLFrameElement, set mLoadCount to 1.

Then a navigation is performed by setting frame.contentWindow.location.href.  The docshell is not created dynamically, nsContentUtils::IsNavigatedFrame returns false, right?  So we're going to end up returning true from ShouldMatchActiveTabPermission(), but the desired behavior is to return false, I would think.  We should make sure to add a corresponding test.  And tests for all the other edge cases we've considered in this bug so far...

That is, we need to keep track of whether the thing we're looking at was in fact loaded from the src attribute of the element, not just of how many loads were done from that src attribute.

Comment 66

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 67

[Andrew Swan [:aswan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #65)
> I _think_ there's a hole here, but please correct me if I'm wrong.

Nope, you're right, your original comment finally clicked for me with this example.  This is going to be tedious but I (think I) understand what's needed now...

Comment 68

[Andrew Swan [:aswan]]

Attachment: wip

Here's a snapshot of the work in progress, I've rolled the tests into the
patch just to (hopefully) make it easier to look at for now.  I've done
the rough work from comment 58 but once we get into docshell I quickly
got lost.  In the current patch I just remember the value of the incoming
flag in nsDocShell::LoadURI() but that's clearly too early, if the load
fails or if it ends up occurring in a different shell, we won't do the
right thing.  But I am unsure how to handle this and could use some advice...

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8934269 [details] [diff] [review]
wip

I'm really sorry about the lag.  I was trying to figure out what we're really trying to accomplish here.

So my gut feeling is that trying to store this in members on docshell/frameloader/etc is going to be very fragile.

It seems to me that conceptually we should do something like this:

1)  A frame element <iframe> or <frame> keeps track of whether the load it's doing is for the initial value of the attribute for a parser-created element.  More on this below.

2)  We propagate this information far enough down the callstack to stash it in the LoadInfo of the channel.

3)  When we need to find this information for a given inner window or document, we ask the document channel.

This would handle cases like the page having <iframe></iframe> with no src and the extension setting src later, for example....

Note that this sort of needs to operate on inner windows, not outer ones.  I'm not sure why the existing checks are done on outer windows or docshells....

Does that make any sense?

Comment 70

[Andrew Swan [:aswan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #69)
> I'm really sorry about the lag.  I was trying to figure out what we're
> really trying to accomplish here.

Not to worry, thanks for being patient with the elementary questions and multiple iterations, I'm slowly absorbing everything here...

> It seems to me that conceptually we should do something like this:
> 
> 1)  A frame element <iframe> or <frame> keeps track of whether the load it's
> doing is for the initial value of the attribute for a parser-created
> element.  More on this below.

To be clear, is the way this information is gathered in the existing patch in nsFrameLoader adequate?

> 2)  We propagate this information far enough down the callstack to stash it
> in the LoadInfo of the channel.

And do I understand correctly that this is again mostly what the existing patch does but enabling the new INTERNAL_LOAD flag, then copying this into loadInfo inside nsDocShell::DoURILoad() ?

> Note that this sort of needs to operate on inner windows, not outer ones. 
> I'm not sure why the existing checks are done on outer windows or
> docshells....

The code path in question here begins with an extension doing something like `browser.tabs.executeScript({tabid, {...}})` where tabid just identifies a tab (which corresponds to an outer window right?).  At some point we need to map this to the specific document currently loaded in that tab.  I'm having a hard time grokking all the different components here, is there a difference between accessing `outerWin.GetDoc()` and `outerWin.GetCurrentInnerWindow().GetDoc()`? (I know the syntax there is wrong but you get the idea)

> Does that make any sense?

I think so, assuming the answer to the questions above isn't "no you're wildly off the mark"

Comment 71

[Boris Zbarsky [:bzbarsky]]

> To be clear, is the way this information is gathered in the existing patch in nsFrameLoader adequate?

Hmm.  I guess we have to use a member there because the load goes async before it needs the "is this the original src?" info, right.

I read over that part just now, and it looks fine.  I wish it were simpler, but I don't see how to do that offhand without major surgery of some sort.

> And do I understand correctly that this is again mostly what the existing patch does but enabling the new
> INTERNAL_LOAD flag, then copying this into loadInfo inside nsDocShell::DoURILoad() ?

That seems plausible, yes.  I'd like to avoid the mFrameAtOriginalSrc member on docshell.

> where tabid just identifies a tab (which corresponds to an outer window right?)

Yes.

> At some point we need to map this to the specific document currently loaded in that tab.

That makes sense.

> is there a difference between accessing `outerWin.GetDoc()` and `outerWin.GetCurrentInnerWindow().GetDoc()`?

There is not.  I guess my point was that we want to store this "I am the result of this sort of load" information on the document (and then get it from the window's current document), not the docshell.

Comment 72

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

I think this reflects everything we discussed previously.  It touches
what seems to me like an unreasonably large number of files/lines, but
if there's a simpler way to do this I'm not seeing it :/

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 73

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8939412 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

>+  loadInfo->SetFrameOriginalSrc(aOriginalFrameSrc);

SetOriginalFrameSrc, please, for consistency.

>+  const long INTERNAL_LOAD_FLAGS_FRAME_ORIGINAL_SRC      = 0x80;

INTERNAL_LOAD_FLAGS_ORIGINAL_FRAME_SRC, please, for consistency.

>+++ b/dom/base/nsFrameLoader.h
>+  // True if a pending load corresponds to the original src (or srcdoc)

We should probably reset mLoadingOriginalSrc to false after we set it on the loadinfo in nsFrameLoader::ReallyStartLoadingInternal.

>+  uint32_t mLoadCount;

Let's make this a boolean mSrcLoadHappened or something?  Then we don't have to worry about overflow.

>+++ b/netwerk/base/LoadInfo.h
>+  bool                             mFrameOriginalSrc;

mOriginalFrameSrcLoad, maybe, for consistency?

>+++ b/netwerk/base/nsILoadInfo.idl
>+  [infallible] attribute boolean frameOriginalSrc;

originalFrameSrcLoad, please.  Or at least originalFrameSrc, like other places.

>+++ b/toolkit/components/extensions/WebExtensionPolicy.cpp
>+  if (AddonManagerWebAPI::IsValidSite(aDoc.PrincipalURL().URI())) {
>+    return false;

This needs documentation.  Why is that check there?  Presumably because we don't want to allow addons to mess with those sites?

>+    return true;
>+  } else {

else after return...

>+  nsCOMPtr<nsILoadInfo> loadInfo;
>+  channel->GetLoadInfo(getter_AddRefs(loadInfo));

  nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();

r=me with the nits fixed.  Thank you for doing this.

And I agree that the way we had to propagate this boolean is moderately nuts.  We should really simplify the loading codepaths and deCOM docshell loadinfo...

Comment 75

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8939413 [details] [diff] [review]
Test for content scripts and activeTab permission

>+    let [tab] = await Promise.all([
>+      browser.tabs.create({url: URL}),
>+      awaitLoad({frameId: 0}),
>+    ]);

Is it really ok to race those against each other?

>+// activeTab perrmission

"permission".

>+    function awaitLoad(filter) {

There's a lot of code duplication here with the first test...  Any way to share that code more?

>+                              "XXX");

What does that mean?

>+                              "In original page, script is injected into base page and original frame");

Why is it not injected into emptyframe?

There are some edge cases that are not really tested here:

1)  <iframe> coming from innerHTML (should not get injected into).
2)  <iframe> coming from an XHR responseXML (so from a network parser) and then getting adopted into the web page document.
3)  innerHTML injecting an <iframe srcdoc> containing another iframe, as in comment 45.
4)  window.open of a new window, which should get injected into, presumably.

Comment 76

[Andrew Swan [:aswan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #75)
> Comment on attachment 8939413 [details] [diff] [review]
> Test for content scripts and activeTab permission
> 
> >+    let [tab] = await Promise.all([
> >+      browser.tabs.create({url: URL}),
> >+      awaitLoad({frameId: 0}),
> >+    ]);
> 
> Is it really ok to race those against each other?

Maybe I'm misunderstanding the comment, are you concerned that the new tab can get created and loaded before we call awaitLoad()?  There are many asynchronous things that happen between entering browser.tabs.create() and the corresponding onCompleted event firing, they would all have to go away for that to become an issue, which I think it is safe to say will not happen.

> >+    function awaitLoad(filter) {
> 
> There's a lot of code duplication here with the first test...  Any way to
> share that code more?

I've tried with the latest revision.  Its not completely straightforward since a bunch of these functions get serialized into strings in extensions (but we write the code this way so we can lint the functions instead of putting the code into big string constants).

> >+                              "XXX");
> 
> What does that mean?

Oops, it means I meant to fill this in but I overlooked it before pushing.  Sorry about that, I'll fix it.

> >+                              "In original page, script is injected into base page and original frame");
> 
> Why is it not injected into emptyframe?

Because in this case we're not passing the (somewhat misleadingly named) `matchAboutBlank` option to executeScript().  But I've done a bunch of refactoring of the tests and am now always passing that flag (and then checking empty frames as appropriate)

Comment 77

[Andrew Swan [:aswan]]

Attachment: Clarify rules for applying activeTab permission to content scripts

MozReview-Commit-ID: 9xPDX8Qk2iR

Comment 78

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 79

[Andrew Swan [:aswan]]

Comment on attachment 8941328 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

(carried over r+ from :bz from comment 74)

Earlier sec-approval request was in comment 37, the answers there still apply.  Sorry for jumping the gun on the earlier request before getting the patch through review...

Comment 80

[Andrew Swan [:aswan]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #75)
> 4)  window.open of a new window, which should get injected into, presumably.

Whoops I realized I didn't cover this in the latest test patch.
However, we don't currently propagate activeTab to children created in this way, and my gut reaction is that we shouldn't do so.  Maybe we could take this question to a follow-up bug?

Comment 81

[Boris Zbarsky [:bzbarsky]]

> are you concerned that the new tab can get created and loaded before we call awaitLoad()

I'm concerned that it can get created and loaded before awaitLoad sets up enough of its state to be awaiting the load.  Or is the idea that once awaitLoad has returned we are all set for the listeners it needs to add?  If so this might be ok, though I would be happier if the awaitLoad() call happened _before_ the create() call.

> Maybe we could take this question to a follow-up bug?

That seems fine.

Comment 82

[Boris Zbarsky [:bzbarsky]]

> That seems fine.

Actually, we should probably just test that we do _not_ inject in that case, because an extension can do the window.open.  Followup bug is OK for the test, though.

Comment 83

[Al Billings [:abillings - ex-MoCo]]

I'd like to give this sec-approval+ and then beta+ to make the last beta but I need Gerry's approval to do so.

Andrew, please nominate the patch for beta if it applies.

Gerry, I would like to get this into the last beta.

Comment 84

[Andrew Swan [:aswan]]

The patch applies cleanly to beta, I haven't actually tried to build and test it yet (I can do so later today)
Which raises another question though, what's the policy about doing try runs with these patches?  This patch is big enough that I would ordinarily test it on try but then the patch would be out in public.  I can live without the new tests on try (the tests are commented enough that having them become public would really reveal how to exploit the bug) but what's okay as far as just ensuring that the main patch doesn't break something else?

Comment 85

[Gerry Chang [:gchang]]

Al, ok to get this in beta 16.

Comment 86

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8941328 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

Thanks!

sec-approval+ for trunk.
I'll approve the beta? when it shows up.

Comment 87

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/ff6e9d3a6441aa4510ed47d5383e79fe5a412576

Comment 88

[Al Billings [:abillings - ex-MoCo]]

(In reply to Andrew Swan [:aswan] from comment #84)
> The patch applies cleanly to beta, I haven't actually tried to build and
> test it yet (I can do so later today)
> Which raises another question though, what's the policy about doing try runs
> with these patches?  This patch is big enough that I would ordinarily test
> it on try but then the patch would be out in public.  I can live without the
> new tests on try (the tests are commented enough that having them become
> public would really reveal how to exploit the bug) but what's okay as far as
> just ensuring that the main patch doesn't break something else?

I would just do it on try at this point since it is checked into the integration branch.

Comment 89

[Andrew Swan [:aswan]]

(In reply to Al Billings [:abillings] from comment #88)
> I would just do it on try at this point since it is checked into the
> integration branch.

Oops, my question about try was originally meant for trunk but now the main patch has landed.  The patches apply cleanly to beta and all the new tests pass.  I haven't done a beta try run but I think if the patch sticks on autoland we should be good:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ff6e9d3a6441aa4510ed47d5383e79fe5a412576

I'm going to request beta approval now since I'll be away from the computer for a while this evening but if the approval is granted quickly can we wait to see that the above run is green before landing there (:RyanVM I assume you are the one who would be doing the landing)

Comment 90

[Andrew Swan [:aswan]]

Comment on attachment 8941328 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

Approval Request Comment
[Feature/Bug causing the regression]:
none

[User impact if declined]:
sec-moderate vulnerability described in the original bug

[Is this code covered by automated tests?]:
yes, but the tests are in a separate patch so they can be embargoed for a while

[Has the fix been verified in Nightly?]:
with automated tests, yes

[Needs manual test from QE? If yes, steps to reproduce]: 
I don't believe additional manual testing is needed

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
not especially

[Why is the change risky/not risky?]:
The patch does touch a lot of code but mostly this is plumbing to add a new flag that is only checked from the webextensions content script injection code.

[String changes made/needed]:
none

Comment 91

[Gerry Chang [:gchang]]

Comment on attachment 8941328 [details] [diff] [review]
Clarify rules for applying activeTab permission to content scripts

Take it in Beta58.

Comment 92

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e57848b337fcaeec8a8e23f45354603df11c2b5b

Comment 93

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 95

[Andrew Swan [:aswan]]

Can we also get a date for when the tests should be checked in (assuming they get r+) so that doesn't fall through the cracks?

Comment 96

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8941329 [details] [diff] [review]
Test for content scripts and activeTab permission

>+// Test that dynamically created iframes do not get the activeTab permission
>+      div2.innerHTML = "<iframe srcdoc=\"<iframe src='http://test2.example.com/'&gt;</iframe&gt;\"></iframe>";

Is there a reason we don't listen to this frame's load with frameLoaded?  Seems like we should (and nframes should be 4, not 3).

r=me with that fixed or explained.  Thank you!

Comment 97

[Al Billings [:abillings - ex-MoCo]]

(In reply to Andrew Swan [:aswan] from comment #95)
> Can we also get a date for when the tests should be checked in (assuming
> they get r+) so that doesn't fall through the cracks?

Set intestsuite? to flag them for later. It is going to be at least four weeks after a release containing the fixes, assuming normal uptake and that we don't, for example, turn off updates for a week to chemspill. In other words, I can't give a specific date at this time. Probably the end of February.

Comment 98

[Andrew Swan [:aswan]]

Attachment: Test for content scripts and activeTab permission

MozReview-Commit-ID: DyVtSuA9voI

Comment 99

[Andrew Swan [:aswan]]

Comment on attachment 8944564 [details] [diff] [review]
Test for content scripts and activeTab permission

The item from comment 96 is addressed, carrying over the r+ from that comment.

Comment 100

[Kris Maglione [:kmag]]

Attachment: Bug 1396399: Test for content scripts and activeTab permission.

Comment 101

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/309b64bd96a1
Test for content scripts and activeTab permission.

Comment 102

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 309b64bd96a1 (bug 1396399) for mochitest failures at test_ext_contentscript_activeTab.html and ESlint failure at SpecialPowersParent.js
https://hg.mozilla.org/integration/autoland/rev/9a09309f554a33bd33996f23f6a9e6a58932fd9a

Failure log ESlint:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=309b64bd96a1763a8bb84bfe2c9338749254a978&selectedTaskRun=cPodKgeyRgi5zeLXlrIORQ.0

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315797300&repo=autoland&lineNumber=403

mochitest
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=YVje81DST2iNfv_erEG2QA.0&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=309b64bd96a1763a8bb84bfe2c9338749254a978

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315797897&repo=autoland&lineNumber=3748

Comment 103

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09f6522b2f48
Test for content scripts and activeTab permission.

Comment 104

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=Cr4AL6LUTqWFZqz3GshyKA.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=09f6522b2f48a938aa575e43306c2b0e1af5aa07&failure_classification_id=2

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315803125&repo=autoland&lineNumber=5334

Backout: https://hg.mozilla.org/integration/autoland/rev/bc6413221c76ad99e57f80174b79c9c6a90baf76

Comment 105

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/150632dfdf3a
Test for content scripts and activeTab permission.

Comment 106

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/150632dfdf3a