580794 - HTTP auth via favicon URL fails silently with invalid user:pass

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

Reported to me by Elie Burzstein: 

Favicon requests can be constructed with "http://user:[e-mail]" URIs, and the user:pass is used for HTTP authentication.  If the authentication fails, it fails without notice to the user and a cleverly-crafted javascript can then stealthily perform a brute-force attack on an HTTP auth-protected URL (like a home router).

Can we disallow user:pass in HTTP/HTTPS URIs in general?  We'd have to rely on the user to authenticate to the appropriate domain in a content window before the cached credentials can be used to fetch the favicon.

Comment 1

[Johnny Stenback (:jst)]

sg:low per discussion in todays critsmash meeting.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree we should disable user:pass in URIs. Only bad things seem to come out of it.

How does one detect if the favicon request succeeded or not though?

Comment 3

[Elie Bursztein]

As fare as U can tell you can't test. We tried a lot of stuff none work. It is a blind shot but the user won't get any warning if it fail. So you can try direct injection / ddos.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

This just strips out the userpass of the favicon uri in browser.js (which eventually makes its way to the favicon service and is also requested from the css handling that actually puts the icon in the tab).
Couple of things I'm not sure about:
1. I added warnings in nsFaviconService.cpp when it detects favicon uris with non-empty userpasses - should we do more than this? less?
2. I needed to add 401 Authorization Required responses to the js http server and ended up breaking the abstraction a bit. Is there a better way to do this?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 636805 [details] [diff] [review]
patch

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

Looks pretty good.  You've got test blocks in a bunch of the favicon functions to make sure the URI doesn't have a userpass... are these necessary if we have good tests and strip it out of URIs when calling the service?

Comment 6

[Dana Keeler (she/her) [:keeler]]

Yeah, I guess that doesn't make a lot of sense. My concern was if someone came along later and added code that used the service without checking for a userpass. I guess this is more of a documentation issue. Also, there are other places currently where the services is used - should I modify them to take out any userpasses?

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Hm, that's a good concern.  Maybe your approach is more solid than changing the callers.

Comment 8

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 636805 [details] [diff] [review]
patch

Felipe - would you mind taking a look at this if/when you have time?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 636805 [details] [diff] [review]
patch

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

I looked at everything except server.js which should probably be reviewed by ctalbert.

::: browser/base/content/browser.js
@@ +3093,5 @@
> +            // strip the user:pass from the uri to prevent brute-forcing
> +            // attempts unbeknownst to the user (favicon requests don't
> +            // prompt for auth)
> +            if (uri.userPass != "")
> +              uri.userPass = null;

setting the userPass can throw (for example on the about: pages), so please wrap this in a try catch. I believe the check for != "" will cover any immutable URL but it's better to be safe.

Most other code clearing the userPass just sets it straight to "" (no checking before), but the getter seems considerably cheaper so I like it this way

is it better to land this without the comment explaining it?

::: toolkit/components/places/nsFaviconService.cpp
@@ +193,5 @@
> +  nsCAutoString userPass;
> +  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
> +      !userPass.IsEmpty()) {
> +    NS_WARNING("security concern: favicon URIs should not have username/password");
> +  }

In opt builds NS_WARNING will be out but we'll still be running GetUserPass and IsEmpty for nothing. I wonder if it's useful to wrap it all in #ifdef DEBUG for that. It's probably not a big deal.

Comment 10

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2

I added the try/catch - should there be anything in the catch block?

I also added "#ifdef DEBUG"s - it does seem like a waste to run those functions for nothing, even if they're fairly simple.

dveditz said we shouldn't worry about landing the patch with a comment that explains the vulnerability - it's pretty apparent from the rest of the patch what's going on, and it's sec-low anyway.

ctalbert - could take a look at the server.js changes?

Comment 11

[cmtalbert]

Comment on attachment 643671 [details] [diff] [review]
patch v2

server.js changes look good

Comment 12

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2 rebased

Rebased patch b/c it no longer applied cleanly. Carrying over r+ and marking checkin-needed.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b77834cb31ad

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: rebased and landed patch

Had to merge the changes in browser/base/content/test/Makefile.in with mozilla-inbound before pushing, so I'm attaching this patch that landed.  Carrying over r+ from ctalbert and filipc.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I don't think you should make the nsFaviconService changes, consumers of this API that might make mistakes like this aren't likely to be running debug builds, so the warning isn't useful.

Comment 16

[Dana Keeler (she/her) [:keeler]]

Backed out due to test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb58cbbb352

Comment 17

[Dana Keeler (she/her) [:keeler]]

Well, looks like bug 779686 makes this unnecessary. Is it still worth it to land the test? (which requires changes to testing/mochitest/server.js that are a bit fragile)

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Hmm, I forgot that that bug was fixing this one.

What's fragile about the server.js changes? Having some test coverage would be great, and it looks potentially useful to other tests as well.

Comment 19

[Dana Keeler (she/her) [:keeler]]

I feel like this could come back to bite us, but I was more reluctant to make significant changes to httpd.js.

+    // strip off the leading '/auth'
+    metadata._path = metadata.path.substr(5);
+    // call the default handler
+    server._handler._handleDefault(metadata, response);

Comment 20

[Dana Keeler (she/her) [:keeler]]

I'm unlikely to do further work on this bug. The original vulnerability has been fixed (see comment 17). It would probably be good to fix up and land the test so this doesn't regress, but I don't have the time and this isn't a priority for me.

Comment 21

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.