User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/525.13 (KHTML, like Gecko) Version/3.1 Safari/525.13 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9) Gecko/2008052906 Firefox/3.0 Currently, it appears that only the URL of the top-level page, and perhaps iframes(?) are checked against the SafeBrowsing database. To get the full utility of the service, all resources (e.g. especially scripts on a page, flash files, etc) should be checked against the SafeBrowsing lists. This will generate more lookup requests, so it might be necessary to implement something in-memory (e.g. a bloom filter to catch the first level requests, going to the database only if necessary), but we see many many new compromises where someone injects a bunch of script tags that point to things already on the malware list. If all the resources on the page were checked, a user would get much better protection than they're currently getting. Reproducible: Always Steps to Reproduce: Go to http://www.ianfette.com/jstest.html (This pulls in a harmless file from ianfette.org that just throws up a javascript alert. However, ianfette.org is on the blacklist, were this a real attack the user could be in trouble.) Actual Results: Get an alert box Expected Results: Should get an interstitial telling you that the page contains resources from ianfette.org which is a known bad site, and that the page is not safe to view.

(Hoping this can be fixed for 3.1 if not before)

This could effect performance quite a bit.

(In reply to comment #2) > This could effect performance quite a bit. ...and/or memory usage. And, of course, users' privacy.

this problem could cause a huge memory usage. I hope this should be fixed maybe in the next version.

There's no reason that it should cause any significant increase in memory usage. In fact, doing it right would actually speed things up. All you need to do is keep a bloom filter in memory, and do lookups against the bloom filter. If you get a hit, you fall back to a database check, if you don't get a hit then obviously you're fine.

Dave, any work here? Do you think this is something we should take in 1.9.0.x or just wait for 1.9.1?

Not really wanted for 1.9.0.x, but this is already marked blocking for 1.9.1. This probably needs a priority to get on queries...

Attached patch runs the classifier on script and style tags. A few notes: * This adds the ability to handle redirects with nsChannelClassifier, for things (like the script and style loaders) that don't install their own redirect handler. * With the patch in 453723, we cache clean host/fragment lookups. On a clean domain (one with no malware marked at all), we won't need to actually send anything to the classifier thread at all. For loads on polluted domains, we'll still only be doing the expensive db lookup once per domain. Future checks on that domain will just check against an in-memory list of malware on that domain. So this shouldn't have any significant performance impact (waiting on a try server build to confirm that).

why do you not want to install a redirect listener everywhere? For example for nsObjectLoadingContent? Or does that call into the classifier on each redirect manually? If so, should you replace that mechanism with this new one?

The classifier can (with this patch) install its own redirect listener. We could change nsObjectLoadingContent to use that if we wanted...

It sounds like it'd be reducing the amount of code if we always used the classifier redirector, so I think ultimately we should do that. If we do that for the 1.9.1 branch or not doesn't matter much IMHO, but do file a separate bug if it'd reduce code. Patch looks good though. Unfortunately the Workers code has added another codesite, feel free to fix that as a separate patch though. http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerScriptLoader.cpp#517

(In reply to comment #8) > Created an attachment (id=345776) [details] > classify scripts and css What is the reason for tracing (i.e. "classifying") .css files? Could you provide an example of "phishing" / "malware" .css file?

{-moz-binding: /*link to executable bad stuff*/} for one. Also, if there's an exploit in an image format it could be linked in from CSS. And then just general spoofing since CSS can totally remake the page.

Any reason why this hasn't landed? We'll want this for Beta 3.

Err, sorry; landed this sunday night and there were test failures. Looking in to those today.

Oops, ned to not try to classify css if AsyncOpen() fails.

actually probably better to just move the error handling for the asyncopen above the classification.

mozilla-central: http://hg.mozilla.org/mozilla-central/rev/44890ee1d15f 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b4d7f11fefc Filed bug 473344 about using nsIChannelClassifier's redirect handling in nsObjectLoadingContent. Filed bug 473345 for checking dom workers.

Backed out, this was causing some leaks on browser chrome tests: - | runtests-leaks | leaked 609 instances of nsCStringKey with size 16 bytes each (9744 bytes total) + | runtests-leaks | leaked 623 instances of nsCStringKey with size 16 bytes each (9968 bytes total) - | runtests-leaks | leaked 609 instances of nsHashKey with size 4 bytes each (2436 bytes total) - | runtests-leaks | leaked 88 instances of nsHashtable with size 44 bytes each (3872 bytes total) + | runtests-leaks | leaked 623 instances of nsHashKey with size 4 bytes each (2492 bytes total) + | runtests-leaks | leaked 90 instances of nsHashtable with size 44 bytes each (3960 bytes total) - | runtests-leaks | leaked 86 instances of nsPrincipal with size 72 bytes each (6192 bytes total) + | runtests-leaks | leaked 88 instances of nsPrincipal with size 72 bytes each (6336 bytes total) - | runtests-leaks | leaked 87 instances of nsStandardURL with size 172 bytes each (14964 bytes total) - | runtests-leaks | leaked 498 instances of nsStringBuffer with size 8 bytes each (3984 bytes total) + | runtests-leaks | leaked 89 instances of nsStandardURL with size 172 bytes each (15308 bytes total) + | runtests-leaks | leaked 504 instances of nsStringBuffer with size 8 bytes each (4032 bytes total) - | runtests-leaks | leaked 91 instances of nsTArray_base with size 4 bytes each (364 bytes total) + | runtests-leaks | leaked 93 instances of nsTArray_base with size 4 bytes each (372 bytes total)

It looks like this is a side-effect of adding to server-locations.txt. I'd say we should probably just up the threshold on the tinderboxen and reland.

Would be great if you could land that separately, so we know for sure that the main patch doesn't leak

You mean land the patch without tests first? (we need the server-location.txt change for the unit tests)

I mean just land the server-location.txt change first and watch leaks go up. Then land the rest of the patch and make sure that that doesn't affect leak stats.

OK, so every server-locations.txt entry marked "privileged" has a principal stored in the nsScriptSecurityManager. The browser chrome tests leak nsScriptSecurityManager, so any new privileged entry shows up as a leaked principal/uri. We don't really need malware.com to be privileged, so I'll just reland this patch without that tomorrow.

http://hg.mozilla.org/mozilla-central/rev/95f52854608a http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d4858eaeb459

Is there a reason for the mNotificationCallbacks null-check in OnChannelRedirect in this patch?

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre ID:20090511031307 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090511 Shiretoko/3.5b5pre ID:20090511031352