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

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

(Depends on 1 bug, {sec-moderate})

33 Branch
Firefox 36
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox33+ verified, firefox34+ verified, firefox35+ verified, firefox36 verified)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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+
(Assignee)

Comment 1

5 years ago
[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:
(Assignee)

Comment 2

5 years ago
+sylvestre for tracking/uplift notice.
(Assignee)

Comment 3

5 years ago
Posted 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+
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Posted 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+
(Assignee)

Comment 9

5 years ago
Posted 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+
(Assignee)

Comment 11

5 years ago
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
(Assignee)

Comment 15

5 years ago
(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)?
(Assignee)

Comment 16

5 years ago
Posted 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)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 18

5 years ago
Posted 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
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/c742dcb56135
Points: --- → 2
Target Milestone: --- → Firefox 36
(Assignee)

Comment 20

5 years ago
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?
(Assignee)

Comment 21

5 years ago
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+
(Assignee)

Comment 24

5 years ago
Sorry about the early a? after landing on fx-team but before m-c.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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)
(Assignee)

Comment 30

5 years ago
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.