384941 - let malware checking block page loads

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Camp (:dcamp)]

We'd like to be able to block page loads for malware URIs.

Comment 1

[Jesse Ruderman]

How does this bug differ from bug 380932?

Comment 2

[Gábor Stefanik]

Probably that only blocks the display of pages, not loading, so it might be too late when the error page is triggered.

Comment 3

[Dave Camp (:dcamp)]

Attachment: first stab

Comment 4

[Boris Zbarsky [:bzbarsky]]

There are various nits I have here, but the most important one is that docshell and uriloader shouldn't be depending on url-classifier, imo.  Nor should we need a lot of the MOZ_URL_CLASSIFIER ifdefs in that code (if any).

On other words, the IDL and contract involved should live in docshell.  url-classifier should provide a component implementing that contract and interface.  Failure to get that component (e.g. if MOZ_URL_CLASSIFIER is not defined) will, in an obvious way, lead to no phishing detection happening.

In other words, we should be able to change our malware-detection impl without having to change anything at all about docshell.  As should any embedder.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and more seriously, I can't get to this review by the M7 freeze (which is today).

Comment 6

[Dave Camp (:dcamp)]

Attachment: add an nsIURIClassifier interface to uriloader

Comment 7

[Dave Camp (:dcamp)]

Attachment: v3

Comment 8

[Jesse Ruderman]

Does this block things like iframe/script/stylesheet loads or only top-level page loads?  If an AdSense server gets hacked, we really want to be able to block script loads; we can't blacklist every site that uses AdSense.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274648 [details] [diff] [review]
v3

>+++ b/browser/locales/en-US/chrome/overrides/
appstrings.properties	Tue Jul 31 10:46:15 2007 -0700

This will need ui-review and all.

>+++ b/docshell/base/nsDocShell.cpp	Tue Jul 31 10:46:15 2007 -0700
>+nsDocShell::GetCheckMalware(PRBool *aCheckMalware)

Is there a usecase for controlling this on a per-docshell basis?  If not, I would remove all this stuff and just observe the pref...

