Closed Bug 932335 (CVE-2016-5282) Opened 6 years ago Closed 4 years ago

Don't allow content to request favicons from non-whitelisted schemes

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox25 --- ?
firefox26 --- ?
firefox27 --- affected
firefox49 --- fixed

People

(Reporter: rnewman, Assigned: ahunt)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main49+])

Attachments

(5 files, 4 obsolete files)

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.
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.
(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.
(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.
(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.
What's the crash stack look like?
Brian, did you grab a stack?
Flags: needinfo?(bnicholson)
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.
Yeah, what Chris said.
Flags: needinfo?(bnicholson)
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.
Yes, but that's orthogonal to not letting arbitrary websites load content out of our JAR (or exploit other coding errors).
(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?
Flags: needinfo?(rnewman)
(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?
(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).
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.
Flags: needinfo?(rnewman)
Anyone in the mood to reverify this?
Component: General → Favicon Handling
Group: core-security → firefox-core-security
Doing some sec bug clean-up... ahunt, can you take a look at this to see if it's still an issue?
Flags: needinfo?(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?
Flags: needinfo?(ahunt) → needinfo?(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 :)
Assignee: nobody → ahunt
Flags: needinfo?(margaret.leibovic)
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...)
Depends on: 1099088
Priority: -- → P1
Attached patch Pre: whitespace fix (obsolete) — Splinter Review
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 )
Attachment #8705782 - Flags: review?(nalexander)
Attachment #8705784 - Flags: review?(nalexander)
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?
Attachment #8705784 - Flags: review?(nalexander)
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!
Attachment #8705782 - Flags: review?(nalexander) → review+
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
Flags: needinfo?(nalexander)
(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!
Flags: needinfo?(nalexander)
Attachment #8705784 - Attachment is obsolete: true
Attachment #8713412 - Flags: review?(nalexander)
Attachment #8713413 - Flags: review?(nalexander)
Attached patch WIP: junit4 testSplinter Review
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 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 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...
Attachment #8713412 - Flags: review?(nalexander) → review+
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.
Attachment #8713413 - Flags: review?(nalexander) → review+
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.
Attachment #8713414 - Flags: feedback+
Is there anything blocking progress on this bug?
Flags: needinfo?(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.
Flags: needinfo?(ahunt)
Attachment #8705782 - Attachment is obsolete: true
Attachment #8713413 - Attachment is obsolete: true
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.
Attachment #8755600 - Flags: review?(s.kaspari)
Group: firefox-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main49+]
Alias: CVE-2016-5282
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.