387196 - support the new google safebrowsing protocol

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Camp (:dcamp)]

Attachment: WIP patch

This (WIP) patch updates the url-classifier component to support the new google safebrowsing protocol.

The new protocol specifies a single lookup algorithm for all tables, rather than having per-table logic.  This lookup logic was moved in to the db service from the javascript.

URL canonicalization was moved completely into C++ too.  The DB service can now handle a query from a raw URI, which will be needed for malware blocking.

Comment 1

[Tony Chang (Google)]

Yay for getting rid of a lot of the JS code!

In nsUrlClassifierDBService:

NS_GetSpecialDirectory doesn't appear to be thread safe.  Specifically, it uses nsIProperties which isn't thread safe.
http://mxr.mozilla.org/seamonkey/source/xpcom/io/nsDirectoryServiceUtils.h#53
http://mxr.mozilla.org/seamonkey/source/xpcom/ds/nsProperties.cpp#44

This doesn't seem like a big deal, you should be able to get the directory on the main thread and pass the nsIFile through.

Also, it looks like @mozilla.org/security/hash;1 is not thread safe:
http://mxr.mozilla.org/seamonkey/source/security/manager/ssl/src/nsNSSComponent.cpp#2356

I think you can work around this by using freebl (in security/nss/lib/freebl/) directly.

In nsUrlClassifierDBServiceWorker::ParseChunks, we should probably sanity check first and last otherwise a malicious server (or MITM attack) could cause to you spin for 2^32 cycles and probably run out of memory.  There are a few other places where we read ints, we should probably make sure those also don't cause problems.


Ok, I have to run.  I'll review the rest this weekend.

Comment 2

[Tony Chang (Google)]

Overall, this looks great!

Thanks for porting the unittests!

Just some small additional nits:
It looks like some hashed files got added accidentally.

It looks like this code removes support for remote lookups.  We should file a bug to remove that from the Options.

In nsIUrlClassifierStreamUpdater, would it be cleaner to have requestBody be a param to downloadUpdates()?

Comment 3

[Garrett Casto]

At the moment, you are downloading both hashed and compressed data.  It doesn't seem likely that we will expose the compressed data at this point, so I would switch that to use just hashed data.  Also, we don't actually have a white list right now, though we will add one shortly.  Just FYI.

Comment 4

[Garrett Casto]

Actually, if you are removing enhanced mode, then there is no reason to have a white list, since it is only useful for preventing excessively high traffic to Google's servers.

Comment 5

[Dave Camp (:dcamp)]

