The default bug view has changed. See this FAQ.

numeric domain normalization only happens on enchash table values

VERIFIED FIXED

Status

()

Toolkit
Safe Browsing
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: Tony Chang (Google), Assigned: Tony Chang (Google))

Tracking

({fixed1.8.1.1})

Trunk
fixed1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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/
(Assignee)

Comment 1

11 years ago
Created attachment 242005 [details] [diff] [review]
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.
Attachment #242005 - Flags: review?(mmchew)
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 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.
Attachment #242005 - Flags: review?(mmchew) → review+
(Assignee)

Comment 4

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Tony, can we also get a branch patch worked up for 1.8.1.1?
Flags: blocking1.8.1.1?
(Assignee)

Comment 6

11 years ago
Created attachment 244261 [details] [diff] [review]
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.
Attachment #244261 - Flags: review?(mmchew)
Comment on attachment 242005 [details] [diff] [review]
v1: canonicalize urls before lookup in -url and -domain tables

Looks fine.
(Assignee)

Comment 8

11 years ago
*** Bug 358966 has been marked as a duplicate of this bug. ***
Comment on attachment 244261 [details] [diff] [review]
patch for ff2 branch

Looks fine.
Attachment #244261 - Flags: review?(mmchew) → review+
(Assignee)

Updated

11 years ago
Attachment #244261 - Flags: approval1.8.1.1?
(Assignee)

Comment 10

11 years ago
Created attachment 244971 [details] [diff] [review]
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).
(Assignee)

Comment 11

11 years ago
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.
Attachment #244971 - Flags: review?(mmchew)
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)
Attachment #244971 - Flags: review?(mmchew) → review+
(Assignee)

Comment 13

11 years ago
Created attachment 245608 [details] [diff] [review]
v3: clean up line > 80 char

Same as v2 with line wrapping.
Attachment #244971 - Attachment is obsolete: true
(Assignee)

Comment 14

11 years ago
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
(Assignee)

Comment 15

11 years ago
Created attachment 245614 [details] [diff] [review]
v2: patch for ff2 branch

Combine the two patches (v1 and v3) into a single patch for the ff 2 branch.
Attachment #244261 - Attachment is obsolete: true
Attachment #245614 - Flags: review?(mmchew)
Attachment #244261 - Flags: approval1.8.1.1?
Comment on attachment 245614 [details] [diff] [review]
v2: patch for ff2 branch

Looks fine.
Attachment #245614 - Flags: review?(mmchew) → review+
(Assignee)

Updated

11 years ago
Attachment #245614 - Flags: approval1.8.1.1?
Flags: blocking1.8.1.1? → blocking1.8.1.1+
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.
Keywords: qawanted
(Assignee)

Comment 18

11 years ago
(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.
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?
(Assignee)

Comment 20

11 years ago
(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

11 years ago
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.
Attachment #245614 - Flags: approval1.8.1.1? → approval1.8.1.1-
(Assignee)

Comment 22

11 years ago
Created attachment 246235 [details] [diff] [review]
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.
Attachment #246235 - Flags: review?(mmchew)

Updated

11 years ago
Blocks: 361542
(Assignee)

Updated

11 years ago
No longer blocks: 361542
(Assignee)

Comment 23

11 years ago
Created attachment 246351 [details] [diff] [review]
v3: cumulative patch for branch
Attachment #245614 - Attachment is obsolete: true
Attachment #246351 - Flags: review?(mmchew)
Comment on attachment 246351 [details] [diff] [review]
v3: cumulative patch for branch

Looks fine.
Attachment #246351 - Flags: review?(mmchew) → review+
Comment on attachment 246235 [details] [diff] [review]
v4: check both old and new canonical form

LG
Attachment #246235 - Flags: review?(mmchew) → review+
(Assignee)

Updated

11 years ago
Attachment #246351 - Flags: approval1.8.1.1?
(Assignee)

Comment 26

11 years ago
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 on attachment 246351 [details] [diff] [review]
v3: cumulative patch for branch

approved for 1.8 branch, a=dveditz for drivers
Attachment #246351 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Comment 28

11 years ago
on branch
Keywords: fixed1.8.1.1
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.
Flags: in-testsuite?
(Assignee)

Comment 30

11 years ago
(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
I'm pretty sure we could use mochitest for this. http://developer.mozilla.org/en/docs/Mochitest_FAQ and ask sayrer ;-)

Comment 32

10 years ago
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?
(Assignee)

Comment 33

10 years ago
(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.
(Assignee)

Comment 34

10 years ago
Created attachment 247594 [details] [diff] [review]
unittests

Patch to move unittests to mochitest framework.
(Assignee)

Comment 35

10 years ago
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
Flags: in-testsuite? → in-testsuite+
I'm guessing QAWANTED is no longer needed on this bug?
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.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.