402611 - deal with changes to the safebrowsing v2 protocol

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dave Camp (:dcamp)]

Google is considering some as-yet-unspecified changes to the safebrowsing protocol.  This is a placeholder bug for the work that will need to be done to implement these changes.

Comment 1

[Mike Connor [:mconnor]]

wanted so we don't lose track, will block on this if we actually start moving forward on these changes.

Comment 2

[Dave Camp (:dcamp)]

We are going forward with these changes, this should be blocking.

Comment 3

[Dave Camp (:dcamp)]

Attachment: Part 1

This is the first part of the changes needed to handle the new protocol.  Sorry for the big patch, all the changes were kinda interdependent.

This makes a few high level changes:
a) We can store either a partial hash (32 bits) or a complete hash (256 bits) for a given fragment.
b) Subs are now tied to a specific add chunk to which they apply.

These two changes made the nsUrlClassifierEntry struct very difficult to deal with, so this patch also refactors the database to be one entry per (hostkey, fragment) tuple rather than per hostkey.

Another patch will be coming soon to deal with the second part of this change (confirming a partial hash match once we've found it).

Comment 4

[Tony Chang (Google)]

Comment on attachment 298597 [details] [diff] [review]
Part 1

>diff -r bb03a042984f toolkit/components/url-classifier
>+// XXX: probably want to replace KEY_LENGTH throughout the code with
>+// sizeof(nsUrlClassifierDomainHash)
>+#define KEY_LENGTH 4

It looks like you got all but one of these.  Alternately, maybe #define KEY_LENGTH sizeof(nsUrlClassifierDomainHash) if that works as a template value?


>+  FragmentOverlay *FragmentAt(PRUint32 index) {
>+    return reinterpret_cast<FragmentOverlay*>(mFrags + (index * mFragmentSize));
>+  }
>+
>+  PRUint32 mFragmentSize;
>+
>+  PRUint8 *mFrags;

mFrags should probably just be void*.  Also, it should be documented what's going on here (i.e., fragment size varies, so we need a pointer to different memory sizes).

That said, this code feels really messy with all the reinterpret_casts.  I wonder if there's a way to avoid that by using a base class + subclasses for the two hash types and just using an nsTArray of pointers to the base types?  I think this would also avoid some of the if <4>, else <32>, else <64> code blocks (e.g., in SplitShaChunk).

>+  // XXX
>+  nsresult SplitShaChunk(PRUint32 chunkNum,
>+                         nsACString& chunk,
>+                         PRUint32 hostKeySize,
>+                         PRUint32 fragmentSize,
>+                         nsTArray<nsUrlClassifierEntry>& entries);
>+

Please document.


So I'm giving a r+ so we can move forward with the new protocol.  But it would be nice to refactor the code to avoid the reinterpret_casts.

Comment 5

[Tony Chang (Google)]

Also, is it possible to write some tests for this?

Comment 6

[Dave Camp (:dcamp)]

Attachment: Part 1, for real

Oops, that last patch was the aborted attempt at the overly-complicated extension of the entry class that I mentioned I was avoiding in comment #1 :)

Here's the real patch.

There are some updates to the unit tests in this one, and there are new unit tests for the partial hash stuff coming in part 2 which I should have attached here sometime today.

Comment 7

[Tony Chang (Google)]

Comment on attachment 298745 [details] [diff] [review]
Part 1, for real

Ah, this is a lot better :)

Looks good with one small nit:

>+  const PRBool StartsWith(const nsUrlClassifierHash<PARTIAL_LENGTH>& hash) const {
>+    return memcmp(buf, hash.buf, PARTIAL_LENGTH) == 0;
>   }
> };

Maybe another NS_ASSERTION here to make sure PARTIAL_LENGTH is less than sHashSize?  I realize this isn't possible, but might be nice to assert for future changes.

Comment 8

[Dave Camp (:dcamp)]

Attachment: Part 2

Not the originally-intended part 2, but this fixes some bugs that I found once I got a real server to test against:
a) Redirected chunks don't specify their own table, so we need to save the intended table along with the URI.
b) Bare IP addresses weren't handled correctly.
c) A few more parsing/formatting issues.

Comment 9

[Dave Camp (:dcamp)]

Attachment: Part 3

And the last part for this bug.

This adds a "Completer" object (kind of an awkward name, any suggestions?) that takes care of the gethash request for partial matches.

This required some shuffling around of the lookup code, as the worker needs to pass a lot more data back to the main thread for completion.

I'll be filing a followup bug to cache the results of the gethash request.

Comment 10

[Dave Camp (:dcamp)]

