441359 - Check all resources on page against SafeBrowsing database

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Ian Fette (Google)]

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.

Comment 1

[Ian Fette (Google)]

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

Comment 2

[Amarjeet Singh Rai]

This could effect performance quite a bit.

Comment 3

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #2)
> This could effect performance quite a bit.

...and/or memory usage. And, of course, users' privacy.

Comment 4

[Nightrider]

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

Comment 5

[Ian Fette (Google)]

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. 

Comment 6

[Samuel Sidler (old account; do not CC)]

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?

Comment 7

[Samuel Sidler (old account; do not CC)]

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...

Comment 8

[Dave Camp (:dcamp)]

Attachment: classify scripts and css

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).

Comment 9

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

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?

Comment 10

[Dave Camp (:dcamp)]

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

Comment 11

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

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

Comment 12

[Bartłomiej Brzozowiec (BartZilla)]

(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?

Comment 13

[Daniel Veditz [:dveditz]]

{-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.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 15

[Dave Camp (:dcamp)]

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

Comment 16

[Dave Camp (:dcamp)]

Attachment: followup

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

Comment 17

[Dave Camp (:dcamp)]

Attachment: better followup

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

Comment 18

[Dave Camp (:dcamp)]

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.

Comment 19

[Dave Camp (:dcamp)]

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)

Comment 20

[Dave Camp (:dcamp)]

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.

Comment 21

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

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

Comment 22

[Dave Camp (:dcamp)]

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

Comment 23

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

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.

Comment 24

[Dave Camp (:dcamp)]

Attachment: updated patch

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.

Comment 25

[Dave Camp (:dcamp)]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Aakash Desai [:aakashd]]

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