356355 - numeric domain normalization only happens on enchash table values

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

We should be normalizing numeric domains (hex/oct/dec form) for urls that appear on -url and -domain tables.  For example, goog-black-url currently has http://24.172.137.213/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/                        , but a warning doesn't trigger on the following:

http://0x18.0xac.0x89.0xd5/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/                  
http://030.0254.0211.0325/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/                  
http://0x18ac89d5/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/                           
http://413960661/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/                           
http://03053104725/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/

Comment 1

[Tony Chang (Google)]

Attachment: v1: canonicalize urls before lookup in -url and -domain tables

To test, make sure http://24.172.137.213/http.www.paypal.com/cgi-bin/webscrcmd=_login-run/                        is in urlclassifier2.sqlite and try loading the variations mentioned above.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

I know I already looked at this, but I just realized that the canonicalization only applies to enchash tables.  Don't we want it for non-enchash tables as well?

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 242005 [details] [diff] [review]
v1: canonicalize urls before lookup in -url and -domain tables

Nevermind, I see in trtable.js the enchash canonicalization is called for all tables.  Sorry for the confusion.

Comment 4

[Tony Chang (Google)]

Checking in enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: 1.5; previous revision: 1.4
done
Checking in trtable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/trtable.js,v  <--  trtable.js
new revision: 1.7; previous revision: 1.6
done
Checking in url-canonicalizer.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/url-canonicalizer.js,v  <--  url-canonicalizer.js
new revision: 1.4; previous revision: 1.3

On trunk.

Comment 5

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

Tony, can we also get a branch patch worked up for 1.8.1.1?

Comment 6

[Tony Chang (Google)]

Attachment: patch for ff2 branch

Same as the other patch except one line was different in enchash-decrypter.js on branch/trunk.  After this patch is applied, the two files will be the same.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 242005 [details] [diff] [review]
v1: canonicalize urls before lookup in -url and -domain tables

Looks fine.

Comment 8

[Tony Chang (Google)]

*** Bug 358966 has been marked as a duplicate of this bug. ***

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 244261 [details] [diff] [review]
patch for ff2 branch

Looks fine.

Comment 10

[Tony Chang (Google)]

Attachment: v2: additional fixes

Fixes 2 additional issues:
- a regression caused by the v1 patch where canonicalizing urls results in only using the first 5 dots of the hostname
- a bug in enchash lookups where we were comparing the non-normalized url against the regular expressions instead of the normalized url

I'll request a review from Monica once she's back from vacation (Nov 13).

Comment 11

[Tony Chang (Google)]

Comment on attachment 244971 [details] [diff] [review]
v2: additional fixes

