bug 580794 - strip userpass from favicon uri (r=filipc,ctalbert)

browser/base/content/browser.js

@@ -3091,6 +3091,17 @@ const DOMLinkHandler = {
             var targetDoc = link.ownerDocument;
             var uri = makeURI(link.href, targetDoc.characterSet);
 
+            // 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 != "") {
+              // setting the userPass can throw, so try/catch it
+              try {
+                uri.userPass = null;
+              }
+              catch (e) {}
+            }
+
             if (gBrowser.isFailedIcon(uri))
               break;
 
@@ -3135,7 +3146,7 @@ const DOMLinkHandler = {
               break;
 
             let tab = gBrowser.tabs[browserIndex];
-            gBrowser.setIcon(tab, link.href);
+            gBrowser.setIcon(tab, uri);
             iconAdded = true;
           }
           break;

browser/base/content/test/Makefile.in

@@ -262,6 +262,9 @@ _BROWSER_FILES = \
                  social_panel.html \
                  social_sidebar.html \
                  social_worker.js \
+                 browser_favicon_no_userPass.js \
+                 favicon_no_userPass.html \
+                 icon.ico \
                  $(NULL)
 
 ifneq (cocoa,$(MOZ_WIDGET_TOOLKIT))

browser/base/content/test/browser_favicon_no_userPass.js

@@ -0,0 +1,96 @@
+var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
+var gIOService = Cc["@mozilla.org/network/io-service;1"]
+                 .getService(Ci.nsIIOService);
+var gFaviconService = Cc["@mozilla.org/browser/favicon-service;1"]
+                      .getService(Ci.nsIFaviconService);
+var gTestBrowser = null;
+var gNextTest = null;
+var gOkayIfIconChanges = false;
+
+Components.utils.import("resource://gre/modules/Services.jsm");
+
+var gHistoryService = Cc["@mozilla.org/browser/nav-history-service;1"]
+                      .getService(Ci.nsINavHistoryService);
+
+var gHistoryObserver = {
+  onBeginUpdateBatch: function() {},
+  onEndUpdateBatch: function() {},
+  onVisit: function(aURI, aVisitID, aTime, aSessionID, aReferringID, aTransitionType) {},
+  onTitleChanged: function(aURI, aPageTitle) {},
+  onBeforeDeleteURI: function(aURI) {},
+  onDeleteURI: function(aURI) {},
+  onClearHistory: function() {},
+  onDeleteVisits: function() {},
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]),
+
+  onPageChanged: function(aURI, aWhat, aValue) {
+    if (aWhat == Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON) {
+      if (gOkayIfIconChanges) {
+        ok(true, "favicon changed");
+        finishTest();
+      }
+      else {
+        ok(false, "favicon should not have changed now");
+        finishTest();
+      }
+    }
+  }
+};
+
+gHistoryService.addObserver(gHistoryObserver, false);
+
+function test() {
+  waitForExplicitFinish();
+
+  var newTab = gBrowser.addTab();
+  gBrowser.selectedTab = newTab;
+  gTestBrowser = gBrowser.selectedBrowser;
+  gTestBrowser.addEventListener("load", pageLoad, true);
+  gBrowser.selectedTab.addEventListener("error", part2, true);
+  prepareTest(stub1, gTestRoot + "favicon_no_userPass.html#link");
+}
+
+function pageLoad() {
+  executeSoon(gNextTest);
+}
+
+function prepareTest(nextTest, url) {
+  gNextTest = nextTest;
+  gTestBrowser.contentWindow.location = url;
+}
+
+function stub1() {
+  gNextTest = null;
+}
+
+function part2() {
+  ok(true, "got expected error for loading favicon");
+  gBrowser.selectedTab.removeEventListener("error", part2, true);
+  prepareTest(part3, gTestRoot + "favicon_no_userPass.html#img");
+  gTestBrowser.reload();
+}
+
+function part3() {
+  gBrowser.selectedTab.addEventListener("error", stub2, true);
+  gOkayIfIconChanges = true;
+  prepareTest(part4, gTestRoot + "favicon_no_userPass.html#link2");
+  gTestBrowser.reload();
+}
+
+function part4() {
+  gNextTest = null;
+}
+
+function stub2() {
+  ok(false, "got unexpected error for loading favicon the second time");
+  finishTest();
+}
+
+function finishTest() {
+  gTestBrowser.removeEventListener("load", pageLoad, true);
+  gHistoryService.removeObserver(gHistoryObserver);
+  gBrowser.selectedTab.removeEventListener("error", stub2, true);
+  gBrowser.removeCurrentTab();
+  window.focus();
+  finish();
+}

browser/base/content/test/favicon_no_userPass.html

@@ -0,0 +1,32 @@
+<html>
+<head>
+<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
+</head>
+<body>
+<script>
+  var path = document.location.href.replace("127.0.0.1:8888", "guest:guest@127.0.0.1:8888/auth").split("/").slice(0,-1).join("/");
+  var frags = document.location.hash.substr(1).split(",");
+  for (var i in frags) {
+    if (frags[i] == "link") {
+      var link = document.createElement("link");
+      link.rel = "icon";
+      link.href = path + "/icon.ico";
+      document.head.appendChild(link);
+    }
+
+    if (frags[i] == "img") {
+      var img = document.createElement("img");
+      img.src = path + "/icon.ico";
+      document.body.appendChild(img);
+    }
+
+    if (frags[i] == "link2") {
+      var link = document.createElement("link");
+      link.rel = "icon";
+      link.href = path + "/icon.ico#foo";
+      document.head.appendChild(link);
+    }
+  }
+</script>
+</body>
+</html>

