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

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

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)"}]}

Comment 1

[Ed Lee :Mardak]

[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:

Comment 2

[Ed Lee :Mardak]

+sylvestre for tracking/uplift notice.

Comment 3

[Ed Lee :Mardak]

Attachment: v1

Comment 4

[Drew Willcoxon :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

Comment 5

[Ed Lee :Mardak]

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?

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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.

Comment 7

[Ed Lee :Mardak]

Attachment: v2

Added test for not allowing {url: "httpJUNKjavascript:42"}

Comment 8

[Drew Willcoxon :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?

Comment 9

[Ed Lee :Mardak]

Attachment: v3

(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

Comment 10

[Drew Willcoxon :adw]

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.

Comment 11

[Ed Lee :Mardak]

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 ?

Comment 12

[Daniel Veditz [: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()...

Comment 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Daniel Veditz [: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.

Comment 15

[Ed Lee :Mardak]

(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)?

Comment 16

[Ed Lee :Mardak]

Attachment: v4

adw, sorry for my quick fix patches :( This one covers links (http/s only) and images/enhanced (https/data only).

Comment 17

[Drew Willcoxon :adw]

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.

Comment 18

[Ed Lee :Mardak]

Attachment: for checkin

(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.

Comment 19

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/c742dcb56135

Comment 20

[Ed Lee :Mardak]

Attachment: for beta/release uplift

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

Comment 21

[Ed Lee :Mardak]

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.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8511576 [details] [diff] [review]
for beta/release uplift

oups, not landed in m-c yet.

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Ed Lee :Mardak]

Sorry about the early a? after landing on fx-team but before m-c.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Mike Hommey [:glandium]]

(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?)

Comment 27

[Sylvestre Ledru [:Sylvestre]]

(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.

Comment 28

[Mike Hommey [:glandium]]

Ah, the 10th anniversary release, sorry for the confusion.

Comment 29

[Cornel Ionce [:noni] [Hubs QA]]

Is there a proper method to manually verify this?

Comment 30

[Ed Lee :Mardak]

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

Comment 31

[Cornel Ionce [:noni] [Hubs QA]]

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!