341946 - Safe browsing considers all jar: URI's safe

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[:Gijs (he/him)]

http://lxr.mozilla.org/seamonkey/source/browser/components/safebrowsing/content/phishing-warden.js#509

This seems odd, considering:

jar:http://chatzilla.hacksrus.com/xpi/chatzilla-0.9.73.xpi!/install.rdf
will just be opened

or even:

jar:jar:http://chatzilla.hacksrus.com/xpi/chatzilla-0.9.73.xpi!/chrome/chatzilla.jar!/content/chatzilla/chatzilla.xul

This doesn't work because the files in the xpi obviously rely on being installed as an extension, but it goes to show that you *can* have active and useful/dangerous content hidden in a jar uri. I fail to see why it should get the same treatment as chrome:// and about: URI's.
 I'm also wondering why about:config is in there, but about:buildconfig and about:mozilla and about:plugins and whatever else we have are not.

Maybe I'm misunderstanding code (I didn't care to read through all of it, I just wanted to debug something that failed, wasn't able to and was more interested in this bit that caught my eye).

Comment 1

[Mike Connor [:mconnor]]

So, this should be fixed, since its an easy way to bypass safebrowsing, and we don't want to play that game.

Comment 2

[Tony Chang (Google)]

Attachment: v1: examine jar: urls, ignore all about: urls

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 226285 [details] [diff] [review]
v1: examine jar: urls, ignore all about: urls

Looks fine to me.

Comment 4

[Tony Chang (Google)]

on trunk

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 226285 [details] [diff] [review]
v1: examine jar: urls, ignore all about: urls

