932335 - (CVE-2016-5282) Don't allow content to request favicons from non-whitelisted schemes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect, P1)

Description

[Richard Newman [:rnewman]]

This probably involves some browser.js hackery.

bnicholson spotted this:

http://people.mozilla.org/~bnicholson/test/favicon_fail.html

Currently that crashes the browser. After I fix that code (Bug 926430), it'll allow arbitrary fetches from our JAR.

Comment 1

[Chris Kitching [:ckitching]]

The original code in LoadFaviconTask.downloadFavicon begins like this:

        if (mFaviconUrl.startsWith("jar:jar:")) {
            return GeckoJarReader.getBitmap(sContext.getResources(), mFaviconUrl);
        }

        // only get favicons for HTTP/HTTPS
        String scheme = targetFaviconURI.getScheme();
        if (!"http".equals(scheme) && !"https".equals(scheme)) {
            return null;
        }

It, sensibly, stops loads from unexpected schemes. The problem is, as you can see, it special-cases jar:jar: schemes ahead of this check, causing this security problem.


There's not a vastly obvious solution to this in the favicon system - sometimes we *do* want to load favicons from the jar. 
Perhaps we should seperate out loading of favicons from the jar into its own type of request, distinct from a normal favicon load?
Perhaps we should only permit loading of favicons from the jar when the page url begins with "about:"? This would perhaps be the simplest of the two suggestions.

Either of the above are probably a neat way to avoid browser.js hackery.

Comment 2

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #1)

> Either of the above are probably a neat way to avoid browser.js hackery.

The issue is that the Gecko browser knows whether the page is a privileged chrome page or untrusted web content. We want the former to be able to load favicons from jars, but not the latter. The Java side shouldn't have to make this decision.

Comment 3

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #2)
> The issue is that the Gecko browser knows whether the page is a privileged
> chrome page or untrusted web content. We want the former to be able to load
> favicons from jars, but not the latter. The Java side shouldn't have to make
> this decision.

I see.

Urgh.

So this is going to need a method for the JS to request such loading from the Java side when required?
Is there no way the Java can easily determine which pages are privelaged chrome pages without needing to involve JS? Naturally, less IPC is always better.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Chris Kitching [:ckitching] from comment #3)

> So this is going to need a method for the JS to request such loading from
> the Java side when required?

No, we'll just filter the URLs that we're already sending from JS to Java; pages that are being a bit lairy just appear to have no favicon set. This is strictly a reduction.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

What's the crash stack look like?

Comment 6

[Richard Newman [:rnewman]]

Brian, did you grab a stack?

Comment 7

[Chris Kitching [:ckitching]]

I can reproduce this - here's the stack:


10-29 22:50:38.025: ERROR/GeckoAppShell(5918): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 278 ("GeckoBackgroundThread")
        java.lang.StringIndexOutOfBoundsException: length=15; regionStart=4; regionLength=-5
        at java.lang.String.startEndAndLength(String.java:593)
        at java.lang.String.substring(String.java:1474)
        at org.mozilla.gecko.util.GeckoJarReader.parseUrl(GeckoJarReader.java:163)
        at org.mozilla.gecko.util.GeckoJarReader.parseUrl(GeckoJarReader.java:153)
        at org.mozilla.gecko.util.GeckoJarReader.getBitmapDrawable(GeckoJarReader.java:35)
        at org.mozilla.gecko.util.GeckoJarReader.getBitmap(GeckoJarReader.java:30)
        at org.mozilla.gecko.favicons.LoadFaviconTask.downloadFavicon(LoadFaviconTask.java:164)
        at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground(LoadFaviconTask.java:286)
        at org.mozilla.gecko.favicons.LoadFaviconTask.doInBackground(LoadFaviconTask.java:42)
        at org.mozilla.gecko.util.UiAsyncTask$BackgroundTaskRunnable.run(UiAsyncTask.java:48)
        at android.os.Handler.handleCallback(Handler.java:605)
        at android.os.Handler.dispatchMessage(Handler.java:92)
        at android.os.Looper.loop(Looper.java:137)
        at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)


It's not hugely interesting in and of itself, since we know the problem.

Comment 8

[Brian Nicholson (:bnicholson)]

Yeah, what Chris said.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Shouldn't we be hardening this code a bit:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/GeckoJarReader.java#163

Looks like it kind of assumes a "!" will be in the URL string.