The only difference from toolbar code is that some unittests are missing (trtable.js doesn't have a unittest block).

I'll try to add the unittests back in in a different patch in a mozilla friendly way.

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 244971 [details] [diff] [review]
v2: additional fixes

Looks fine.
>===================================================================
>--- toolkit/components/url-classifier/content/enchash-decrypter.js	old
>+++ toolkit/components/url-classifier/content/enchash-decrypter.js	new
>+    testing["http://www.barclays.co.uk.brccontrol.assruspede.org.bz/detailsconfirm"] = "co.uk.brccontrol.assruspede.org.bz";
over 80 chars (I think)

Comment 13

[Tony Chang (Google)]

Attachment: v3: clean up line > 80 char

Same as v2 with line wrapping.

Comment 14

[Tony Chang (Google)]

v3: on trunk

Checking in enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: 1.6; previous revision: 1.5
done
Checking in trtable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/trtable.js,v  <--  trtable.js
new revision: 1.8; previous revision: 1.7
done

Comment 15

[Tony Chang (Google)]

Attachment: v2: patch for ff2 branch

Combine the two patches (v1 and v3) into a single patch for the ff 2 branch.

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 245614 [details] [diff] [review]
v2: patch for ff2 branch

Looks fine.

Comment 17

[Daniel Veditz [:dveditz]]

Does this patch also normalize the URLs coming from the server? In the urlclassifier database there are phishing urls of every conceivable obfuscation, I'd hate to add this check and start missing all the non-normalized ones.

qawanted: Need to verify that on trunk before we take this patch on the branch, because that result would be worse than the original problem if not handled.

Comment 18

[Tony Chang (Google)]

(In reply to comment #17)
> Does this patch also normalize the URLs coming from the server? In the
> urlclassifier database there are phishing urls of every conceivable
> obfuscation, I'd hate to add this check and start missing all the
> non-normalized ones.

This doesn't normalize the URLs from the server, but we're going to be normalizing all of them real soon now (we're still testing to make sure we're not losing anything in the process).

If we wanted to normalize URLs from the server, it would be more complicated since the update happens on a background thread but nsIIOService (to make the nsIURI objects) needs to run on the main thread.

Comment 19

[Daniel Veditz [:dveditz]]

Define "Real Soon Now"... this sounds like the kind of thing where client and server need to update around the same time, or we're leaving some big gaps. Or the server could start sending both forms for non-normalized ones for a while... better to have a temporarily bloated list than holes in coverage. Especially since people won't upgrade to the fixed version instantaneously. The bulk update the first few days, but there's a tail of people who apparently have update problems or insufficient rights.

We're hoping to ship 2.0.0.1 mid December. Will the server be changed over by then?

Comment 20

[Tony Chang (Google)]

(In reply to comment #19)
> Define "Real Soon Now"... this sounds like the kind of thing where client and
> server need to update around the same time, or we're leaving some big gaps. Or
> the server could start sending both forms for non-normalized ones for a
> while... better to have a temporarily bloated list than holes in coverage.
> Especially since people won't upgrade to the fixed version instantaneously. The
> bulk update the first few days, but there's a tail of people who apparently
> have update problems or insufficient rights.
> 
> We're hoping to ship 2.0.0.1 mid December. Will the server be changed over by
> then?

After some more discussion, it seems to make more sense for the client to try both (check the table for the normalized and the non-normalized url).  That way we don't need to try to sync the client with the server.  The servers will make the switch some time after 2.0.0.1 ships.

I'll have an updated patch for this later today.

Comment 21

[Jay Patel [:jay]]

Comment on attachment 245614 [details] [diff] [review]
v2: patch for ff2 branch

Will wait for Tony's new patch, please request approval for that one.

Comment 22

[Tony Chang (Google)]

Attachment: v4: check both old and new canonical form

Additional patch for trunk:
Check both the URL both using the original canonicalized form and the new form in local database lookups.

Comment 23

[Tony Chang (Google)]

Attachment: v3: cumulative patch for branch

Comment 24

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 246351 [details] [diff] [review]
v3: cumulative patch for branch

Looks fine.

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 246235 [details] [diff] [review]
v4: check both old and new canonical form

LG

Comment 26

[Tony Chang (Google)]

patch v4: check both old and new canonical form
on trunk

Checking in trtable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/trtable.js,v  <--  trtable.js
new revision: 1.9; previous revision: 1.8

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 246351 [details] [diff] [review]
v3: cumulative patch for branch

approved for 1.8 branch, a=dveditz for drivers

Comment 28

[Tony Chang (Google)]

on branch

Comment 29

[Benjamin Smedberg]

This patch landed in the toolkit without a unit test. Tree rules for the toolkit now require unit tests, please prepare a test for this bug.

Comment 30

[Tony Chang (Google)]

(In reply to comment #29)
> This patch landed in the toolkit without a unit test. Tree rules for the
> toolkit now require unit tests, please prepare a test for this bug.

How do I go about testing chrome JS?  There are a bunch of tests[1], they're just not automated.

https://bugzilla.mozilla.org/attachment.cgi?id=246351&action=diff#toolkit/components/url-classifier/content/enchash-decrypter.js_sec3

Comment 31

[Benjamin Smedberg]

I'm pretty sure we could use mochitest for this. http://developer.mozilla.org/en/docs/Mochitest_FAQ and ask sayrer ;-)

Comment 32

[Chris Cooper [:coop] (he/him)]

Trying to verify this fix, but the browser doesn't seem to want to load any hex, octal, or decimal IP addresses.

For ref: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061201 BonEcho/2.0.0.1pre

The listed examples long since fell off the net, so I went to phishtank.com and grabbed a new dotted quad phishing address for testing. I ended up using:

http://210.21.119.127/sbcbconfirm/

...which the anti-phishing correctly catches. However, when I try to load both:

http://0xd2.0x15.0x77.0x7f/sbcbconfirm/
http://0322.025.0167.0177/sbcbconfirm/

...the anti-phishing _does_ pop up the web forgery warning, but the pages themselves do not actually load.

http://3524622207/sbcbconfirm/

...doesn't even load, redirecting instead to http://www.3524622207.com/sbcbconfirm/

Are these numeric domains supposed to load?

Comment 33

[Tony Chang (Google)]

(In reply to comment #32)
> However, when I try to load both:
> 
> http://0xd2.0x15.0x77.0x7f/sbcbconfirm/
> http://0322.025.0167.0177/sbcbconfirm/
> 
> ...the anti-phishing _does_ pop up the web forgery warning, but the pages
> themselves do not actually load.
> 
> http://3524622207/sbcbconfirm/
> 
> ...doesn't even load, redirecting instead to
> http://www.3524622207.com/sbcbconfirm/
> 
> Are these numeric domains supposed to load?

Hmm, it looks like hex/dec domains only load on Windows.  Not sure why they get renamed in linux/OSX.

Comment 34

[Tony Chang (Google)]

Attachment: unittests

Patch to move unittests to mochitest framework.

Comment 35

[Tony Chang (Google)]

Tests on trunk.

Checking in testing/mochitest/tests/index.html;
/cvsroot/mozilla/testing/mochitest/tests/index.html,v  <--  index.html
new revision: 1.47; previous revision: 1.46
done
RCS file: /cvsroot/mozilla/testing/mochitest/tests/test_bug356355.xhtml,v
done
Checking in testing/mochitest/tests/test_bug356355.xhtml;
/cvsroot/mozilla/testing/mochitest/tests/test_bug356355.xhtml,v  <--  test_bug356355.xhtml
initial revision: 1.1
done
Checking in toolkit/components/url-classifier/content/enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/url-classifier/content/trtable.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/trtable.js,v  <--  trtable.js
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/components/url-classifier/content/url-canonicalizer.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/url-canonicalizer.js,v  <--  url-canonicalizer.js
new revision: 1.5; previous revision: 1.4
done

Comment 36

[u279076]

I'm guessing QAWANTED is no longer needed on this bug?

Comment 37

[Aakash Desai [:aakashd]]

It's not needed anymore as the qawanted flag was set back by comment #17 before the patch landed on branch. Also, I'm verifying this due to the number of unit tests in mochitest that have been there since 2006 per comment #35.