Last Comment Bug 665930 - Safebrowsing code has minor bugs, protocol violations
: Safebrowsing code has minor bugs, protocol violations
Status: RESOLVED FIXED
[testday-2011-06-24]
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Gian-Carlo Pascutto [:gcp]
:
Mentors:
: 633644 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-21 09:38 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2014-05-27 12:25 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (4.12 KB, patch)
2011-06-21 09:38 PDT, Gian-Carlo Pascutto [:gcp]
no flags Details | Diff | Splinter Review
Patch v2 1/3 Fix for the fragmentation routine (2.31 KB, patch)
2011-06-22 21:32 PDT, Gian-Carlo Pascutto [:gcp]
tony: review+
Details | Diff | Splinter Review
Patch v2 2/3 Add testcase for the fragmentation routine (1.87 KB, patch)
2011-06-22 21:33 PDT, Gian-Carlo Pascutto [:gcp]
tony: review+
Details | Diff | Splinter Review
Patch v2 3/3 Fix for framenting of numerical IPs (1.92 KB, patch)
2011-06-23 00:13 PDT, Gian-Carlo Pascutto [:gcp]
bryner+phishing: review+
Details | Diff | Splinter Review
Patch v2 extra Fix some dbservice testcases (1.43 KB, patch)
2011-06-23 00:33 PDT, Gian-Carlo Pascutto [:gcp]
tony: review+
Details | Diff | Splinter Review
Patch v3 1/3 Fix the fragmentation routine (6.74 KB, patch)
2011-06-23 19:14 PDT, Gian-Carlo Pascutto [:gcp]
tony: review+
Details | Diff | Splinter Review
Patch v3 2/3 Fix the fragmentation of numerical IPs (2.13 KB, patch)
2011-06-23 19:14 PDT, Gian-Carlo Pascutto [:gcp]
tony: review+
Details | Diff | Splinter Review
Patch v3 Fix dbservice testcase typos (1.43 KB, patch)
2011-06-23 19:15 PDT, Gian-Carlo Pascutto [:gcp]
gpascutto: review+
Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2011-06-21 09:38:36 PDT
Created attachment 540775 [details] [diff] [review]
Patch v1

There are some bugs in the Safe Browsing code, leading to minor protocol violations.

1) When generating the list of URL fragments, it's possible for an empty path (top-level domain) to be generated twice. The impact is limited because the duplicate entry will be intercepted by the cache.

2) When hitting a site with a numerical IP hostname in the URL, known to containing an exploit anywhere, the fragmenting code will generate URL's containing truncated versions of the IP. This is unneeded and a violation of the protocol spec.

Patch attached.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2011-06-21 09:45:28 PDT
Comment on attachment 540775 [details] [diff] [review]
Patch v1

