368998 - when normalizing hostnames, we don't properly escape non-alphanumerics

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

According to the spec on the wiki:
http://wiki.mozilla.org/Phishing_Protection:_Server_Spec#Canonical_Hostname_Creation

When canonicalizing hostnames, the final step is to url escape everything that is not alphanumeric or hyphen or dot.  In the client code, we use encodeURI, but that doesn't do the escaping we want.  We'll need to write our own function to do this.

Comment 1

[Tony Chang (Google)]

Attachment: do our own escaping of hostnames

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 254340 [details] [diff] [review]
do our own escaping of hostnames

looks good

Comment 3

[Tony Chang (Google)]

On trunk

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.9; previous revision: 1.8
done
Checking in testing/mochitest/tests/test_bug356355.xhtml;
/cvsroot/mozilla/testing/mochitest/tests/test_bug356355.xhtml,v  <--  test_bug356355.xhtml
new revision: 1.3; previous revision: 1.2
done
Checking in testing/mochitest/tests/test_bug368998.xhtml;
/cvsroot/mozilla/testing/mochitest/tests/test_bug368998.xhtml,v  <--  test_bug368998.xhtml
initial revision: 1.1

Comment 4

[Tony Chang (Google)]

Attachment: branch patch

same as above patch minus tests because mochitest isn't on branch

Comment 5

[Tony Chang (Google)]

Attachment: refactor to allow unittesting

Refactor the escaping code so we can unittest it separately.  Migrate server side tests to mochitest and consolidate all tests into a single file.  I'll get rid of the old mochitest files once the test tinderboxen are clobbered.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 256874 [details] [diff] [review]
refactor to allow unittesting

looks great, thanks for doing this

Comment 7

[Tony Chang (Google)]

on trunk, branch patch coming up

Comment 8

[Tony Chang (Google)]

Attachment: branch patch

Comment 9

[Tony Chang (Google)]

Comment on attachment 256907 [details] [diff] [review]
branch patch

Ugh, it turns out this is slow for really long hostnames.  This needs to be in C++, I'll add it to the utils component in bug 370860.

Comment 10

[Tony Chang (Google)]

Attachment: move host encoding to c++ (url classifier utils component)

Do the hostname escaping in c++.  Also use unescape instead of the JS hexDecode because it's much faster.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 257048 [details] [diff] [review]
move host encoding to c++ (url classifier utils component)

>--- url-classifier/public/nsIUrlClassifierUtils.idl	27 Feb 2007 23:49:44 -0000	1.1
>+++ url-classifier/public/nsIUrlClassifierUtils.idl	2 Mar 2007 19:20:40 -0000
>-[scriptable, uuid(9afd3add-eadc-409f-a187-e3bf60e47290)]
>+[scriptable, uuid(89ea43b0-a23f-4db2-8d23-6d90dc55f67a)]
> interface nsIUrlClassifierUtils : nsISupports

I guess changing this interface on the branch will be ok if we never shipped a release with the old version?


>--- url-classifier/src/nsUrlClassifierUtils.h	27 Feb 2007 23:49:44 -0000	1.1
>+++ url-classifier/src/nsUrlClassifierUtils.h	2 Mar 2007 19:20:40 -0000
>@@ -34,21 +34,25 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef nsUrlClassifierUtils_h_
> #define nsUrlClassifierUtils_h_
> 
> #include "nsIUrlClassifierUtils.h"
> 
>+class Charmap;

This is kind of a generic class name, and there's nothing to scope it.  Maybe have it be a nested class in nsUrlClassifierUtils?

>+  Charmap *mEscapeCharmap;

This seems like a good place to use nsAutoPtr.

Looks good otherwise.

Comment 12

[Tony Chang (Google)]

Attachment: v2: move host encoding to c++ (url classifier utils component)

use nsAutoPtr and move Charmap class inside nsUrlClassifierUtils class

Comment 13

[Tony Chang (Google)]

"v2: move host encoding to c++" is on trunk