Attachment: http gethash tests

Attaching some unit tests for the http completer.  These aren't ready to commit (they require changes to httpd.js that I'm not happy with yet), but I'm attaching them here so they don't get lost.

Comment 11

[Tony Chang (Google)]

Comment on attachment 299322 [details] [diff] [review]
Part 3

Re name: I don't know, maybe UrlClassifierHashFinisher or UrlClassifierHashCompleter?

Can you merge nsIUrlClassifierHttpCompleter and nsIUrlClassifierCompleter into a single interface?  They seem only useful together and only used together.

What is the request context used for?  I see it passed around, but I don't see it used anywhere.

Thanks for all the tests!

>diff -r 5d428345885c toolkit/components/url-classifier/src/nsUrlClassifierHttpCompleter.cpp
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/toolkit/components/url-classifier/src/nsUrlClassifierHttpCompleter.cpp	Fri Jan 25 15:52:38 2008 -0800
>+  }
>+
>+  aRequestBody.AppendInt(4);

Isn't this a const from a different file? Can it be moved into a class static int?

Comment 12

[Dave Camp (:dcamp)]

Attachment: Part 3 v2

(In reply to comment #11)
> Re name: I don't know, maybe UrlClassifierHashFinisher or
> UrlClassifierHashCompleter?

I liked HashCompleter, and used that everywhere.

> Can you merge nsIUrlClassifierHttpCompleter and nsIUrlClassifierCompleter into
> a single interface?  They seem only useful together and only used together.

Done.

> What is the request context used for?  I see it passed around, but I don't see
> it used anywhere.

It was originally going to be used to associate the completion request with the lookup result, but I ended up changing that.  Removed that.

> >+  aRequestBody.AppendInt(4);
> 
> Isn't this a const from a different file? Can it be moved into a class static
> int?

Changed to PARTIAL_LENGTH, which should accurately reflect its contents.

Comment 13

[Tony Chang (Google)]

Comment on attachment 299359 [details] [diff] [review]
Part 3 v2

2 small nits:

>+[scriptable, uuid(1a3c19d9-ccd6-4d1a-a48a-1ab662e56e60)]
>+interface nsIUrlClassifierHashCompleter : nsISupports
>+{
>+  /**
>+   * Request a completed hash.
>+   *
>+   * @param partialHash
>+   *        The 32-bit hash encountered by the url-classifier.
>+   * @param callback
>+   *        An nsIUrlClassifierCompleterCallback instance.
>+   * @param ctx
>+   *        An opaque object reference that will be passed to the callback.
>+   */

Remove ctx in the docstring.

>+DummyCompleter.prototype =
>+{
>+QueryInterface: function(iid)
>+{
>+  if (!iid.equals(Ci.nsISupports) &&
>+      !iid.equals(Ci.nsIUrlClassifierHashCompleter)) {
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  }
>+  return this;
>+},
>+
>+complete: function(partialHash, cb, ctx)

I think ctx is also unused here.

Comment 14

[Dave Camp (:dcamp)]

Attachment: final patch for checkin

Comment 15

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

Comment on attachment 299471 [details] [diff] [review]
final patch for checkin

a=beltzner

Dave: did you want to blog about this when it lands? Just to get people looking at the right things in nightlies?

Comment 16

[Dave Camp (:dcamp)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.260; previous revision: 1.259
done
Checking in browser/components/safebrowsing/content/globalstore.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/globalstore.js,v  <--  globalstore.js
new revision: 1.17; previous revision: 1.16
done
Checking in browser/components/safebrowsing/content/malware-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/malware-warden.js,v  <--  malware-warden.js
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/safebrowsing/content/phishing-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-warden.js,v  <--  phishing-warden.js
new revision: 1.25; previous revision: 1.24
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.23; previous revision: 1.22
done
Checking in toolkit/components/build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.48; previous revision: 1.47
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.23; previous revision: 1.22
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.13; previous revision: 1.12
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.16; previous revision: 1.15
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierHashCompleter.idl,v
done
Checking in toolkit/components/url-classifier/public/nsIUrlClassifierHashCompleter.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierHashCompleter.idl,v  <--  nsIUrlClassifierHashCompleter.idl
initial revision: 1.1
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.19; previous revision: 1.18
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.18; previous revision: 1.17
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.54; previous revision: 1.53
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.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp,v
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp,v  <--  nsUrlClassifierHashCompleter.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.h,v
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.h,v  <--  nsUrlClassifierHashCompleter.h
initial revision: 1.1
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.16; previous revision: 1.15
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.11; previous revision: 1.10
done
Checking in toolkit/components/url-classifier/tests/unit/head_urlclassifier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js,v  <--  head_urlclassifier.js
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/url-classifier/tests/unit/test_addsub.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js,v  <--  test_addsub.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/url-classifier/tests/unit/test_dbservice.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_dbservice.js,v  <--  test_dbservice.js
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_partial.js,v
done
Checking in toolkit/components/url-classifier/tests/unit/test_partial.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_partial.js,v  <--  test_partial.js
initial revision: 1.1
done
Checking in toolkit/components/url-classifier/tests/unit/test_streamupdater.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_streamupdater.js,v  <--  test_streamupdater.js
new revision: 1.2; previous revision: 1.1
done

Comment 17

[Dave Camp (:dcamp)]

Reverted for bloat test orange.

Comment 18

[Manish Singh]

Comment on attachment 299471 [details] [diff] [review]
final patch for checkin

>diff -u -8 -p -r1.258 firefox.js
>--- browser/app/profile/firefox.js	26 Jan 2008 22:33:24 -0000	1.258
>+++ browser/app/profile/firefox.js	26 Jan 2008 23:01:56 -0000
>@@ -540,16 +540,17 @@ pref("browser.safebrowsing.provider.0.up
> 
> pref("browser.safebrowsing.dataProvider", 0);
> 
> // Does the provider name need to be localizable?
> pref("browser.safebrowsing.provider.0.name", "Google");
> pref("browser.safebrowsing.provider.0.lookupURL", "http://sb.google.com/safebrowsing/lookup?sourceid=firefox-antiphish&features=TrustRank&client={moz:client}&appver={moz:version}&");
> pref("browser.safebrowsing.provider.0.keyURL", "https://sb-ssl.google.com/safebrowsing/getkey?client={moz:client}&");
> pref("browser.safebrowsing.provider.0.reportURL", "http://sb.google.com/safebrowsing/report?");
>++pref("browser.safebrowsing.provider.0.gethashURL", "http://sb.google.com/safebrowsing/gethash?client={moz:client}&appver={moz:version}&pver=2.1");

Don't you have a stray extra "+" there?

Comment 19

[Dave Camp (:dcamp)]

Attachment: Bustage fixes

This patch includes some small bustage fixes.

Of particular note is the NS_IMPL_THREADSAFE_ISUPPORTS - the proxy objects can occasionally ref/unref on the worker thread (though it will properly proxy the final unref), so we need a threadsafe AddRef/Release implementation for the objects we're proxying to the worker thread.

Comment 20

[Damon Sicore (:damons)]

Comment on attachment 299939 [details] [diff] [review]
Bustage fixes

a1.9+=damons

Comment 21

[Dave Camp (:dcamp)]

Re-landed:

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.264; previous revision: 1.263
done
Checking in browser/components/safebrowsing/content/globalstore.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/globalstore.js,v  <--  globalstore.js
new revision: 1.19; previous revision: 1.18
done
Checking in browser/components/safebrowsing/content/malware-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/malware-warden.js,v  <--  malware-warden.js
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/safebrowsing/content/phishing-warden.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/phishing-warden.js,v  <--  phishing-warden.js
new revision: 1.27; previous revision: 1.26
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.25; previous revision: 1.24
done
Checking in toolkit/components/build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.50; previous revision: 1.49
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.25; previous revision: 1.24
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.15; previous revision: 1.14
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.18; previous revision: 1.17
done
Checking in toolkit/components/url-classifier/public/nsIUrlClassifierHashCompleter.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierHashCompleter.idl,v  <--  nsIUrlClassifierHashCompleter.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.21; previous revision: 1.20
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.20; previous revision: 1.19
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.56; previous revision: 1.55
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.11; previous revision: 1.10
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.cpp,v  <--  nsUrlClassifierHashCompleter.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierHashCompleter.h,v  <--  nsUrlClassifierHashCompleter.h
new revision: 1.4; previous revision: 1.3
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.18; previous revision: 1.17
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.13; previous revision: 1.12
done
Checking in toolkit/components/url-classifier/tests/unit/head_urlclassifier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js,v  <--  head_urlclassifier.js
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/components/url-classifier/tests/unit/test_addsub.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js,v  <--  test_addsub.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/url-classifier/tests/unit/test_dbservice.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_dbservice.js,v  <--  test_dbservice.js
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/components/url-classifier/tests/unit/test_partial.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_partial.js,v  <--  test_partial.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/url-classifier/tests/unit/test_streamupdater.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_streamupdater.js,v  <--  test_streamupdater.js
new revision: 1.4; previous revision: 1.3
done