Closed Bug 1588956 Opened 5 years ago Closed 5 years ago

apply addon csp to relative urls

Categories

(WebExtensions :: General, enhancement, P1)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

If a content script sets img.src to a full url, the addon csp is used. If it sets a relative url, it is not. This was choosen in bug 1532207, yet even earlier in bug 1406278.

The result is that the addon will violate csp only if using a full url even if the relative url resolves to the same full url otherwise.

[1] https://phabricator.services.mozilla.com/D22121

Flags: needinfo?(ckerschb)

also ni? bz as he requested the ordering in the patch.

Flags: needinfo?(bzbarsky)

If it sets a relative url, it is not

But the page CSP is used in that situation, right?

The ordering in bug 1406278 was premised on the fact that we were going to NOT apply the page's CSP (nor an extension CSP, because we had no extension CSP at that point), and that makes sense for URLs the page does not control but the addon does. The issue with relative URLs is that neither the addon nor the page has full control over the URL.

So what behavior do we want for the relative URL case when we're triggering the load from addon code? Some possible options:

  1. Apply no CSP at all. Probably not a great idea.
  2. Apply the page CSP but not the addon CSP. This was the original approach in bug 1406278, effectively, but back then we had no addon CSP to apply anyway. This allows addon CSP bypasses. What are the goals of addon CSP, exactly? Link to resource on that would be appreciated.
  3. Apply the addon CSP but not the page CSP. This allows inadvertent page CSP bypasses.
  4. Apply both the addon CSP and the page CSP.
  5. Disallow the load unconditionally.

Note that this applies to all callsites of nsContentUtils::GetAttrTriggeringPrincipal, not just img src.

Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

If it sets a relative url, it is not

But the page CSP is used in that situation, right?

yes, however, to my understanding, the page CSP should never affect the content script, even in the current manifest V2. My read of the initial code that Kris did was as a performance shortcut, but I haven't asked him for his thinking around that.

The issue with relative URLs is that neither the addon nor the page has full control over the URL.

It is an odd duck. Right now page csp controls this, and that kind of makes sense, but it's a content script causing the action. In the new world where we do have a content script specific csp, that's what we're leaning towards. If the content script triggers something by touching the page, the page csp is not involved, but the add csp is. The default is relatively strict, and can be relaxed by defining a csp in the manifest (which makes it more reviewable).

So what behavior do we want for the relative URL case when we're triggering the load from addon code? Some possible options:

  1. Apply no CSP at all. Probably not a great idea.
  2. Apply the page CSP but not the addon CSP. This was the original approach in bug 1406278, effectively, but back then we had no addon CSP to apply anyway. This allows addon CSP bypasses. What are the goals of addon CSP, exactly? Link to resource on that would be appreciated.
  3. Apply the addon CSP but not the page CSP. This allows inadvertent page CSP bypasses.
  4. Apply both the addon CSP and the page CSP.
  5. Disallow the load unconditionally.

Note that this applies to all callsites of nsContentUtils::GetAttrTriggeringPrincipal, not just img src.

What are the goals of addon CSP, exactly? Link to resource on that would be appreciated.