Comment 10

[Richard Newman [:rnewman]]

Yes, but that's orthogonal to not letting arbitrary websites load content out of our JAR (or exploit other coding errors).

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Looks like it kind of assumes a "!" will be in the URL string.

A "!" will be in a _valid_ jar: URL, but of course the code should handle invalid URLs gracefully.

The bug in it's current form looks like a DoS -- does this need to be a hidden security bug?

Comment 12

[Chris Kitching [:ckitching]]

(In reply to Daniel Veditz [:dveditz] from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > Looks like it kind of assumes a "!" will be in the URL string.
> 
> A "!" will be in a _valid_ jar: URL, but of course the code should handle
> invalid URLs gracefully.
> 
> The bug in it's current form looks like a DoS -- does this need to be a
> hidden security bug?

I don't think it's a DoS - the issue is that by crafting a website with a jar:jar URL you can have us load anything you like from our jar file as your favicon.
What happens if you target some JS with this approach, or something?

Comment 13

[Brian Nicholson (:bnicholson)]

(In reply to Chris Kitching [:ckitching] from comment #12)
> I don't think it's a DoS - the issue is that by crafting a website with a
> jar:jar URL you can have us load anything you like from our jar file as your
> favicon.
> What happens if you target some JS with this approach, or something?

Saying we "load" anything is a bit misleading -- we try to read bytes from the JAR file, which we then try to decode into a bitmap. We should of course avoid doing this, but I don't see how that could be used to actually execute code (especially considering that this is happening on the Java side, isolated from Gecko).

Comment 14

[Richard Newman [:rnewman]]

It'd be a DoS if we consistently OOMed trying to load and decode something huge, or -- as bnicholson's current example shows, which is fixed in my queue -- doing so makes us throw.

Comment 15

[Richard Newman [:rnewman]]

Anyone in the mood to reverify this?

Comment 16

[:Margaret Leibovic]

Doing some sec bug clean-up... ahunt, can you take a look at this to see if it's still an issue?

Comment 17

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #16)
> Doing some sec bug clean-up... ahunt, can you take a look at this to see if
> it's still an issue?

Yes, it's still an issue (the first page linked still loads the jar favicon when opened on mobile). Should I start work on fixing this?

Comment 18

[:Margaret Leibovic]

(In reply to Andrzej Hunt :ahunt from comment #17)
> (In reply to :Margaret Leibovic from comment #16)
> > Doing some sec bug clean-up... ahunt, can you take a look at this to see if
> > it's still an issue?
> 
> Yes, it's still an issue (the first page linked still loads the jar favicon
> when opened on mobile). Should I start work on fixing this?

Yes, please :)

Comment 19

[Andrzej Hunt :ahunt]

In Bug 1099088 we'll be adding a CHROMEUI SecurityMode to SiteIdentity (which is passed through from browser.js) - we could maybe use to determine whether or not we can use the jar favicons on the java side. The only issue I see there is that we'd determine what counts as a CHROMEUI page using a whitelist in browser.js. (I'm going to have a look at whether there's a better way of doing that...)

Comment 20

[Andrzej Hunt :ahunt]

Attachment: Pre: whitespace fix

Comment 21

[Andrzej Hunt :ahunt]

Attachment: Part 1: don't load jar:jar: favicons for external pages

Not yet sure who to ask to review this.

This patch depends on landing Bug 1099088, or at least the Part 1 patch for that ( https://reviewboard.mozilla.org/r/29973/diff/1#index_header )

Comment 22

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8705784 [details] [diff] [review]
Part 1: don't load jar:jar: favicons for external pages

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

I feel like this could be a fertile place for a JUnit 4 test, since you could have a dummy listener that reports success.

::: mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java
@@ +191,5 @@
>       * Otherwise, the result is drawn from the database or network and the listener invoked when the
>       * result becomes available.
>       *
>       * @param pageURL Page URL for which a Favicon is desired.
> +     * @param isPrivileged Whether the page is a privileged (about:) page

This thing already has 5 parameters, and boolean flags of this sort are very easy to miss.

Consider making this an enum, and strongly consider making two methods, so that it's always clear.

@@ +201,5 @@
>       * @return The id of the asynchronous task created, NOT_LOADING if none is created, or
>       *         LOADED if the value could be dispatched on the current thread.
>       */
> +    public static int getSizedFavicon(Context context, String pageURL, boolean isPrivileged, String faviconURL, int targetSize, int flags, OnFaviconLoadedListener listener) {
> +        if (faviconURL != null && faviconURL.startsWith("jar:jar:") && !isPrivileged) {

First, test the cheap thing (!isPrivileged) first.

Second, shouldn't this go later and test `cacheURL`?

Finally, why not go whole-hog and restrict `!isPrivileged` to `faviconURL.startsWith("http://") || faviconURL.startsWith("https://")`?  It seems foolish to allow "jar:" and "chrome:" and other things that shouldn't be available to content.

Is it possible that URLs are not absolute at this point?

Comment 23

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8705782 [details] [diff] [review]
Pre: whitespace fix

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

No need for review of such things.  Consider getting review if you're re-formatting a whole file or a whole directory, but no need for nits.  We trust you!

Comment 24

[Andrzej Hunt :ahunt]

Thanks for the review - I have a few questions remaining (inline) before I resubmit!

(In reply to Nick Alexander :nalexander from comment #22)
> Comment on attachment 8705784 [details] [diff] [review]
> Part 1: don't load jar:jar: favicons for external pages

<SNIP: I'm working on the unit test + method splitting.>

> @@ +201,5 @@
> >       * @return The id of the asynchronous task created, NOT_LOADING if none is created, or
> >       *         LOADED if the value could be dispatched on the current thread.
> >       */
> > +    public static int getSizedFavicon(Context context, String pageURL, boolean isPrivileged, String faviconURL, int targetSize, int flags, OnFaviconLoadedListener listener) {
> > +        if (faviconURL != null && faviconURL.startsWith("jar:jar:") && !isPrivileged) {
> 
> First, test the cheap thing (!isPrivileged) first.

Done!

> Second, shouldn't this go later and test `cacheURL`?

Our cache shouldn't ever associate a jar:jar: icon with a page (the page is only inserted into the cache if we successfully fetch an icon the first time round - but if a page attempts to use a jar:jar: icon, we don't fetch the icon, and don't put it in the url cache). (I'll add a comment about this.)

I.e. we only put the pages into the cache when calling Favicons.putFaviconURLForPageURLInCache, which is only used in LoadFaviconTask - and we never reach LoadFaviconTask with a jar:jar: url for an unprivileged page.

However I'd like to do the test later to protect against any changes in our caching behaviour - does that sound sensible?

> Finally, why not go whole-hog and restrict `!isPrivileged` to
> `faviconURL.startsWith("http://") || faviconURL.startsWith("https://")`?  It
> seems foolish to allow "jar:" and "chrome:" and other things that shouldn't
> be available to content.

Done!

> Is it possible that URLs are not absolute at this point?

We resolve the URLs (for both favicons and apple-touch-icons) at [1], so the URLs should be absolute here.

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4069

Comment 25

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Andrzej Hunt :ahunt from comment #24)
> Thanks for the review - I have a few questions remaining (inline) before I
> resubmit!
> 
> (In reply to Nick Alexander :nalexander from comment #22)
> > Comment on attachment 8705784 [details] [diff] [review]
> > Part 1: don't load jar:jar: favicons for external pages
> 
> <SNIP: I'm working on the unit test + method splitting.>
> 
> > @@ +201,5 @@
> > >       * @return The id of the asynchronous task created, NOT_LOADING if none is created, or
> > >       *         LOADED if the value could be dispatched on the current thread.
> > >       */
> > > +    public static int getSizedFavicon(Context context, String pageURL, boolean isPrivileged, String faviconURL, int targetSize, int flags, OnFaviconLoadedListener listener) {
> > > +        if (faviconURL != null && faviconURL.startsWith("jar:jar:") && !isPrivileged) {
> > 
> > First, test the cheap thing (!isPrivileged) first.
> 
> Done!
> 
> > Second, shouldn't this go later and test `cacheURL`?
> 
> Our cache shouldn't ever associate a jar:jar: icon with a page (the page is
> only inserted into the cache if we successfully fetch an icon the first time
> round - but if a page attempts to use a jar:jar: icon, we don't fetch the
> icon, and don't put it in the url cache). (I'll add a comment about this.)
> 
> I.e. we only put the pages into the cache when calling
> Favicons.putFaviconURLForPageURLInCache, which is only used in
> LoadFaviconTask - and we never reach LoadFaviconTask with a jar:jar: url for
> an unprivileged page.
> 
> However I'd like to do the test later to protect against any changes in our
> caching behaviour - does that sound sensible?

Thanks for the explanation -- consider adding a shortened version as a commnet.  But yes, sounds good to push the test a little later, at your discretion.

Rock on!

Comment 26

[Andrzej Hunt :ahunt]

Attachment: Part 1: don't load jar:jar: favicons for external pages

Comment 27

[Andrzej Hunt :ahunt]

Attachment: Part 2: add about:home to CHROMEUI whitelist

Comment 28

[Andrzej Hunt :ahunt]

Attachment: WIP: junit4 test

I've started working on the test - it's extremeley basic currently, and only tests that we can't load jar:jar: (and other non http[s]:// icons). In order to test that we can successfully load jar:jar: icons for internal pages / http[s] icons for normal pages, I'll need to spend some more time looking into making the rest of the Favicon infrastructure actually run in the test environment.

Comment 29

[Chris Kitching [:ckitching]]

Comment on attachment 8713412 [details] [diff] [review]
Part 1: don't load jar:jar: favicons for external pages

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

Haven't done this for a while. How exciting.

::: mobile/android/base/java/org/mozilla/gecko/Tab.java
@@ +481,5 @@
> +        if (mSiteIdentity.getSecurityMode() == SiteIdentity.SecurityMode.CHROMEUI) {
> +            mFaviconLoadId = Favicons.getPrivilegedSizedFavicon(mAppContext, mUrl, mFaviconUrl, Favicons.browserToolbarFaviconSize, flags, listener);
> +        } else {
> +            mFaviconLoadId = Favicons.getUnprivilegedSizedFavicon(mAppContext, mUrl, mFaviconUrl, Favicons.browserToolbarFaviconSize, flags, listener);
> +        }

Such duplication. Very wow.

Your first approach had two calls here that were identical modulo one parameter, now they're identical modulo a function that delegates to that parameter. I can haz template parameters?

Unfortunately, no, I can't, since Java remains resolutely insane.

What about using inheritance to solve your ickiness problem? Functor classes are a handy way of creating specialised versions of functions in ways that avoid piles of obnoxious mode flags or enums.
This is probably why factory-factories (and, in the presence of overzealousness, explosions within such) are such a common thing in Java. I expect you'd end up with this block looking roughly like this:

boolean isPrivelaged = mSiteIdentity.getSecurityMode() == SiteIdentity.SecurityMode.CHROMEUI;
mFaviconLoadId = Favicons.getFaviconLoader(isPrivalaged).getSizedFavicon(mAppContext, mUrl, mFaviconUrl, Favicons.browserToolbarFaviconSize, flags, listener);

I expect having done that, you'd find other areas in the favicon system that could be simplified as a result. You'd also officially be one of *those* Java programmers.

::: mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java
@@ +202,4 @@
>              return dispatchResult(pageURL, null, defaultFavicon, listener);
> +        } else if (!isPrivileged &&
> +                !(cacheURL.startsWith("http://") || cacheURL.startsWith("https://"))) {
> +            return NOT_LOADING;

Is there any particular reason we can't serve favicons over ftp or such? Having URL schemes hardcoded is mildly nauseating in any case (then again, I probably did this in the first place, and it's comically offtopic).

@@ +244,5 @@
> +     */
> +    public static int getUnprivilegedSizedFavicon(Context context, String pageURL, String faviconURL, int targetSize, int flags, OnFaviconLoadedListener listener) {
> +        return getSizedFavicon(context, pageURL, false, faviconURL, targetSize, flags, listener);
> +    }
> +

Icky duplicated delegator functions with squillions of parameters. Calling for a factory and inheritance, methinks.

(Or, you know, C++ templates. I'm sure Java will get something like that eventually. Probably when the current compiler-dev team dies of old age)

Comment 30

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8713412 [details] [diff] [review]
Part 1: don't load jar:jar: favicons for external pages

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

Update the commit message to be more clear, for example "Differentiate between unprivileged (http(s) URLs) and privileged URLs (which can include chrome://, jar://, ...)."

::: mobile/android/base/java/org/mozilla/gecko/Tab.java
@@ +486,3 @@
>      }
>  
>      public synchronized void clearFavicon() {

ckitching's point is well made here.  An enum may be better, but I don't feel strongly about it.

::: mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java
@@ +184,5 @@
>          }
>          return getSizedFaviconFromCache(faviconURL, targetSize);
>      }
>  
> +    private static int getSizedFavicon(Context context, final String pageURL, boolean isPrivileged, final String faviconURL, int targetSize, int flags, OnFaviconLoadedListener listener) {

Add a note about what `isPrivileged` means here:

// If `isPrivileged` is false, the favicon URL must start with "http://" or "https://".  If `isPrivileged` is true, the favicon URL is unrestricted, and can expose sensitive resources like "chrome://" or "resource://" URLs.

@@ +206,1 @@
>          }

Meh.  We can blacklist (resource://, chrome://, jar://) or we can whitelist.

@@ +249,1 @@
>      /**

ckitching: possibly.  I asked for the explicit name in the function, because I want every caller to consider whether they are privileged or not.  A hidden boolean "privileged" isn't great for surfacing subtle security options.  I could imagine an enum, and I think I floated it as an option...

Comment 31

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8713413 [details] [diff] [review]
Part 2: add about:home to CHROMEUI whitelist

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

This is so fragile.  How are we not slurping this from https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js#11?  I have commented on the (public) ticket about this, and would like to see this made less fragile.

Let's land this for now and improve in (public) follow-up.

Comment 32

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8713414 [details] [diff] [review]
WIP: junit4 test

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

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/favicons/TestPrivilegedIcons.java
@@ +30,5 @@
> +                assertTrue(false);
> +            }
> +        };
> +
> +        // Currently we can only test load failures. Successfully testing loading depends on calling

Interesting.  Thanks for exploring this!  I'd like to understand more about what we can and cannot test with Robolectric.

Comment 33

[:Margaret Leibovic]

Is there anything blocking progress on this bug?

Comment 34

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #33)
> Is there anything blocking progress on this bug?

I'm back to looking at this now! (As of bug 1018994 we don't show the favicon in the URL bar, which makes this less immediate - otoh there's no way of stopping a site from simply hosting the desired icon themselves if they want to spoof a built-in page.)

We've had an explosion of getSizedFavicon* methods in Favicon, so our current patches don't apply, and I'll have to rethink my approach here.

Comment 35

[Andrzej Hunt :ahunt]

Attachment: Pre: 1-3: whitespace and visibility fixup

Comment 36

[Andrzej Hunt :ahunt]

Attachment: Pre 4: deduplicate code to reduce mental overhead

Comment 37

[Andrzej Hunt :ahunt]

Attachment: Part 1: distinguish between privilege and unpirivileged favicon loading

Comment 38

[Andrzej Hunt :ahunt]

Attachment: Part 2: add about:home to CHROMEUI whitelist

Comment 39

[Andrzej Hunt :ahunt]

Comment on attachment 8755600 [details] [diff] [review]
Pre 4: deduplicate code to reduce mental overhead

I'm splitting this patch into a separate bug to allow landing the actual fix.

Comment 40

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/14a136ae4655bc17fef1df6ab6e4cf5ccc8ad2d4
Bug 932335 - Pre 1: fix whitespace r=me

https://hg.mozilla.org/integration/fx-team/rev/41b8ce50bc2726cf588680646bae48737f57cf67
Bug 932335 - Pre 2: make getSizedFaviconFromCache package-private r=me

https://hg.mozilla.org/integration/fx-team/rev/228556a98ec92b6ca7782e1bd9f8c0dc70aae6a5
Bug 932335 - Pre 3: Make getPreferredSizeFaviconForPage private r=me

https://hg.mozilla.org/integration/fx-team/rev/0a86d2aee5eb037767c6161b89a6816d64eda887
Bug 932335 - Part 1: differentiate between unprivileged (http(s) URLs) and privileged URLs r=nalexander

https://hg.mozilla.org/integration/fx-team/rev/37846cb483b903ff7ab8df396ede6b4c0889035e
Bug 932335 - Part 2: add about:home to CHROMEUI whitelist r=nalexander

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14a136ae4655
https://hg.mozilla.org/mozilla-central/rev/41b8ce50bc27
https://hg.mozilla.org/mozilla-central/rev/228556a98ec9
https://hg.mozilla.org/mozilla-central/rev/0a86d2aee5eb
https://hg.mozilla.org/mozilla-central/rev/37846cb483b9