>+  // Check an empty path (for whole-domain blacklist entries)
>+  if (!path.Equals(EmptyCString())) {

!path.IsEmpty()
Comment 2 Tony Chang (Google) 2011-06-21 09:48:53 PDT
Is it possible to write a test for this?

(cc ian to find someone on the safe browsing team to actually verify that this matches the protocol)
Comment 3 Gian-Carlo Pascutto [:gcp] 2011-06-21 11:08:24 PDT
>Is it possible to write a test for this?

This is fairly low-level so it probably needs a compiled-code test to test the fragmenting routine. But:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/tests/Makefile.in

has:

# XXX Get this to work in libxul builds.
## simple c++ tests (no xpcom)
#CPP_UNIT_TESTS = \
#	TestUrlClassifierUtils.cpp \
#	$(NULL)

Help?
Comment 4 Mehdi Mulani [:mmm] (I don't check this) 2011-06-21 11:14:01 PDT
*** Bug 633644 has been marked as a duplicate of this bug. ***
Comment 5 Mehdi Mulani [:mmm] (I don't check this) 2011-06-21 11:33:28 PDT
Awesome work Gian-Carlo!

I believe (1) can be tested by making a toy HashCompleter and checking what hashes are asked to be completed when visiting a URL (kind of as described in bug 633644).

Possibly (2) could be tested in the same way.

The basic idea is to add good prefixes and bad prefixes to the DBService and check which are completed for, i.e. which the DBService matches.
Comment 6 Gian-Carlo Pascutto [:gcp] 2011-06-22 21:32:21 PDT
Created attachment 541282 [details] [diff] [review]
Patch v2 1/3 Fix for the fragmentation routine
Comment 7 Gian-Carlo Pascutto [:gcp] 2011-06-22 21:33:05 PDT
Created attachment 541283 [details] [diff] [review]
Patch v2 2/3 Add testcase for the fragmentation routine
Comment 8 Gian-Carlo Pascutto [:gcp] 2011-06-23 00:13:53 PDT
Created attachment 541300 [details] [diff] [review]
Patch v2 3/3 Fix for framenting of numerical IPs
Comment 9 Ian Fette (Google) 2011-06-23 00:16:18 PDT
Thanks for CCing me tony. sending it around internally to see who can take a look.
Comment 10 Gian-Carlo Pascutto [:gcp] 2011-06-23 00:29:58 PDT
Regarding case (1), the comments in bug 633644 mention another failure case that wasn't fixed by the first version of the patch. This is now fixed in v2. They're also correct that those duplicated fragments can end up calling the hashcompleter twice, so we are in fact leaking what URL is being visited, and causing unnecessary load on the service.

I included a testcase for this.

For (2), it's not easy to construct a JS test: before fragmenting, the code first gathers all known prefixes for a given host, but the code doing that correctly handles numerical IP hostnames, so it will only return prefixes for the full IP. 
If the fragmenting code now generates excess fragments by stripping parts of the IP, their 32-bit hash prefix must still match the fragments from the full IP before a hashcompletion is sent out. In other words, you need an actual 32-bit SHA-256 collision to trigger this.
Comment 11 Gian-Carlo Pascutto [:gcp] 2011-06-23 00:33:50 PDT
Created attachment 541301 [details] [diff] [review]
Patch v2 extra Fix some dbservice testcases

The testing code for dbservice most probably has some copy-paste typos and the comments don't correctly reflect what it is doing. This should fix that.
Comment 12 Tony Chang (Google) 2011-06-23 10:36:24 PDT
Comment on attachment 541301 [details] [diff] [review]
Patch v2 extra Fix some dbservice testcases

Review of attachment 541301 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 13 Brian Ryner 2011-06-23 14:48:13 PDT
Comment on attachment 541300 [details] [diff] [review]
Patch v2 3/3 Fix for framenting of numerical IPs

># HG changeset patch
># User Gian-Carlo Pascutto <gpascutto@mozilla.com>
># Parent 98e2a913f76b394cabff150e555de7e82b5a9855
>Bug 665930 - Safe Browsing: Fix fragmenting of numerical IPs.
>
>diff --git a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
>--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
>+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
>@@ -1451,28 +1451,31 @@ nsUrlClassifierDBServiceWorker::GetLooku
>    * a) The exact hostname of the url
>    * b) The 4 hostnames formed by starting with the last 5 components and
>    *    successivly removing the leading component.  The top-level component
>    *    can be skipped.
>    */
>   nsTArray<nsCString> hosts;
>   hosts.AppendElement(host);
> 
>-  host.BeginReading(begin);
>-  host.EndReading(end);
>-  int numComponents = 0;
>-  while (RFindInReadable(NS_LITERAL_CSTRING("."), begin, end) &&
>-         numComponents < MAX_HOST_COMPONENTS) {
>-    // don't bother checking toplevel domains
>-    if (++numComponents >= 2) {
>-      host.EndReading(iter);
>-      hosts.AppendElement(Substring(end, iter));
>+  int numComponents;
>+  if (!IsCanonicalizedIP(host)) {
>+    host.BeginReading(begin);
>+    host.EndReading(end);
>+    numComponents = 0;

nit: There's not really an advantage to reusing numComponents here for the host components and later for the path components, and it invites some confusion.  I'd suggest maybe having a separate hostComponents variable that's scoped inside of this "if", and renaming the later variable to pathComponents.
Comment 14 Brian Ryner 2011-06-23 15:14:42 PDT
Comment on attachment 541283 [details] [diff] [review]
Patch v2 2/3 Add testcase for the fragmentation routine

It would be great if you could add the testcases from section 6.2 of http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec to give us some more coverage.
Comment 15 Tony Chang (Google) 2011-06-23 15:18:36 PDT
Comment on attachment 541282 [details] [diff] [review]
Patch v2 1/3 Fix for the fragmentation routine

Review of attachment 541282 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1489,2 @@
>    path.BeginReading(iter);
>    path.EndReading(end);

Nit: It looks like BeginReading() has an option second param that will fill in the end of the string.  We can remove this extra EndReading() call if we use it.
Comment 16 Tony Chang (Google) 2011-06-23 15:19:31 PDT
Comment on attachment 541283 [details] [diff] [review]
Patch v2 2/3 Add testcase for the fragmentation routine

Review of attachment 541283 [details] [diff] [review]:
-----------------------------------------------------------------

I would land this with patch part 1/3 (keep the code change and test together as a single commit).
Comment 17 Gian-Carlo Pascutto [:gcp] 2011-06-23 19:14:07 PDT
Created attachment 541577 [details] [diff] [review]
Patch v3 1/3 Fix the fragmentation routine
Comment 18 Gian-Carlo Pascutto [:gcp] 2011-06-23 19:14:49 PDT
Created attachment 541578 [details] [diff] [review]
Patch v3 2/3 Fix the fragmentation of numerical IPs
Comment 19 Gian-Carlo Pascutto [:gcp] 2011-06-23 19:15:55 PDT
Created attachment 541579 [details] [diff] [review]
Patch v3 Fix dbservice testcase typos
Comment 20 Gian-Carlo Pascutto [:gcp] 2011-06-23 19:18:42 PDT
Review comments incorporated, except for the BeginReading(). The prototype seems to mismatch and I didn't find any working code outside nsXXXString itself, but lots of code doing the 2 calls.
Comment 21 Tony Chang (Google) 2011-06-24 09:23:36 PDT
Comment on attachment 541577 [details] [diff] [review]
Patch v3 1/3 Fix the fragmentation routine

Review of attachment 541577 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 22 Tony Chang (Google) 2011-06-24 09:23:52 PDT
Comment on attachment 541578 [details] [diff] [review]
Patch v3 2/3 Fix the fragmentation of numerical IPs

Review of attachment 541578 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 23 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-06-24 09:49:54 PDT
question: does this also block change in protocol?  ie http versus https?
Comment 24 Gian-Carlo Pascutto [:gcp] 2011-06-24 09:57:58 PDT
>question: does this also block change in protocol?  ie http versus https?

I'm not sure what you're asking exactly, but these patches don't affect that at all. They don't operate on the scheme part of the URL, only from the hostname and on.
Comment 25 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-06-24 13:21:30 PDT
Just wondering if the block would occur on https://www.mozilla.org if the block occurs on http://www.mozilla.org and if we may need to worry about something like that.  Some websites might end up changing their login structure to track you if you login and it switches to https:// is what my concern was.  I'm not sure if this would be a valid concern.

(In reply to comment #24)
> >question: does this also block change in protocol?  ie http versus https?
> 
> I'm not sure what you're asking exactly, but these patches don't affect that
> at all. They don't operate on the scheme part of the URL, only from the
> hostname and on.
Comment 26 Gian-Carlo Pascutto [:gcp] 2011-06-29 11:45:08 PDT
Comment on attachment 541579 [details] [diff] [review]
Patch v3 Fix dbservice testcase typos

carrying forward review
Comment 27 Gian-Carlo Pascutto [:gcp] 2011-06-29 11:50:55 PDT
Passed try:
http://tbpl.mozilla.org/?tree=Try&rev=f2366e35c45c
Comment 30 Gian-Carlo Pascutto [:gcp] 2011-06-30 08:43:32 PDT
For an external way to verify the fixes, see the description in bug 633644.

Note You need to log in before you can comment on or make changes to this bug.