Closed Bug 1088729 Opened 6 years ago Closed 6 years ago

Only allow http(s) directory links and https/data images

Categories

(Firefox :: New Tab Page, defect)

33 Branch
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.3
Tracking Status
firefox33 + verified
firefox34 + verified
firefox35 + verified
firefox36 --- verified

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: sec-moderate)

Attachments

(2 files, 4 obsolete files)

The client should filter out urls that aren't http/s.

about:config browser.newtabpage.directory.source:

data:text/plain,{"en-US":[{"url":"javascript:alert(5)"}]}
Flags: firefox-backlog+
[Tracking Requested - why for this release]: If the directory link source is hijacked (during transmission somehow?), we want to make sure links aren't javascript:
+sylvestre for tracking/uplift notice.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8511119 - Flags: review?(adw)
Comment on attachment 8511119 [details] [diff] [review]
v1

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

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +383,5 @@
>  
>        // all directory links have a frecency of DIRECTORY_FRECENCY
> +      return rawLinks.filter(link => {
> +        // Only allow urls that start with http(s)
> +        return link.url.slice(0, 4) == "http";

We've been using string.startsWith() instead of things like this for a while now, so let's use that here, too.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith
Attachment #8511119 - Flags: review?(adw) → review+
bsmedberg, we added key pinning for tiles.services.mozilla.com with bug 1030135, so the potential attack for this bug is somewhat limited. However, that server just a frontend that redirects to the appropriate cdn location.

$ curl -i https://tiles.services.mozilla.com/v2/links/fetch/en-US
HTTP/1.1 303 SEE OTHER
Location: https://dtex4kvbppovt.cloudfront.net/US/en-US.2c9903f80db84235c1dfdcb397f4bbeb3ab12dca.json

The original push for the key pinning is that we send tiles.services.mozilla.com some user data (for ping and fetch). The redirect (only happens for fetch) doesn't contain that data, but potentially opens up this type of bug.

With key pinning on tiles.services.mozilla.com and this bug fixed, is it necessary to file a bug to pin cloudfront or whatever we happen to redirect to?
I don't like the check for just "http". Why not actually check for "https://" and "http://" ?

That closes the door to attacks where a URL like "http<SOMECRAZYINJECTIONVECTORjavascript:alert(1)" works.

I'm not saying that attacks works. But doing a proper check instead of a shortcut gives me a better feeling security wise.
Attached patch v2 (obsolete) — Splinter Review
Added test for not allowing {url: "httpJUNKjavascript:42"}
Attachment #8511119 - Attachment is obsolete: true
Attachment #8511152 - Flags: review?(adw)
Comment on attachment 8511152 [details] [diff] [review]
v2

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

To be really safe, maybe we should parse these as nsIURIs and then check uri.scheme?
Attachment #8511152 - Flags: review?(adw) → review+
Attached patch v3 (obsolete) — Splinter Review
(In reply to Drew Willcoxon :adw from comment #8)
> To be really safe, maybe we should parse these as nsIURIs and then check
> uri.scheme?
Careful what you ask for in those review comments! ;) :p
Attachment #8511152 - Attachment is obsolete: true
Comment on attachment 8511180 [details] [diff] [review]
v3

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

Let's go with this then.

::: browser/modules/DirectoryLinksProvider.jsm
@@ +369,5 @@
>        this._enhancedLinks.clear();
>  
>        // all directory links have a frecency of DIRECTORY_FRECENCY
> +      return rawLinks.filter(link => {
> +        // Only allow urls that start with http(s)

With this change, "start with" is kind of a simplistic way to put it.  "URLs that are http(s)."

@@ +375,5 @@
> +        try {
> +          scheme = Services.io.newURI(link.url, null, null).scheme;
> +        }
> +        catch(ex) {}
> +        return scheme.search(/^https?$/) == 0;

Nit: scheme == "http" || scheme == "https" would be more straightforward.
Attachment #8511180 - Flags: review+
dveditz, the patches so far only address the tile link and not the image/enhanced image. Those images get loaded as css background image, e.g.,

background-image: url("javascript:alert(5)");

Do we need to filter those out as well? Or per comment 6, we could be proactive and perhaps avoid protocol issues.

For link: allow only http/https
For image: allow only http/https/data ?
Flags: needinfo?(dveditz)
Comment on attachment 8511119 [details] [diff] [review]
v1

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

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +383,5 @@
>  
>        // all directory links have a frecency of DIRECTORY_FRECENCY
> +      return rawLinks.filter(link => {
> +        // Only allow urls that start with http(s)
> +        return link.url.slice(0, 4) == "http";

I'd prefer
   return link.url.match(/^https?:/);

(or if you need to squeeze a little more perf out construct the regexp above the filter and use regexp.test(link.url))

Sure it's unlikely, but someone /might/ invent an httpfoo: scheme in the future. Since this is a privileged context you'd be able to load it even if it was marked URI_DANGEROUS_TO_LOAD and kept out of web pages.

And now off to look at the places we've been using startsWith()...
Sorry about commenting on a now-obsolete patch... asynchronicity. I have no complaints about creating an actual nsIURI. It would be absolutely necessary to do so if you were taking a blacklisting approach ("filter out javascript"), but of course I'd try to talk you into whitelisting (as you are here) instead if you were.

(In reply to Ed Lee :Mardak from comment #11)
> background-image: url("javascript:alert(5)");
> 
> Do we need to filter those out as well? Or per comment 6, we could be
> proactive and perhaps avoid protocol issues.

I'm not worried about javascript: in an image context since we filter those elsewhere, but yeah be proactive. Who knows what dumb URL-as-commands scheme some add-on will invent for internal use, that tiles could activate because it's a privileged page and not web content.

> For link: allow only http/https
> For image: allow only http/https/data ?

yup.
Flags: needinfo?(dveditz)
I'm rating this sec-moderate on the grounds that it would be a sec-critical bug if a javascript: url was injected into this context, but we've got mitigations in place (TLS transport, trusted and assumed-secure server) that hackers would have to overcome first.
Keywords: sec-moderate
(In reply to Daniel Veditz [:dveditz] from comment #13)
> > For image: allow only http/https/data ?
oyiptong/clarkbw, given that we're serving the images, I guess we could potentially be even stricter by only allowing https and data uris (and not http)?
Attached patch v4 (obsolete) — Splinter Review
adw, sorry for my quick fix patches :( This one covers links (http/s only) and images/enhanced (https/data only).
Attachment #8511180 - Attachment is obsolete: true
Attachment #8511230 - Flags: review?(adw)
Summary: Only allow http(s) directory links → Only allow http(s) directory links and https/data images
Comment on attachment 8511230 [details] [diff] [review]
v4

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

No problem!

::: browser/modules/DirectoryLinksProvider.jsm
@@ +47,5 @@
>  // The preference that tells if newtab is enhanced
>  const PREF_NEWTAB_ENHANCED = "browser.newtabpage.enhanced";
>  
> +// Only allow link urls that are http(s)
> +const ALLOWED_LINK_SCHEMES = ["http", "https"];

Sets are easier to work with in this case.

@@ +367,5 @@
>  
>    /**
> +   * Check if a url's scheme is in an array of allowed schemes
> +   */
> +  _checkScheme: function DirectoryLinksProvider__checkScheme(url, allowed) {

A better name would be urlAllowed or isURLAllowed or something like that -- something to indicate that the return value is a boolean and there are bad URLs and good URLs.

@@ +391,5 @@
> +        return this._checkScheme(link.url, ALLOWED_LINK_SCHEMES) &&
> +               (link.imageURI === undefined ||
> +                this._checkScheme(link.imageURI, ALLOWED_IMAGE_SCHEMES)) &&
> +               (link.enhancedImageURI === undefined ||
> +                this._checkScheme(link.enhancedImageURI, ALLOWED_IMAGE_SCHEMES));

Not having to check === undefined would make this a lot simpler.  In fact since _checkScheme try-catches its use of the url param, you should just be able to call _checkScheme directly without checking for undefined first.
Attachment #8511230 - Flags: review?(adw) → review+
Attached patch for checkinSplinter Review
(In reply to Drew Willcoxon :adw from comment #17)
> > +const ALLOWED_LINK_SCHEMES = ["http", "https"];
> Sets are easier to work with in this case.
Wrapped with new Set and uses .has

> > +  _checkScheme: function DirectoryLinksProvider__checkScheme(url, allowed) 
> something to indicate that the return value is a boolean and there are bad
> URLs and good URLs.
Renamed to isURLAllowed.

> > +               (link.imageURI === undefined ||
> > +                this._checkScheme(link.imageURI, ALLOWED_IMAGE_SCHEMES)) &&
> > +               (link.enhancedImageURI === undefined ||
> > +                this._checkScheme(link.enhancedImageURI, ALLOWED_IMAGE_SCHEMES));
> Not having to check === undefined would make this a lot simpler.
Thanks. Cleaned up with a short circuit true inside isURLAllowed if no url.
Attachment #8511230 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/c742dcb56135
Points: --- → 2
Target Milestone: --- → Firefox 36
Approval Request Comment
[Feature/regressing bug #]: bug 975211 bug 986521
[User impact if declined]: Downloaded malicious directory links data file could run javascript with chrome privileges on click of compromised tile
[Describe test coverage new/current, TBPL]: Added tests to only allow desired links and images https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7137cd59ac8c
[Risks and why]: Low - filtering of excessive data, doesn't change behavior of common case
[String/UUID change made/needed]: none
Attachment #8511576 - Flags: approval-mozilla-release?
Attachment #8511576 - Flags: approval-mozilla-beta?
Comment on attachment 8511285 [details] [diff] [review]
for checkin

Approval Request Comment: See comment 20 for a?beta/release.

Only difference here is jsm+test have moved on aurora35/nightly36 from their original location on beta34/release33.
Attachment #8511285 - Flags: approval-mozilla-aurora?
Attachment #8511576 - Flags: approval-mozilla-release?
Attachment #8511576 - Flags: approval-mozilla-release+
Attachment #8511576 - Flags: approval-mozilla-beta?
Attachment #8511576 - Flags: approval-mozilla-beta+
Comment on attachment 8511576 [details] [diff] [review]
for beta/release uplift

oups, not landed in m-c yet.
Attachment #8511576 - Flags: approval-mozilla-release?
Attachment #8511576 - Flags: approval-mozilla-release+
Attachment #8511576 - Flags: approval-mozilla-beta?
Attachment #8511576 - Flags: approval-mozilla-beta+
Sorry about the early a? after landing on fx-team but before m-c.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(sledru)
Resolution: --- → FIXED
Flags: needinfo?(sledru)
Attachment #8511576 - Flags: approval-mozilla-release?
Attachment #8511576 - Flags: approval-mozilla-release+
Attachment #8511576 - Flags: approval-mozilla-beta?
Attachment #8511576 - Flags: approval-mozilla-beta+
Attachment #8511285 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/1d58695aae32
> https://hg.mozilla.org/releases/mozilla-beta/rev/410afcc51b13
> https://hg.mozilla.org/releases/mozilla-release/rev/137b543a1ec4
> 
> Not landed on the 33.0 relbranch given the lack of comments here saying to
> do so. Needinfo me if it needs to.

Is it really expected that this won't make it to 33.0.2? (which is what not being on the 33.0 relbranch means, but then, why ask for -release approval if it's not to get it on the next chemspill?)
(In reply to Mike Hommey [:glandium] from comment #26)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/1d58695aae32
> > https://hg.mozilla.org/releases/mozilla-beta/rev/410afcc51b13
> > https://hg.mozilla.org/releases/mozilla-release/rev/137b543a1ec4
> > 
> > Not landed on the 33.0 relbranch given the lack of comments here saying to
> > do so. Needinfo me if it needs to.
> 
> Is it really expected that this won't make it to 33.0.2? (which is what not
> being on the 33.0 relbranch means, but then, why ask for -release approval
> if it's not to get it on the next chemspill?)
Ed asked for release approval to have it in 33.1. We are using the relbranch for 33.0.X and mozilla-release for 33.1.
Ah, the 10th anniversary release, sorry for the confusion.
Group: core-security
Iteration: --- → 36.3
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: cornel.ionce
Is there a proper method to manually verify this?
Flags: needinfo?(edilee)
You can set about:config pref browser.newtabpage.directory.source to..

data:text/plain,{"en-US":[{"title":"BAD","url":"ftp://ftp.mozilla.org","title":"BAD"}]}

You should not see a tile titled "BAD"

Similarly to test the image filtering:

data:text/plain,{"en-US":[{"title":"BAD","url":"http://ftp.mozilla.org","imageURI":"http://mozorg.cdn.mozilla.net/media/img/mozorg/mozilla-256.jpg"}]}

Again should not see "BAD"


Testing the allowed case:

data:text/plain,{"en-US":[{"title":"OK","url":"http://ftp.mozilla.org","imageURI":"https://mozorg.cdn.mozilla.net/media/img/mozorg/mozilla-256.jpg"}]}

Should see "OK" and a mozilla logo
Flags: needinfo?(edilee)
Using the above details I was able to confirm this fix on:
- Latest Nightly, build ID: 20141119030200;
- Firefox 35.0a2, build ID: 20141119144355;
- Firefox 34 beta 10, build ID: 20141117202603;
- Firefox 33.1.1, build ID: 20141113143407

Thank you Ed!
Depends on: 1105099
You need to log in before you can comment on or make changes to this bug.