(In reply to comment #1)

> NS_GetSpecialDirectory doesn't appear to be thread safe.  Specifically, it uses
> nsIProperties which isn't thread safe.
> This doesn't seem like a big deal, you should be able to get the directory on
> the main thread and pass the nsIFile through.

Worker::Init() is actually called from the main thread (I moved the GetSpecialDirectory() call from OpenDb() which is called from the thread).

> Also, it looks like @mozilla.org/security/hash;1 is not thread safe:
> I think you can work around this by using freebl (in security/nss/lib/freebl/)
> directly.

I borrowed an ugly hack from extensions/metrics to build against freebl's md5 implementation.

> In nsUrlClassifierDBServiceWorker::ParseChunks, we should probably sanity check
> first and last otherwise a malicious server (or MITM attack) could cause to you
> spin for 2^32 cycles and probably run out of memory.  There are a few other
> places where we read ints, we should probably make sure those also don't cause
> problems.

ParseChunks() only parses something we already put in the db, but I added sanity checks.  I also added a sanity check for the chunk size.

(In reply to comment #2)
> It looks like some hashed files got added accidentally.

Not sure what you meant here?

> It looks like this code removes support for remote lookups.  We should file a
> bug to remove that from the Options.

It should affect remote lookups (and it doesn't seem to)

> In nsIUrlClassifierStreamUpdater, would it be cleaner to have requestBody be a
> param to downloadUpdates()?

Indeed.  I changed that.

(In reply to comment #3)
> At the moment, you are downloading both hashed and compressed data.  It doesn't
> seem likely that we will expose the compressed data at this point, so I would
> switch that to use just hashed data.  Also, we don't actually have a white list
> right now, though we will add one shortly.  Just FYI.

I changed it to grab goog-phish-md5 and goog-white-exp.

Comment 6

[Dave Camp (:dcamp)]

Attachment: new version

This version also fixes -md5 tables and uses raw md5sums and a BLOB column in the db rather than asciified md5sums.

Comment 7

[Tony Chang (Google)]

Comment on attachment 271767 [details] [diff] [review]
new version

(In reply to comment #5)
> (In reply to comment #2)
> > It looks like some hashed files got added accidentally.
> 
> Not sure what you meant here?

Oh, I was confused by your added files.  It's fine, the diff is just a little funny.

> > It looks like this code removes support for remote lookups.  We should file a
> > bug to remove that from the Options.
> 
> It shouldn't affect remote lookups (and it doesn't seem to)

Oh, you're right.


It would be nice to have some comments explaining the methods in nsUrlClassifierDBServiceWorker.  It might also be nice for the class to have an overview of the program flow.

Comment 8

[Garrett Casto]

I also don't see how you are respecting the new timing response.  It looks like
requests still happen using kUpdateInterval as the timer value, so everything
happens at 30 minute intervals.  Or am I missing something?

Also, a few minor typos.

Header comments for PROT_ListManager.prototype.makeUpdateRequest_,
PROT_ListManager.prototype.updateSuccess_, and
PROT_ListManager.prototype.updateError_ all have the wrong input parameter name
(tableNames -> tableData, status-> waitForUpdate, status -> result).

nsIUrlClassifierDBService.idl:
spec -> key

nsUrlClassifierDBService::Finish():
Looks like there are a few extra newlines.

Comment 9

[Brian Ryner (not reading)]

I noticed a couple of problems with the canonicalization in nsUrlClassifierUtils.cpp.

First, it needs to collapse runs of consecutive slashes... I can't find any code that does this.

Second, the old code which you ported in CleanupHostname drops characters <= 0x1f and >= 0x7f.  We're not doing this for the new protocol, instead these characters should be percent-escaped.

You can probably eliminate the separate hostname escaping (EscapeHostname).  This was used to format hostnames for lookup in enchash lists, but we don't use this style of escaping with the new protocol.  You should just be able to do one escaping pass on the host + path, using SpecialEncode, after canonicalization is done.

Comment 10

[Tony Chang (Google)]

The original patch for collapsing slashes is on bug 367538.

Comment 11

[Marria Nazif]

Just a reminder that we should add a sanity check on parsing the next time to ping the server.  In case there is an error in the value sent, we should put the minimum next ping time at 1 minute.

Comment 12

[Dave Camp (:dcamp)]

Attachment: bigger better

This version changes some things to get better performance:

* Entries in the database are per-domain (this is explained better in the comments) rather than per-simplified-regexp.
* Lookups can happen during a long-running update (through some fairly ugly HandlePendingLookups() scattered through the update code)

And there are some other changes that happened along the way:

* the md5sums were changed to truncated sha256 hashes to match the new server
* The server protocol was changed to give up hostnames first.

Comment 13

[Tony Chang (Google)]

Comment on attachment 273522 [details] [diff] [review]
bigger better

GetKeyForURI looks much cleaner.

For the queue lock, use nsAutoLock.h rather than having to remember to release the lock explicitly (may not work in HandlePendingLookups).  Alternately, it would probably have been cleaner to just re-queue events, but the lock seems safe here.

Do we need to call HandlePendingLookups so frequently?  I'm a bit worried that acquiring the lock is expensive, but perhaps it's relatively fast.

mPendingHead/mPendingTail: can't we use an nsTArray or nsTPTrArray instead?  Also, don't we need to clean up pending lookups in the destructor?

The rest looks good to me.

Comment 14

[Dave Camp (:dcamp)]

Attachment: updated

uses nsAutoLock and nsTArray for the queue.

I call ProcessPendingLookups() before potentially time-consuming database operations.  Otherwise you'll get all the 

It might be ok to check if there are any pending updates before getting the lock...

Comment 15

[Dave Camp (:dcamp)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.188; previous revision: 1.187
done
Checking in browser/components/safebrowsing/content/list-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/list-warden.js,v  <--  list-warden.js
new revision: 1.14; previous revision: 1.13
done
Checking in browser/components/safebrowsing/content/sb-loader.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/sb-loader.js,v  <--  sb-loader.js
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/components/build/Makefile.in;
/cvsroot/mozilla/toolkit/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.50; previous revision: 1.49
done
Removing toolkit/components/url-classifier/content/enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: delete; previous revision: 1.15
done
Checking in toolkit/components/url-classifier/content/listmanager.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/listmanager.js,v  <--  listmanager.js
new revision: 1.19; previous revision: 1.18
done
Removing toolkit/components/url-classifier/content/multi-querier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/multi-querier.js,v  <--  multi-querier.js
new revision: delete; previous revision: 1.4
done
Removing toolkit/components/url-classifier/content/trtable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/trtable.js,v  <--  trtable.js
new revision: delete; previous revision: 1.14
done
Removing toolkit/components/url-classifier/content/wireformat.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/wireformat.js,v  <--  wireformat.js
new revision: delete; previous revision: 1.7
done
Checking in toolkit/components/url-classifier/content/moz/alarm.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/moz/alarm.js,v  <--  alarm.js
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/url-classifier/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/url-classifier/public/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl,v  <--  nsIUrlClassifierDBService.idl
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/components/url-classifier/public/nsIUrlClassifierStreamUpdater.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierStreamUpdater.idl,v  <--  nsIUrlClassifierStreamUpdater.idl
new revision: 1.5; previous revision: 1.4
done
Removing toolkit/components/url-classifier/public/nsIUrlClassifierTable.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierTable.idl,v  <--  nsIUrlClassifierTable.idl
new revision: delete; previous revision: 1.4
done
Checking in toolkit/components/url-classifier/public/nsIUrlClassifierUtils.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierUtils.idl,v  <--  nsIUrlClassifierUtils.idl
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/url-classifier/public/nsIUrlListManager.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlListManager.idl,v  <--  nsIUrlListManager.idl
new revision: 1.16; previous revision: 1.15
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.14; previous revision: 1.13
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.24; previous revision: 1.23
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierListManager.js;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierListManager.js,v  <--  nsUrlClassifierListManager.js
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp,v  <--  nsUrlClassifierStreamUpdater.cpp
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h,v  <--  nsUrlClassifierStreamUpdater.h
new revision: 1.6; previous revision: 1.5
done
Removing toolkit/components/url-classifier/src/nsUrlClassifierTable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierTable.js,v  <--  nsUrlClassifierTable.js
new revision: delete; previous revision: 1.7
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp,v  <--  nsUrlClassifierUtils.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierUtils.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.h,v  <--  nsUrlClassifierUtils.h
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/src/nssstubs.c,v
done
Checking in toolkit/components/url-classifier/src/nssstubs.c;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nssstubs.c,v  <--  nssstubs.c
initial revision: 1.1
done
Checking in toolkit/components/url-classifier/tests/Makefile.in;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/components/url-classifier/tests/TestUrlClassifierUtils.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/TestUrlClassifierUtils.cpp,v  <--  TestUrlClassifierUtils.cpp
new revision: 1.5; previous revision: 1.4
done
Removing toolkit/components/url-classifier/tests/jar.mn;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/jar.mn,v  <--  jar.mn
new revision: delete; previous revision: 1.2
done
Removing toolkit/components/url-classifier/tests/test_enchash-decrypter.xhtml;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/test_enchash-decrypter.xhtml,v  <--  test_enchash-decrypter.xhtml
new revision: delete; previous revision: 1.2
done
Removing toolkit/components/url-classifier/tests/unittests.xul;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unittests.xul,v  <--  unittests.xul
new revision: delete; previous revision: 1.3
done

RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js,v
done
Checking in head_urlclassifier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js,v  <--  head_urlclassifier.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js,v
done
Checking in tail_urlclassifier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js,v  <--  tail_urlclassifier.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_dbservice.js,v
done
Checking in test_dbservice.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_dbservice.js,v  <--  test_dbservice.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_urikeys.js,v
done
Checking in test_urikeys.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_urikeys.js,v  <--  test_urikeys.js
initial revision: 1.1

Comment 16

[Dave Camp (:dcamp)]

backed out due to failed assertions on shutdown and bad codesize bloat due to pulling in sha512.c

Comment 17

[Dave Camp (:dcamp)]

Attachment: some needed fixes

a) removes extra commas from some enums
b) nulls out mozStorageStatements so that they're destroyed on the right thread
c) uses nsICryptoHash rather than pulling in sha512.c
A few of us on irc took a quick glance at nsCryptoHash and it looks like it's safe to use from a thread as long as you don't try to use it from multiple threads.

Comment 18

[Dave Camp (:dcamp)]

relanded with fixes.

Comment 19

[Michael Wu [:mwu]]

I'm getting a crash in this code. Here's part of the stack trace:

#3  <signal handler called>
#4  nsACString_internal::CharAt (this=0x2aacc1aef840, i=4294967295) at ../../../../dist/include/string/nsTSubstring.h:196
#5  0x00002aaaac55ba09 in nsACString_internal::operator[] (this=0x2aacc1aef840, i=0) at ../../../../dist/include/string/nsTSubstring.h:202
#6  0x00002aaaac55fd16 in nsUrlClassifierUtils::CleanupHostname (this=<value optimized out>, hostname=@0x7fffee1ee950, _retval=@0x7fffee1ee8f0) at /home/mikew/work/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp:228
#7  0x00002aaaac5604d5 in nsUrlClassifierUtils::CanonicalizeHostname (this=0x2640c80, hostname=@0x7fffee1eeae0, _retval=@0x7fffee1eebc0) at /home/mikew/work/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp:173
#8  0x00002aaaac560620 in nsUrlClassifierUtils::GetKeyForURI (this=0x2640c80, uri=0x1db5d00, _retval=@0x7fffee1eebc0) at /home/mikew/work/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierUtils.cpp:133
#9  0x00002aaaac55756c in nsUrlClassifierDBService::Lookup (this=0x2607ff0, spec=<value optimized out>, c=0x20292b0, needsProxy=1) at /home/mikew/work/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:2134

Comment 20

[Dave Camp (:dcamp)]

Attachment: fix for empty or all-dot hostnames

fix for empty or all-dot hostnames.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Errors encountered running the unit test in a debug build

When I run the unit test in a debug build, I get assertions about using the observer service off of the main thread.  Assertions are fatal in xpcshell unit tests, so this causes them to fail.  (Unfortunately none of our tinderboxen that run unit tests are debug builds, so they didn't catch this.)  I've attached the log file of the test, although I'm not sure it provides any great insight.  The test failed at the command line with "Abort trap", so it's definitely the assertion causing the failure.

Comment 22

[Dave Camp (:dcamp)]

That happens if the url-classifier thread creates a cryptohash before psm has been initialized on the main thread.  I opened bug 390324 on this.

Comment 23

[Phil Ringnalda (:philor)]

Since this seems to have killed nsUrlClassifierTable.js by removing it from EXTRA_PP_COMPONENTS, shouldn't it also be CVS removed, or at least removed from all the various packages-static files where it's currently spewing "Warning: package error or possible missing or unnecessary file" in build logs?