testing/mochitest/server.js

@@ -201,6 +201,7 @@ function createMochitestServer(serverBasePath)
   server.registerDirectory("/", serverBasePath);
   server.registerPathHandler("/server/shutdown", serverShutdown);
   server.registerPathHandler("/server/debug", serverDebug);
+  server.registerPrefixHandler("/auth/", authPrefixHandler);
   server.registerContentType("sjs", "sjs"); // .sjs == CGI-like functionality
   server.registerContentType("jar", "application/x-jar");
   server.registerContentType("ogg", "application/ogg");
@@ -664,3 +665,36 @@ function defaultDirHandler(metadata, response)
     response.write(ex);
   }  
 }
+
+/**
+ * Respond with 401 Authorization Required unless test sends proper
+ * authorization (user: guest, pass: guest, which happens to be 
+ * "Z3Vlc3Q6Z3Vlc3Q=" encoded), in which case fall back to the default handler.
+ */
+function authPrefixHandler(metadata, response)
+{
+  if (!metadata.hasHeader("Authorization")) {
+    response.setStatusLine("1.1", 401, "Authorization Required");
+    response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);
+    return;
+  }
+
+  let auth = metadata.getHeader("Authorization");
+  if (auth.indexOf("Basic ") == 0) {
+    let encoded = auth.substr(6);
+    if (encoded != "Z3Vlc3Q6Z3Vlc3Q=") {
+      response.setStatusLine("1.1", 401, "Authorization Required");
+      response.setHeader("WWW-Authenticate", 'Basic realm="secret"', false);
+      return;
+    }
+
+    // Breaking the abstraction... :(
+    // strip off the leading '/auth'
+    metadata._path = metadata.path.substr(5);
+    // call the default handler
+    server._handler._handleDefault(metadata, response);
+  } else {
+    response.setStatusLine("1.1", 400, "Bad Request");
+    return;
+  }
+}

toolkit/components/places/nsFaviconService.cpp

@@ -190,6 +190,13 @@ nsFaviconService::SetFaviconUrlForPage(nsIURI* aPageURI, nsIURI* aFaviconURI)
 {
   NS_ENSURE_ARG(aPageURI);
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   // If we are about to expire all favicons, don't bother setting a new one.
   if (mFaviconsExpirationRunning) {
@@ -348,6 +355,13 @@ nsFaviconService::SetAndLoadFaviconForPage(nsIURI* aPageURI,
 {
   NS_ENSURE_ARG(aPageURI);
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   if (mFaviconsExpirationRunning)
     return NS_OK;
@@ -464,6 +478,13 @@ nsFaviconService::SetFaviconData(nsIURI* aFaviconURI, const PRUint8* aData,
                                  PRTime aExpiration)
 {
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   if (mFaviconsExpirationRunning)
     return NS_OK;
@@ -638,6 +659,14 @@ nsFaviconService::SetFaviconDataFromDataURL(nsIURI* aFaviconURI,
                                             PRTime aExpiration)
 {
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
+
   if (mFaviconsExpirationRunning)
     return NS_OK;
 
@@ -699,6 +728,13 @@ nsFaviconService::GetFaviconData(nsIURI* aFaviconURI, nsACString& aMimeType,
   NS_ENSURE_ARG(aFaviconURI);
   NS_ENSURE_ARG_POINTER(aDataLen);
   NS_ENSURE_ARG_POINTER(aData);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   nsCOMPtr<nsIURI> defaultFaviconURI;
   nsresult rv = GetDefaultFavicon(getter_AddRefs(defaultFaviconURI));
@@ -777,6 +813,13 @@ nsFaviconService::GetFaviconDataAsDataURL(nsIURI* aFaviconURI,
                                           nsAString& aDataURL)
 {
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   PRUint8* data;
   PRUint32 dataLen;
@@ -908,6 +951,13 @@ nsFaviconService::GetFaviconLinkForIcon(nsIURI* aFaviconURI,
 {
   NS_ENSURE_ARG(aFaviconURI);
   NS_ENSURE_ARG_POINTER(aOutputURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   nsCAutoString spec;
   if (aFaviconURI) {
@@ -934,6 +984,13 @@ NS_IMETHODIMP
 nsFaviconService::AddFailedFavicon(nsIURI* aFaviconURI)
 {
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
 
   nsCAutoString spec;
   nsresult rv = aFaviconURI->GetSpec(spec);
@@ -972,6 +1029,14 @@ NS_IMETHODIMP
 nsFaviconService::IsFailedFavicon(nsIURI* aFaviconURI, bool* _retval)
 {
   NS_ENSURE_ARG(aFaviconURI);
+#ifdef DEBUG
+  nsCAutoString userPass;
+  if (NS_SUCCEEDED(aFaviconURI->GetUserPass(userPass)) &&
+      !userPass.IsEmpty()) {
+    NS_WARNING("security concern: favicon URIs should not have username/password");
+  }
+#endif
+
   nsCAutoString spec;
   nsresult rv = aFaviconURI->GetSpec(spec);
   NS_ENSURE_SUCCESS(rv, rv);