>+nsDocShell::SetCheckMalware(PRBool aCheckMalware)
>+{
>+    // If mUseErrorPages is set explicitly, stop observing the pref.
>+    if (mObserveErrorPages) {

There seem to be some copy/paste issues here.  But again, can we just remove this code?

>@@ -2835,16 +2860,17 @@ nsDocShell::DisplayLoadError(nsresult aE

Please have biesi look over the error page changes.  The code looks ok, but he should do the design review.

> nsDocShell::LoadErrorPage(nsIURI *aURI, const PRUnichar 
>+    if (escapedCSSClass[0]) {

What if escapedCSSClass is null?

>+        errorPageUrl.AppendASCII("?c=");
>+        errorPageUrl.AppendASCII(escapedCSSClass);
>+        errorPageUrl.AppendASCII("&e=");
>+    } else {
>+        errorPageUrl.AppendASCII("?e=");

Can we just put c= after e= and not need this conditional stuff?

>@@ -3435,20 +3482,31 @@ nsDocShell::Create()
>+    if (NS_SUCCEEDED(rv)) {
>+        if (mObserveErrorPages)

I know the code used to do that, but it's wrong.  Even if the pref isn't set yet (so rv is failure) you still want to observe it.  Same for your new pref.

>@@ -3469,21 +3527,26 @@ nsDocShell::Destroy()
>+            prefs->RemoveObserver("browser.safebrowsing.malware.enabled", this);

Might be nice to have a #define for this string so we don't have it in more than one spot in this code.

> nsDocShell::Observe(nsISupports *aSubject, const char *aTopic,

>+        if (mObserveErrorPages &&
>+            !nsCRT::strcmp(aData,
>+                           NS_LITERAL_STRING("browser.xul.error_pages.enabled").get())) {
>+            PRBool tmpbool;
>+            rv = prefs->GetBoolPref("browser.xul.error_pages.enabled",
>+                                    &tmpbool);
>+            if (NS_SUCCEEDED(rv))
>+                mUseErrorPages = tmpbool;

This code is also wrong.  If the get failed we should fall back on our default value (set in the ctor), not just leave the boolean as-is.  The way the code is written, removing the pref is ignored.

Same thing for your new pref.

I'd also use 
NS_LITERAL_STRING("browser.xul.error_pages.enabled").Equals(aData) myself, but either way.

>+++ b/docshell/base/nsIDocShell.idl	Tue Jul 31 10:46:15 
>+  attribute boolean checkMalware;

Please put the new property at the end of the interface.  But again, I question the need to add it at all.

>+++ b/dom/locales/en-US/chrome/appstrings.properties	Tue 
>+malwareBlocked=The website at %S has been reported as an attack site.
>\ No newline at end of file

That's bad.  Please to put in the newline. :)

The toolkit changes look ok to me, but probably need review from the owner of that code.

>+++ b/uriloader/base/Makefile.in	Tue Jul 31 10:46:15 > SDK_XPIDLSRCS = 						\
> 				nsIURIContentListener.idl 	\
>+				nsIURIClassifier.idl		\

Absolutely not.  SDK_XPIDLSRCS is for frozen interfaces.  This isn't frozen so it should live in XPIDLSRCS.

>+++ b/uriloader/base/nsIURIClassifier.idl	Tue Jul 31 

This interface needs a lot more documentation.  Things like what the point is, what the callback interface is all about, what the caller can expect in terms of sync/async behavior, which thread async calls (if any) will happen on, whether there's any way to associate the handleEvent call with the original classify call, etc.

>+%{C++
>+
>+#define NS_ERROR_MALWARE_URI   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_URILOADER, 3)
>+
>+#define NS_URICLASSIFIERSERVICE_CONTRACTID "@mozilla.org/uriclassifierservice"
>+
>+%}

This doesn't belong in an IDL file (existing examples notwithstanding).  The contract should be in nsDocShellCID.h.  The error...  it's not really public outside the docshell module, right?  You could even stick it in some header that all the relevant files include directly if you wanted.  We should really have an nsDocShellErrors file, but that can be a followup bug.

>+++ b/uriloader/base/nsIURILoader.idl	Tue Jul 31 10:46:15 2007 -0700
>+   * If this flag is set and the url-classifier service is built and available,

Why not just "is available"?  Not built is a special case of not available.

>+   * the load will fail with an error if the URI was decided to be malware */
>+  const unsigned long CHECK_MALWARE        = 1 << 2;

With which error?  I think it should be just fine to say what the error will be.  Also, what does "the load will fail" mean?  openURIWithFlags() will throw?  Or the channel's OnStartRequest will have that error status?  Or something else?

>+++ b/uriloader/base/nsURIClassifierStreamListener.cpp	Tue 

>+#if defined(PR_LOGGING)

So the logging will only be available in debug builds, right?

>+  if (!gURIClassifierStreamListenerLog)
>+    gURIClassifierStreamListenerLog =
>+      PR_NewLogModule("URIClassifierStreamListener");
>+#endif

Why not just init the static pointer where it's declared?

>+nsURIClassifierStreamListener::StartQuery(nsIURI* aURI)
>+  nsCOMPtr<nsIURIClassifier> uriClassifier =
>+    do_GetService(NS_URICLASSIFIERSERVICE_CONTRACTID, &rv);
>+  if (NS_FAILED(rv)) return rv;

OK.  So no service means the method throws, right?

>+nsURIClassifierStreamListener::HandleEvent(PRUint32 aValue)

For what it's worth, you might want to log the |this| pointer throughout this code so that you can match up query requests and responses in the logs.

>+++ b/uriloader/base/nsURIClassifierStreamListener.h	Tue 
>+  nsURIClassifierStreamListener();
...
>+  void SetChildListener(nsIStreamListener *aChildListener)

Why not just make that a constructor arg?  Can you think of a reason to change mChildListener?

>+++ b/uriloader/base/nsURILoader.cpp	Tue Jul 31 10:46:15 

>+  nsRefPtr<nsURIClassifierStreamListener> classifier =
>+    new nsURIClassifierStreamListener();
>+  if (!classifier) return;

Shouldn't we bail out of the load in this case instead of silently disabling malware checking?

>+  if (NS_FAILED(classifier->StartQuery(uri))) {
>+    return;

Same here, for all cases except when the classifier service is not available.

>+  NS_ADDREF(*aListener = static_cast<nsIStreamListener*>(classifier.get()));

I don't think you need either the .get() or the cast.

>+++ b/uriloader/base/nsURILoader.h	Tue Jul 31 10:46:15 
>+  void CheckMalware(nsIChannel *aChannel,
>+                    nsIStreamListener *aChildListener,
>+                    nsIStreamListener **aListener);

Document the behavior, please (once we sort out what it should be).

This doesn't do malware checking on <object> loads of HTML pages.  Do you plan to address this in a followup?

Comment 10

[Dave Camp (:dcamp)]

Attachment: v4

Comment 11

[Dave Camp (:dcamp)]

(In reply to comment #9)
> Is there a usecase for controlling this on a per-docshell basis?  If not, I
> would remove all this stuff and just observe the pref...

Removed the per-docshell stuff.
 
> >@@ -2835,16 +2860,17 @@ nsDocShell::DisplayLoadError(nsresult aE
> 
> Please have biesi look over the error page changes.  The code looks ok, but he
> should do the design review.

r?'ed biesi.
 
> Can we just put c= after e= and not need this conditional stuff?

I did that;  while I was there, I changed c= to s= in netError.xhtml, because we send c=charset here.
 
> >@@ -3435,20 +3482,31 @@ nsDocShell::Create()
> >+    if (NS_SUCCEEDED(rv)) {
> >+        if (mObserveErrorPages)
> 
> I know the code used to do that, but it's wrong.  Even if the pref isn't set
> yet (so rv is failure) you still want to observe it.  Same for your new pref.

That specific NS_SUCCEEDED() is for the QI to nsIPrefBranch2 (which is needed to add the observers) - I restructured that code to be a bit more obvious there, and fixed up the other pref bits to fall back to the default on failure.

> >@@ -3469,21 +3527,26 @@ nsDocShell::Destroy()
> >+            prefs->RemoveObserver("browser.safebrowsing.malware.enabled", this);
> 
> Might be nice to have a #define for this string so we don't have it in more
> than one spot in this code.

I added a define for that and the use-error-pages pref.


> So the logging will only be available in debug builds, right?

It would probably be useful to have it in release builds, I added that.

> 
> This doesn't do malware checking on <object> loads of HTML pages.  Do you plan
> to address this in a followup?
> 

Hrm, probably should, yeah.

Other comments should be fixed.

-dave

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275041 [details] [diff] [review]
v4

>+++ b/docshell/resources/content/netError.xhtml	Thu >+        // s is optional, if no matchjust return nothing

s/matchjust/match just/

>+++ b/uriloader/base/nsIURIClassifier.idl	Thu Aug 02 

Excellent. Thank you for the documentation!

>+   *        A set of classifications (defined below in nsIURIClassifier) that
>+   *        apply to the URI.

So this will always be a subset of the bits passed to the classify() call?

>+++ b/uriloader/base/nsIURILoader.idl	Thu Aug 02 15:34:24 
>+   * If the URI is on the URI classifier's blacklist, the channel's
>+   * OnStartRequest will be called with an NS_ERROR_MALWARE_URI failure.

Perhaps better to say that the channel will be canceled with that status code?

The rest looks good, with one more nit... if StartQuery fails, it seems like we'll deadlock (because we'll never get a HandleEvent), no?  It would make the most sense to me that if StartQuery returns failure it should store that value in a member, and then cancel the channel in OnStartRequest with that value.  That really only affects the redirect case, so we could store the return value in a member there or something.

Comment 13

[Tony Chang (Google)]

Comment on attachment 275041 [details] [diff] [review]
v4

>+        var matches = url.match(/s\=([^&]+)\&/);
>+        // s is optional, if no matchjust return nothing
>+        if (!matches || matches.length < 2)
>           return "";

Nit: Typo.

>+  if (aFlags & CHECK_MALWARE) {
>+    nsCOMPtr<nsIStreamListener> malwareLoader;
>+    CheckMalware(channel, loader, getter_AddRefs(malwareLoader));
>+    if (malwareLoader) {

Nit: The difference between return value (function completing successfully) and malwareLoader being null (do a regular lookup) took me a while to understand, but I guess it's not a big deal.  I see the documentation in the header file, but perhaps it would be good to also comment the code.  At first glance I thought you were missing a return value check.


r+

Comment 14

[Dave Camp (:dcamp)]

(In reply to comment #12)

> The rest looks good, with one more nit... if StartQuery fails, it seems like
> we'll deadlock (because we'll never get a HandleEvent), no?  It would make the
> most sense to me that if StartQuery returns failure it should store that value
> in a member, and then cancel the channel in OnStartRequest with that value. 
> That really only affects the redirect case, so we could store the return value
> in a member there or something.

CheckMalware() throws away the stream listener if StartQuery() fails.  This should avoid the deadlock.

But maybe it shouldn't be throwing away the listener?  Will throwing StartQuery's error from CheckMalware() up through OpenChannel()/OpenURI() be enough?

(v4 doesn't actually throw the error from CheckMalware(), but I'll change that for the next version)

Comment 15

[Johnathan Nightingale [:johnath]]

Two changes to the error page string(s).  First off, we don't think it's necessary to associate it with Firefox explicitly (despite me doing so in earlier mockups :) - we don't do so with phishing, for instance.  Second, we'd like to include some language that lets users build an intuition as to where this behaviour comes from (i.e. their malware blocking prefs) so recommend changing both malwareBlocked strings to read:

The site at %S has been reported as an attack site and has been blocked based on your security preferences.

This is from discussion with beltzner, so whether you count that as his ui-r or my stealing his flag, it should be good to go.

Comment 16

[Dave Camp (:dcamp)]

Attachment: v5

Updated strings, fixed nits, and fixed the problem with redirects.

Comment 17

[Dave Camp (:dcamp)]

(In reply to comment #14)
> (In reply to comment #12)

> > most sense to me that if StartQuery returns failure it should store that value
> > in a member, and then cancel the channel in OnStartRequest with that value. 
> > That really only affects the redirect case, so we could store the return value
> > in a member there or something.
> 
> CheckMalware() throws away the stream listener if StartQuery() fails.  This
> should avoid the deadlock.

Never mind, I see what you're saying now.  Should be fixed in v5.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275925 [details] [diff] [review]
v5

Yeah, sorry about not being clear.  I did mean the redirect case.  Thank you for sorting it out!

sr=bzbarsky

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 275925 [details] [diff] [review]
v5

docshell/base/nsDocShell.cpp
+    nsAutoString cssClass;

I'd make this an nsCAutoString and avoid the UTF16->UTF8 conversion

+    if (!nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {

change this to plain strcmp while you're touching this code

uriloader/base/nsIURIClassifier.idl
+  void handleEvent(in unsigned long aClassification);

I don't really like that name... how about onClassified? or onClassifyComplete?

+%{C++
+
+%}

please remove that

uriloader/base/nsIURILoader.idl

Can you add the new function directly under openURI, so that related functions are grouped together?

(I was planning on just changing the aIsPreferred argument to a flags argument, but never got around to it...)

uriloader/base/nsURIClassifierStreamListener.cpp
+        *aResult = NS_STATIC_CAST(nsIChannelEventSink *, this);

static_cast<> please

+    monitor.Wait();

As described on http://www.mozilla.org/projects/nspr/reference/html/prmon.html#16436 you have to do this in a loop and check mHaveResult in it

uriloader/base/nsURILoader.cpp
+  // Don't bother checking URIs without a host

Should you get the innermost URI before doing this check? (seems like otherwise, one could use jar: URIs to get around the check)

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

So, that was the review of the code, but I'm not sure I like this interface. URILoader doesn't really care what the classifier is checking... couldn't the interface look something like:

interface nsIURIClassifierCallback {
  void onClassifyComplete(in nsresult errorCode);
}

interface nsIURIClassifier {
  void classify(in nsIURI uri, in nsIURIClassifierCallback callback);
}

and then URI Loader can just use the provided error code for cancelling the channel (if it's a failure code). that way, there's no need to have to update uriloader when you want to check for some new thing in addition to phishing and malware.


It would also be nice if multiple classifiers could be registered, but that might be somewhat hard to implement given the asynchronicity.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

I'm also not sure that it's a great idea to hang the UI for potentionally large amounts of time while waiting for the classifier response... perhaps you should Suspend() the channel while waiting, instead of blocking on a monitor?

Comment 22

[Dave Camp (:dcamp)]

Attachment: v7

Using Suspend() was much nicer than the monitor, I changed that.

In the last version the listener wasn't actually getting the OnChannelRedirect.  I talked with biesi on irc and he suggested overriding the channel callbacks during the query.  This version does that.

I switched to the simpler nsIURIClassifer interface biesi proposed.  With the changed interface it might be nice to:
a) Remove the browser.safebrowsing.malware.enabled check from the docshell and always check the URI classifier (if it exists), and
b) Check the malware pref in the uri classifier implementation and just throw a "no check necessary" error if the pref is disabled.

Thoughts?

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

That seems like a good idea

Comment 24

[Dave Camp (:dcamp)]

Attachment: move prefs over to url-classifier

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277007 [details] [diff] [review]
move prefs over to url-classifier

>+++ b/uriloader/base/nsURILoader.cpp
>+nsURILoader::CheckClassifier(nsIChannel *aChannel,
>+  // Don't bother checking chrome URIs or URIs without a host

What's special about "chrome"?  Should't we just bail out on any URI which doesn't have URI_LOADABLE_BY_ANYONE?  Or maybe on any URI which has URI_DANGEROUS_TO_LOAD or URI_IS_UI_RESOURCE or URI_IS_LOCAL_FILE.

I have one concern about the use of suspend()...  Since we're calling OnStartRequest on the listener, we'll perform content dispatch, right?  And call DoContent on the docshell?  And put in place the new content viewer... and _then_ cancel the channel?  Won't that mean that we end up with a zombie document in the docshell?  Is that the behavior we want?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Especially, what happens when error pages are not in use?  It seems to me like depending on how things race we may or may not blow away the previous document...  If error pages are in use we'll always blow it away, of course.

Comment 27

[Boris Zbarsky [:bzbarsky]]

So I talked to biesi about suspend/resume, and it looks like the way to go here is to suspend right after AsyncOpen, if we plan to classify, and resume once the classification is complete.  This way we're guaranteed that by the time OnStartRequest (or for that matter OnChannelRedirect) is called, the original channel has been classified.

Now this doesn't quite work for HTTP yet, but bug 392837 will fix that.

Comment 28

[Dave Camp (:dcamp)]

Hrm, in OnChannelRedirect the new channel hasn't been AyncOpened yet, so we won't be able to Suspend yet...

Comment 29

[Boris Zbarsky [:bzbarsky]]

Hmm...  Will posting an event to suspend guarantee that the event fires before the channel's OnStartRequest?

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

what you could perhaps do is block the redirect and start the new load yourself

Comment 31

[Boris Zbarsky [:bzbarsky]]

You'd need to clone the post-redirect channel, right?

Comment 32

[Dave Camp (:dcamp)]

And wouldn't HandleAsyncRedirect will OnStartRequest/OnStopRequest with the old channel, which isn't really what you'd want?

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

yes, you'd have to clone the post-redirect channel

you could ignore the onStartRequest/onStopRequest calls if they're for the old channel, but maybe that's getting too complicated

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

though if you do it by creating a new instance of the nsURIClassifierStreamListener  and just set some state in the old one to ignore everything, then it shouldn't be too bad.

Comment 35

[Dave Camp (:dcamp)]

Attachment: redoing all the docshell bits

This patch changes the strategy a bit, in light of suspend-after-asyncopen.  We no longer need the stream listener (suspends and resumes can be managed without listening to OnStartRequest).

Putting this in URILoader doesn't quite make sense anymore, because OpenChannel doesn't know when AsyncOpen is going to be called.  We could put it in OpenURI, but then you'd have flags that apply to OpenURI but not OpenChannel, and it'd be kinda weird.

So this version puts the logic up in the docshell.  This also lets us handle redirects without the wrapper callbacks.

I moved the error page pref handling into a separate bug (393074), and moved the openURIWithFlags change into bug 340749.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Hmm...  So moving this from OpenChannel to docshell means that <object> is no longer subject to malware blocking, right?  Is that a problem?

Of course before that <object> would always freeze the UI for the entire duration of the DB lookup, since OpenChannel would get called after the initial response from the server...

Comment 37

[Dave Camp (:dcamp)]

As I understand the uriloader contract, it wouldn't be safe to start the query in OpenChannel anyway, since it's not guaranteed to be AsyncOpened by that point.

Maybe it would make sense to export nsClassifierCallback (or something like it) to make it easy for nsObjectLoadingContent to wrap its own loads with the classifier?

Comment 38

[Dave Camp (:dcamp)]

(and ideally I'd like to do that as a followup so we can get this damned thing in)

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

personally I think that pages that use an <object> containing malware should just be considered malware themselves.

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 277584 [details] [diff] [review]
redoing all the docshell bits

+    if (NS_FAILED(status)) return NS_OK;

please put the return statement on its own line (allows setting breakpoints on it)

+    nsCOMPtr<nsICachingChannel> cachingChannel = do_QueryInterface(channel);
+    PRBool fromCache;
+    if (NS_SUCCEEDED(cachingChannel->IsFromCache(&fromCache)) && fromCache) {

hmm, I don't think you can assume that all channels that get here are caching channels

I'm not sure that it's a good idea to return an error from DoChannelLoad when the channel is actually loading data. Perhaps you should cancel it if CheckClassifier returned an error?

+            PR_LOG(gDocShellLog, PR_LOG_DEBUG,
+                   ("nsClassifierCallback[%p]: cancelling channel %p",
+                    this, mSuspendedChannel.get()));

would be nice to mention the error code too, IMO

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277584 [details] [diff] [review]
redoing all the docshell bits

>+++ b/docshell/base/nsDocShell.h
>+#include "nsRefPtrHashtable.h"

Doesn't seem to be needed.

With that and biesi's comments addressed, r=bzbarsky

Comment 42

[Dave Camp (:dcamp)]

Attachment: comments fixed

Comments fixed.

Asking for approval - there's some risk associated with this patch, but it's necessary for Firefox's malware blocking feature.

Comment 43

[Johnathan Nightingale [:johnath]]

Comment on attachment 279139 [details] [diff] [review]
comments fixed

Just making the earlier string change request official.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 279139 [details] [diff] [review]
comments fixed

a=me, but in general no one is triaging a1.9 flags in Firefox components.  Bugs with core parts in them need to be in a core component.

Also, please get the <object> followup filed; it should probably block 1.9.

Comment 45

[Jesse Ruderman]

Eh?  How are Firefox-only patches supposed to get approval, then?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Firefox-only patches don't need approvals yet.

Comment 47

[Dave Camp (:dcamp)]

Filed bug 394485 for <object> loads.

Comment 48

[Dave Camp (:dcamp)]

Checking in browser/locales/en-US/chrome/overrides/appstrings.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/overrides/appstrings.properties,v  <--  appstrings.properties
new revision: 1.8; previous revision: 1.7
done
Checking in docshell/base/Makefile.in;
/cvsroot/mozilla/docshell/base/Makefile.in,v  <--  Makefile.in
new revision: 1.64; previous revision: 1.63
done
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.850; previous revision: 1.849
done
Checking in docshell/base/nsDocShell.h;
/cvsroot/mozilla/docshell/base/nsDocShell.h,v  <--  nsDocShell.h
new revision: 1.215; previous revision: 1.214
done
RCS file: /cvsroot/mozilla/docshell/base/nsIURIClassifier.idl,v
done
Checking in docshell/base/nsIURIClassifier.idl;
/cvsroot/mozilla/docshell/base/nsIURIClassifier.idl,v  <--  nsIURIClassifier.idl
initial revision: 1.1
done
Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.695; previous revision: 1.694
done
Checking in docshell/build/nsDocShellCID.h;
/cvsroot/mozilla/docshell/build/nsDocShellCID.h,v  <--  nsDocShellCID.h
new revision: 1.4; previous revision: 1.3
done
Checking in docshell/resources/content/netError.xhtml;
/cvsroot/mozilla/docshell/resources/content/netError.xhtml,v  <--  netError.xhtml
new revision: 1.23; previous revision: 1.22
done
Checking in dom/locales/en-US/chrome/appstrings.properties;
/cvsroot/mozilla/dom/locales/en-US/chrome/appstrings.properties,v  <--  appstrings.properties
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in toolkit/components/url-classifier/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/url-classifier/src/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.h,v  <--  nsUrlClassifierDBService.h
new revision: 1.4; previous revision: 1.3
done
Checking in uriloader/base/nsURILoader.h;
/cvsroot/mozilla/uriloader/base/nsURILoader.h,v  <--  nsURILoader.h
new revision: 1.27; previous revision: 1.26
done

Comment 49

[Dave Camp (:dcamp)]

Attachment: only malware check http uris

This was causing some failures on (at least) javascript URIs.  Here's a temporary fix, I'll open a bug to take a better look at which URIs can e checked.

Comment 50

[Dave Camp (:dcamp)]

committed, followup in 394525.

Comment 51

[Steffen Wilberg]

Is there a (hardcoded) testpage like the one for safe browsing?
http://www.mozilla.com/firefox/its-a-trap.html
http://www.google.com/tools/firefox/safebrowsing/phish-o-rama.html

Comment 52

[Dave Camp (:dcamp)]

There is http://www.mozilla.com/firefox/its-an-attack.html

Comment 53

[Reed Loden [:reed]]

(In reply to comment #52)
> There is http://www.mozilla.com/firefox/its-an-attack.html

Mind filing a bug under Websites :: www.mozilla.com to get a page made and a special rewrite added so that that page doesn't redirect?

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #49)
> Created an attachment (id=279187) [details]
> only malware check http uris
> 
> This was causing some failures on (at least) javascript URIs.  Here's a
> temporary fix, I'll open a bug to take a better look at which URIs can e
> checked.

Was this filed?  This seems like a potential problem; somebody could start wrapping their malware in a JAR (and jar: URI) to prevent us from checking malware lists.  (But making sure we check the inner URI of a jar: URI might be more than undoing this.)

Comment 55

[Carsten Book [:Tomcat]]

created a litmus testcase for this -> setting the flag