> PROT_PhishingWarden.prototype.isSpurious_ = function(url) {
>-  return (url == "about:blank" ||
>-          url == "about:config" ||  
>+  return (url.startsWith("about:") ||
>           url.startsWith("chrome://") ||

You really ought to be using URI objects rather than strings. This function then becomes a series of uri.schemeIs() calls. This would go back to nsDocNavStartProgressListener::Observe where instead of doing nsIRequest::GetName() (which seems dodgy to me anyway) you should QI to the nsIChannel and get the URI there.

>-          url.startsWith("jar:") ||

On the trunk you could attempt to QI the nsIURI to nsINestedURI, and if it succeeds use the innermostURI for this routine. That would "see through" jar: and similar pseudo-protocols rather than having to unpack it by hand. Alas, that's not going to land on the 1.8 branch.

>           url.startsWith("javascript:"));

If javascript: is OK then data: should be, and vice versa. I think they're both problematic: all a phisher needs to do to evade detection is redirect to a javascript URL which can simply return the HTML of the phishing page as a string.

 document.location.href = "javascript:atob('"+btoa(document.body.innerHTML)+"');";

Sure, it'd look unusual in the location bar but the "Safe Browsing" feature is aimed at the sort of users who might not notice. If anything javascript: should be classed suspicious, not given a pass.

Comment 6

[Tony Chang (Google)]

(In reply to comment #5)
> (From update of attachment 226285 [details] [diff] [review] [edit])
> >           url.startsWith("javascript:"));
> 
> If javascript: is OK then data: should be, and vice versa. I think they're both
> problematic: all a phisher needs to do to evade detection is redirect to a
> javascript URL which can simply return the HTML of the phishing page as a
> string.
> 
>  document.location.href =
> "javascript:atob('"+btoa(document.body.innerHTML)+"');";
> 
> Sure, it'd look unusual in the location bar but the "Safe Browsing" feature is
> aimed at the sort of users who might not notice. If anything javascript: should
> be classed suspicious, not given a pass.

Actually, it looks like the javascript: filter does anything since the document.location.href doesn't get set.  We end up with a wysiwyg: uri which is then checked.

Comment 7

[Tony Chang (Google)]

(In reply to comment #6)
> Actually, it looks like the javascript: filter does anything since the
> document.location.href doesn't get set.  We end up with a wysiwyg: uri which is
> then checked.

Derrr, that should read, "it looks like the javascript: filter doesn't do anything".

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 226285 [details] [diff] [review]
v1: examine jar: urls, ignore all about: urls

So, aside from the pieces dveditz approved, this hasn't landed on the trunk and hasn't been reviewed by a module owner/peer.  Javascript is risky, and shouldn't get a free pass.

This should be backed out until these issues are properly addressed.

Comment 9

[Mike Connor [:mconnor]]

er, it has landed on the trunk, forgot to edit that part

Comment 10

[Tony Chang (Google)]

backed out of trunk, creating a new patch

Comment 11

[Tony Chang (Google)]

Attachment: v2: -jar, -javascript, +about

Incorporate feedback from dveditz.

Move isSpurious into C++ code (where it should be).

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 226609 [details] [diff] [review]
v2: -jar, -javascript, +about

># 
># old_revision [ecfc34109bac398873f9b312aedd4b73649c275d]
># 
># patch "browser/components/safebrowsing/content/phishing-warden.js"
>#  from [e3f3736610ac7908749861a01510c76dc990bb56]
>#    to [2721b123978930b11b6056875d14ba0a4907c6c5]
># 
># patch "browser/components/safebrowsing/src/nsDocNavStartProgressListener.cpp"
>#  from [3ce805bf01ee04ea29fdbad00ecb9e2184b80a47]
>#    to [ed9bee9afb69752bab51d2b7ef3ec521570f430e]
># 
># patch "browser/components/safebrowsing/src/nsDocNavStartProgressListener.h"
>#  from [4ac8ea3f88c6aaee92c383abf72725da8280b753]
>#    to [de66ec7bff71afc01b1f37d4d6668b8eec83e812]
># 
>Index: browser/components/safebrowsing/content/phishing-warden.js
>===================================================================
>--- browser/components/safebrowsing/content/phishing-warden.js	old
>+++ browser/components/safebrowsing/content/phishing-warden.js	new
>@@ -449,18 +449,17 @@ PROT_PhishingWarden.prototype.isBlacklis
>  * @param callback Function
>  */
> PROT_PhishingWarden.prototype.checkUrl_ = function(url, callback) {
>   // First check to see if it's a blacklist url.
>   if (this.isBlacklistTestURL(url)) {
>     callback();
>     return;
>   }
>-  if (!this.isSpurious_(url))
>-    this.isEvilURL_(url, callback);
>+  this.isEvilURL_(url, callback);
> }
> 
> /**
>  * Callback for found local blacklist match.  First we report that we have
>  * a blacklist hit, then we bring up the warning dialog.
>  */
> PROT_PhishingWarden.prototype.localListMatch_ = function(url, request) {
>   // Maybe send a report
>@@ -487,23 +486,8 @@ PROT_PhishingWarden.prototype.checkRemot
> 
>   if (trValues["phishy"] == 1) {     // It's on our blacklist 
>     G_Debug(this, "Remote blacklist hit");
>     callback(this);
>   } else {
>     G_Debug(this, "Remote blacklist miss");
>   }
> }
>-
>-/**
>- * Helper function to determine whether a given URL is "spurious" for some
>- * definition of "spurious".
>- *
>- * @param url String containing the URL to check
>- * 
>- * @returns Boolean indicating whether Fritz thinks it's too boring to notice
>- */ 
>-PROT_PhishingWarden.prototype.isSpurious_ = function(url) {
>-  return (url.startsWith("about:") ||
>-          url.startsWith("chrome://") ||
>-          url.startsWith("file://") ||
>-          url.startsWith("javascript:"));
>-}
>Index: browser/components/safebrowsing/src/nsDocNavStartProgressListener.cpp
>===================================================================
>--- browser/components/safebrowsing/src/nsDocNavStartProgressListener.cpp	old
>+++ browser/components/safebrowsing/src/nsDocNavStartProgressListener.cpp	new
>@@ -35,16 +35,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsCURILoader.h"
> #include "nsDocNavStartProgressListener.h"
> #include "nsIChannel.h"
> #include "nsIRequest.h"
> #include "nsITimer.h"
>+#include "nsIURI.h"
> #include "nsIWebProgress.h"
> #include "nsServiceManagerUtils.h"
> #include "nsString.h"
> 
> NS_IMPL_ISUPPORTS4(nsDocNavStartProgressListener,
>                    nsIDocNavStartProgressListener,
>                    nsIWebProgressListener,
>                    nsIObserver,
>@@ -95,16 +96,50 @@ nsDocNavStartProgressListener::DetachLis
>   nsresult rv;
>   nsCOMPtr<nsIWebProgress> webProgressService = do_GetService(
>       NS_DOCUMENTLOADER_SERVICE_CONTRACTID, &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return webProgressService->RemoveProgressListener(this);
> }
> 
>+/**
>+ * We ignore about:, chrome: and file: URIs.
>+ * It's better to err on the side of checking too many URIs.  So if
>+ * something fails, we return false.
>+ */
>+PRBool
>+nsDocNavStartProgressListener::IsSpurious(nsIRequest* aReq)
>+{
>+  nsCOMPtr<nsIChannel> channel;
>+  nsresult rv;
>+  channel = do_QueryInterface(aReq, &rv);
>+  if (NS_FAILED(rv)) return PR_FALSE;
>+
>+  nsCOMPtr<nsIURI> uri;
>+  rv = channel->GetURI(getter_AddRefs(uri));
>+  if (NS_FAILED(rv)) return PR_FALSE;
>+
>+  PRBool isSpurious;
>+
>+  if (NS_FAILED(uri->SchemeIs("about", &isSpurious)))
>+    return PR_FALSE;
>+  if (isSpurious)
>+    return isSpurious;
>+
>+  if (NS_FAILED(uri->SchemeIs("chrome", &isSpurious)))
>+    return PR_FALSE;
>+  if (isSpurious)
>+    return isSpurious;
>+
>+  if (NS_FAILED(uri->SchemeIs("file", &isSpurious)))
>+    return PR_FALSE;
>+  return isSpurious;
>+}

I don't understand the semantics of NS_FAILED here.  Don't we need to check for about, chrome, and file before returning false?

>+
> // nsIDocNavStartProgressCallback ***********************************************
> 
> // nsDocNavStartProgressListener::GetEnabled
> 
> NS_IMETHODIMP
> nsDocNavStartProgressListener::GetEnabled(PRBool* aEnabled)
> {
>   *aEnabled = mEnabled;
>@@ -270,20 +305,22 @@ nsDocNavStartProgressListener::Observe(n
>     NS_ASSERTION(length > 0, "timer callback with empty request queue?");
>     length = mTimers.Count();
>     NS_ASSERTION(length > 0, "timer callback with empty timer queue?");
> #endif
> 
>     nsIRequest* request = mRequests[0];
> 
>     if (mCallback) {
>-      nsCAutoString name;
>-      nsresult rv = request->GetName(name);
>-      NS_ENSURE_SUCCESS(rv, rv);
>+      if (!IsSpurious(request)) {
>+        nsCAutoString name;
>+        nsresult rv = request->GetName(name);
>+        NS_ENSURE_SUCCESS(rv, rv);
> 
>-      mCallback->OnDocNavStart(request, name);
>+        mCallback->OnDocNavStart(request, name);
>+      }
>     }
> 
>     mRequests.RemoveObjectAt(0);
>     mTimers.RemoveObjectAt(0);
>   }
>   return NS_OK;
> }
>Index: browser/components/safebrowsing/src/nsDocNavStartProgressListener.h
>===================================================================
>--- browser/components/safebrowsing/src/nsDocNavStartProgressListener.h	old
>+++ browser/components/safebrowsing/src/nsDocNavStartProgressListener.h	new
>@@ -66,9 +66,12 @@ protected:
>   nsCOMPtr<nsIDocNavStartProgressCallback> mCallback;
> 
>   // queue of pending requests; should we use nsDeque instead?
>   nsCOMArray<nsIRequest> mRequests;
>   nsCOMArray<nsITimer> mTimers;
> 
>   nsresult AttachListeners();
>   nsresult DetachListeners();
>+
>+  // Return true if we don't want to check the request url.
>+  PRBool IsSpurious(nsIRequest* aReq);
> };

Comment 13

[Tony Chang (Google)]

(In reply to comment #12)
> (From update of attachment 226609 [details] [diff] [review] [edit])
> >+  PRBool isSpurious;
> >+
> >+  if (NS_FAILED(uri->SchemeIs("about", &isSpurious)))
> >+    return PR_FALSE;
> >+  if (isSpurious)
> >+    return isSpurious;
> >+
> >+  if (NS_FAILED(uri->SchemeIs("chrome", &isSpurious)))
> >+    return PR_FALSE;
> >+  if (isSpurious)
> >+    return isSpurious;
> >+
> >+  if (NS_FAILED(uri->SchemeIs("file", &isSpurious)))
> >+    return PR_FALSE;
> >+  return isSpurious;
> >+}
> 
> I don't understand the semantics of NS_FAILED here.  Don't we need to check for
> about, chrome, and file before returning false?

ShemeIs will return an error code if there's an error while calling the method (kind of like throwing an exception in JS).  If we get an exception, it's probably better to just return false so we check the URI.

Comment 14

[Tony Chang (Google)]

Attachment: v3: -jar, -javascript, +about

Here's a cleaner and easier to read version of the patch.

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 227170 [details] [diff] [review]
v3: -jar, -javascript, +about

Looks fine.  Thanks for the cleanup.

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 227170 [details] [diff] [review]
v3: -jar, -javascript, +about

>@@ -95,16 +96,49 @@ nsDocNavStartProgressListener::DetachLis
>+nsresult
>+nsDocNavStartProgressListener::IsSpurious(nsIRequest* aReq, PRBool* isSpurious)
>+{
>+  nsCOMPtr<nsIChannel> channel;
>+  nsresult rv;
>+  channel = do_QueryInterface(aReq, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIURI> uri;
>+  rv = channel->GetURI(getter_AddRefs(uri));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = uri->SchemeIs("about", isSpurious);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (*isSpurious)
>+    return NS_OK;
>+
>+  rv = uri->SchemeIs("chrome", isSpurious);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (*isSpurious)
>+    return NS_OK;
>+
>+  rv = uri->SchemeIs("file", isSpurious);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

Probably more straightforward to just call GetScheme and do some Equals() comparisons.  sr=me with that change.

Comment 17

[Tony Chang (Google)]

Attachment: v4: -jar, -javascript, +about

incorporating Brian's changes and landing on trunk

Comment 18

[Darin Fisher]

I don't understand how this fixes the bug.  Does isEvilURL strip off jar: prefixes?

Comment 19

[Tony Chang (Google)]

(In reply to comment #18)
> I don't understand how this fixes the bug.  Does isEvilURL strip off jar:
> prefixes?

No, you would have to add a jar: url into the blacklist.  E.g., you could add jar:http://chatzilla.hacksrus.com/xpi/chatzilla-0.9.73.xpi!/install.rdf to the black list to avoid being phished by that file.

Comment 20

[Tony Chang (Google)]

on branch

Comment 21

[Tony Chang (Google)]

backed out of branch due to memory leak

Comment 22

[Tony Chang (Google)]

Attachment: bonecho: -jar, -javascript, +about (fixes memory leaks)

Comment 23

[Tony Chang (Google)]

on branch