The current goal, until there is more clarity around what google is doing (though I can't see how it could work in any vastly different way), is to treat this basically like we do the existing setting (but targeting content scripts). A pages CSP should play no part in what the content script does. If an addon content script does something, it should be restricted by the addon content script csp. Right now, for the most part, no csp is applied to content scripts. Relative urls seem to be an exception to that.

So, #3 sounds like the the match, but I'm not sure what you think "inadvertent" bypasses are.

the page CSP should never affect the content script

OK.

In the new world where we do have a content script specific csp, that's what we're leaning towards.

OK, then you should probably just change GetAttrTriggeringPrincipal as needed.

The default is relatively strict, and can be relaxed by defining a csp in the manifest (which makes it more reviewable)

This is the part that I was trying to figure out: what is the source and purpose of the add-on CSP? If the point is that we impose a pretty strict one by default, then the add-on can explicitly try to loosen it, so it's not self-imposed restrictions but rather us-imposed ones, then it's relatively more important that we apply it.

but I'm not sure what you think "inadvertent" bypasses are

Cases when the script sets a URL-valued attribute to something and doesn't realize that the resulting URL is not what it expected, because that resulting URL can be controlled by the page or things injected into the page.

Put another way, are there use cases for allowing relative-URL loads from content scripts at all?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

but I'm not sure what you think "inadvertent" bypasses are

Cases when the script sets a URL-valued attribute to something and doesn't realize that the resulting URL is not what it expected, because that resulting URL can be controlled by the page or things injected into the page.

Put another way, are there use cases for allowing relative-URL loads from content scripts at all?

Hrm, that's a good question and I don't know. We apparently prevent relative urls in xhr/fetch in content scripts. I'm not certain how much effort it would be to catch all the potential locations we'd need to handle that, so I'd lean first to enforcing the addon csp regardless, then if we think it's a good idea to prevent relative urls, knock those off in another bug. I don't have a strong inclination either way.

That seems fine to me. Thank you for talking that through!

Assignee: nobody → mixedpuppy
Type: task → enhancement
Priority: -- → P1
Summary: determine correct csp handling of relative url → apply addon csp to relative urls
Blocks: 1589171

We can't ignore the page's CSP or use an extension's triggering principal if the page has a role in choosing what's going to be loaded. That opens a gaping hole for privilege escalation. It's a complete non-starter.

Just to play devil's advocate for a sec, if the addon does:

img.src = img.src;

where img comes from a web page, that will ignore the page's CSP (because it will be a relative URL)... I'm not sure I have a good way of dealing with that, short of implementing some sort of string tainting...

(In reply to Kris Maglione [:kmag] from comment #7)

We can't ignore the page's CSP or use an extension's triggering principal if the page has a role in choosing what's going to be loaded. That opens a gaping hole for privilege escalation. It's a complete non-starter.

I was just writing about that...due to re-reading bug 1406278 comment 17, and got collision.

Extensions can use an absolute url to get around the page csp here, so I'm going to wontfix. We may still want to consider whether we want to do anything about bug 1589171.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

Just to play devil's advocate for a sec, if the addon does:

img.src = img.src;

where img comes from a web page, that will ignore the page's CSP (because it will be a relative URL)... I'm not sure I have a good way of dealing with that, short of implementing some sort of string tainting...

Yes, that is not ideal, but it is at least obviously wrong. The security implications of relying on relative URLs are less obvious, though, and harder to avoid.

(In reply to Kris Maglione [:kmag] from comment #7)

We can't ignore the page's CSP or use an extension's triggering principal if the page has a role in choosing what's going to be loaded. That opens a gaping hole for privilege escalation. It's a complete non-starter.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

Just to play devil's advocate for a sec, if the addon does:

img.src = img.src;

where img comes from a web page, that will ignore the page's CSP (because it will be a relative URL)... I'm not sure I have a good way of dealing with that, short of implementing some sort of string tainting...

These problems are not unique to relative URLs. The same can be said of absolute URLs, and I expect a consistent set of rules to apply to both of them.

In a world where content scripts don't have any CSP, I understand the decision to enforce the page's CSP for relative URLs.
Now, content scripts are no longer left without CSP, and it can be confusing when relative URLs can be used to bypass the content script's CSP (see point 2 below for an example).

bz listed the relevant options in comment 2, which are as follows (I omitted 1, because we should obviously not ignore all CSP):

  1. Apply the page CSP but not the addon CSP. This was the original approach in bug 1406278, effectively, but back then we had no addon CSP to apply anyway. This allows addon CSP bypasses. What are the goals of addon CSP, exactly? Link to resource on that would be appreciated.

This is the current situation, and allows addon CSP bypass.
If an extension declares default-src 'none', and we enforce addon CSP, then I expect img.src = url to always be blocked. However, if we ignore the content script CSP (like we do right now, option 2) and enforce the page's CSP (which can be missing / very permissive), then a content script could set the base URL and assign a relative URL to img.src and bypass the content script's CSP.

The reason for developing CSP for content script is to have the ability to restrict code execution from content scripts. This reduces the impact of XSS vulnerabilities in content scripts, and also makes it easier for add-on reviewers to audit the behavior of an extension's content scripts.

  1. Apply the addon CSP but not the page CSP. This allows inadvertent page CSP bypasses.

This looks like the most desirable option for extension developers.

Chromium does currently not enforce a CSP in content scripts, but Chrome extensions are currently allowed to fully bypass the page's CSP.
A reason for this is to allow extensions to overrule the page's CSP on behalf of the user, another reason is to reduce noise in CSP violation reports.

  1. Apply both the addon CSP and the page CSP.

This looks like the most "secure" option. If option 3 is a non-starter, then I think that we should default to 4, with an opt-in for option 3.

  1. Disallow the load unconditionally.

This is proposed in bug 1589171 , but I don't think that it's a good solution, because relative URLs are a fundamental part of the web.
If an extension uses relative URLs, then it is reasonable to assume that the extension wanted to load a URL that is resolved relative to the document's base URI (often same-origin, sometimes different).

Regardless of the chosen option, I believe that we should apply the (same) approach to relative and absolute URLs.

(In reply to Rob Wu [:robwu] from comment #12)

(In reply to Kris Maglione [:kmag] from comment #7)

We can't ignore the page's CSP or use an extension's triggering principal if the page has a role in choosing what's going to be loaded. That opens a gaping hole for privilege escalation. It's a complete non-starter.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

Just to play devil's advocate for a sec, if the addon does:

img.src = img.src;

where img comes from a web page, that will ignore the page's CSP (because it will be a relative URL)... I'm not sure I have a good way of dealing with that, short of implementing some sort of string tainting...

These problems are not unique to relative URLs. The same can be said of absolute URLs, and I expect a consistent set of rules to apply to both of them.

When it is the web, this is handled consistently. We just may not be able to be consistent in the same way in an addons content script.

In a world where content scripts don't have any CSP, I understand the decision to enforce the page's CSP for relative URLs.
Now, content scripts are no longer left without CSP, and it can be confusing when relative URLs can be used to bypass the content script's CSP (see point 2 below for an example).

This is proposed in bug 1589171 , but I don't think that it's a good solution, because relative URLs are a fundamental part of the web.
If an extension uses relative URLs, then it is reasonable to assume that the extension wanted to load a URL that is resolved relative to the document's base URI (often same-origin, sometimes different).

Is there something that can be done with relative urls that cannot be done with full urls? Extensions can easily build the full url that they need to bypass this one instance. Preventing the relative url in this case makes it more obvious that it wont work, rather than it surprising us when it works in non-obvious ways.

Regardless of the chosen option, I believe that we should apply the (same) approach to relative and absolute URLs.

That would be ideal and is what I was shooting for, and would also prefer...if you've got a good idea on how to address the relative url issue that avoids the problems.

How about we resolve relative urls set by an addon to the addon's base url rather than the pages? That or blocking is about the best low-cost alternatives I have. I dislike leaving it as-is.

Flags: needinfo?(ckerschb)
You need to log in before you can comment on or make changes to this bug.