301694 - Create IDN blacklist that include 'DIVISION SLASH'(U+2215) and 'FRACTION SLASH'(U+2044)

Status: RESOLVED DUPLICATE of bug 309311

(Core :: Networking, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is remain work of bug 283016.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[Gervase Markham [:gerv]]

Suggested comment:

// If a domain includes any of the following characters, it may be a spoof 
// attempt and so we always display the domain name as punycode. This would 
// override the settings "network.IDN_show_punycode" and 
// "netword.IDN.whitelist.*".
// The list currently contains the characters:
// U+2215 DIVISION SLASH
// U+2044 FRACTION SLASH

We should list the chars as not all editors can display them.

Gerv

Comment 3

[Benjamin Smedberg]

Comment on attachment 190114 [details] [diff] [review]
Patch rv1.0

Please do not request approval until reviews are completed.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

Comment 5

[Tony Hansen]

Attachment: all pattern syntax idn characters, as a C array

'DIVISION SLASH' (U+2215) and 'FRACTION SLASH' (U+2044) are examples of unicode
Pattern_Syntax characters. In addition to these two, you should probably
consider all of the other Pattern_Syntax characters as illegal. There are a
total of 2006 of them (unfortunately). I've uploaded a complete list, extracted
from http://www.unicode.org/reports/tr36/draft/idn-chars.txt, and expressed as
a C array.

Comment 6

[Gervase Markham [:gerv]]

Tony: we are marking these two particularly as illegal because they can be used
to spoof URL separators. The IAB (Internet Architecture Board), the parent of
the IETF, has put together a working group to consider restricting the
characters permissible in IDN. We don't want to prejudice that work, so we are
keeping our blacklist limited to the more seriously problematic characters.

Gerv

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in

Comment 8

[Neil Harris]

Regarding the full list of 2006 characters: playing around with various
transformations and compressions of the list gives an apparent limiting
complexity of about 300 to 500 bytes for compressing the list (roughly 1.2 bits
per character).

I can get it down to about 500 - 750 bytes with some bitmap and range-table
compression (15-way switch statement for 8 MSBs, with bitmap lookups for the
LSBs, or none if entire MSB range is empty) but just doing a simple C switch
statement generates code that's about 1280 bytes on an x86 with gcc and
optimization, and this form of the code is much more readable and maintainable
and does not need a hand-hacked "pre-compiler" to generate the bitmaps etc.

At the moment, it does not look like the super-efficient implementation is
needed, unless there's a desperate need to save 500 bytes of code.

Challenge: can anyone do better than 750 bytes for the code+data for this?

If you need the super-efficient implementation for the full set of characters,
drop me an E-mail.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

can nsIUGenCategory be used?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See bug 286535.

Comment 11

[Gervase Markham [:gerv]]

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

Comment 12

[Neil Harris]

The following are also possible Unicode homographs for (one or more) slashes,
and should be added to this blocklist, in addition to the existing U+2215 and
U+2044:

Combining long solidus overlay U+0338 &#x0338; <br>
Fullwidth solidus U+FF0F &#xff0f; <br>
Big solidus U+29F8 &#x29f8;<br>
Solidus with overbar U+29F6 &#x29f6;<br>
Triple solidus binary relation U+2AFB &#x2afb;<br>
Double solidus operator U+2AFD &#x2afd;<br>
Combining short solidus overlay U+0337 &#x0337;<br>
Integral extension U+23AE &#x23ae;<br>

Note: some of these are from math fonts, and some may be visible depending on
combining-character rendering. The "integral extension" character, in
particular, may be rendered either as a vertical bar or a slash-like sloping
line, depending on font. None of these (in my opinion) are reasonable characters
for use in identifiers.

Comment 13

[Neil Harris]

Attachment: Python program to generate Standard C escaped UTF-8 string

This Python program generates the expanded blacklist as a standard C
hex-escaped UTF-8 string from a human-readable listing of the offending
characters.

Comment 14

[Neil Harris]

Attachment: Created a patch to add new blacklist characters, with more self-documenting string format

This adds the new proposed characters, and uses Javascript \u escape format,
which makes for a more self-documenting blacklist string. See
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Guide:Unicode for
documentation of this string escape format.

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 193309 [details] [diff] [review]
Created a patch to add new blacklist characters, with more self-documenting string format

Great - I like the new format. r=gerv.

Gerv

Comment 16

[Gervase Markham [:gerv]]

Reopening; but if there are any further patches, let's do them in new bugs.

Gerv

Comment 17

[Gervase Markham [:gerv]]

Another thought: can we turn that pref into a regexp? That way, we could include
character ranges if that ever became necessary.

Gerv

Comment 18

[Benjamin Smedberg]

gerv: not easily, no... the networking engine is rather far removed from
anything that does regexp processing

Comment 19

[Andreas Otte]

Where in the code is this check, that is expanded here, happening?

Comment 20

[Darin Fisher]

Comment on attachment 193309 [details] [diff] [review]
Created a patch to add new blacklist characters, with more self-documenting string format

sr=darin

Comment 21

[Gervase Markham [:gerv]]

andreas: see the patch attached to bug 283016.

Gerv

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

that's in IDN code, and the only thing it does is show the host as punycode
instead of the unicode form, afaict. therefore, I think bug 304904 should be
fixed independently of this blacklist.

Comment 23

[Andreas Otte]

There is a check with isOnlySafeChars, but I suspect it will only get triggered
if the IDNService is used, which does not happen with ascii-only hostsnames I
think. Both checks are necessary, maybe it is possible to chare the ascii
portion of the blacklist.

Comment 24

[Gervase Markham [:gerv]]

Fixed on trunk and branch. Any further updates in new bugs, please.

Checking in ./modules/libpref/src/init/all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.591; previous revision: 3.590
done

---

Checking in ./modules/libpref/src/init/all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.585.2.5; previous revision: 3.585.2.4
done

Gerv

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

guys, why does this work? I'm told this causes an NS_WARNING in prefread.cpp,
due to lack of support for \u.
http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/prefread.cpp#388

This file is not parsed by a JS interpreter, but by a specialized parser.

Did anyone test this patch?

reopening because of that

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Please re-create or back out the latest patch.
The patch makes infinite loop on the test case.
See http://bugbug.cocolog-nifty.xn--cominfo-ef0d.nekodama.com/icons/nekodama64b.gif

usenet:
You *MUST* test before attaching a patch.

Comment 27

[Darin Fisher]

Gah... i'm also seeing the NS_WARNING in my trunk build.  I figured the code had
been tested when I reviewed this (and forgot that I never coded support for
\u-escape sequences in prefread.cpp) :(

Comment 28

[Darin Fisher]

I backed this patch out on the trunk.

Comment 29

[Darin Fisher]

I filed bug 307438 on adding support for \u escape sequences to prefread.cpp

Comment 30

[Asa Dotzler [:asa]]

Darin, is this critical to get backed out for Firefox 1.5 beta 1?

Comment 31

[Darin Fisher]

Asa: Hmm.. yes, I think so.  I would have backed it out had I realized that it
was commited on the 1.8 branch.  Apparently, no one added the fixed1.8 keyword! :(

Comment 32

[Darin Fisher]

OK, I backed out the patch from the 1.8 branch as well.

Comment 33

[Darin Fisher]

backout from 1.8 branch was with a=asa ("per the results of our driver
discussion" according to asa).

Comment 34

[Neil Harris]

Aaargh. Mea culpa. My apologies: I should have tested, but mistakenly assumed
this was trivial. I won't make that mistake again.

Some punycoded test cases:
u'www.pyth\u0337on.org' -> www.xn--python-e8d.org
u'www.pyth\u0338on.org' -> www.xn--python-l8d.org
u'www.pyth\u2044on.org' -> www.xn--python-sq0c.org
u'www.pyth\u2215on.org' -> www.xn--python-se3c.org
u'www.pyth\u23aeon.org' -> www.xn--python-lq5c.org
u'www.pyth\u29f6on.org' -> www.xn--python-6w4d.org
u'www.pyth\u29f8on.org' -> www.xn--python-lx4d.org
u'www.pyth\u2afbon.org' -> www.xn--python-ef6d.org
u'www.pyth\u2afdon.org' -> www.xn--python-sf6d.org
u'www.pyth\uff0fon.org' -> www.pyth/on.org

UTF-8 versions in an attachment coming soon. I'll let you know when I've tested
them.

Comment 35

[Neil Harris]

Attachment: Some test cases, in a UTF-8-encoded HTML file

Here are some test cases. Although some are caught by the nameprep logic,
others are not, and although some are more convincing than others (depending on
your choice of font), some of them are quite believable spoofs, even one really
devious one where the slash is made up of a Canadian syllabics character and a
solidus overlay.

Comment 36

[Neil Harris]

Also, note that URLs with nbsps in the domain name are displayed as clickable
links: they probably shouldn't be, since nameprep will convert them to ASCII
spaces, are not valid either in a conventional domain label, or in a Punycoded
IDN label (since Punycoding passes them straight through). 

This should probably also apply to any other URL where the nameprep'd version of
the FQDN has an ASCII space in it.

Comment 37

[Gervase Markham [:gerv]]

My sincere apologies for not testing this - as the guy who checked it in, the
buck stops here. And I forgot the fixed1.8 keyword as well :-(

[e-mail]: any chance of configuring your real name in Bugzilla,
so we don't have to call you "usenet"? :-)

Gerv

Comment 38

[Gervase Markham [:gerv]]

[e-mail]: are you able to work on bug 307438 and then this one
again?

Gerv

Comment 39

[Gervase Markham [:gerv]]

Alternatively, we could do the new list using the current encoding method. Given
that we are locked down for 1.5, this may be a better plan. Anyone got a
Unicode-compatible editor? :-)

Gerv

Comment 40

[Neil Harris]

Attachment: COMPLETELY UNTESTED new patch, using UTF-8 encoding

This is an (as yet untested) patch for this bug, just encoding the characters
as UTF-8. This was generated by a Python program, as some of these characters
cause some text editors (even emacs) to do funny things.

Please don't commit this until I've tested it.

Comment 41

[Neil Harris]

Attachment: Patch now tested

Note: this patch is not a proper CVS patch, since I can't currently make my
build environment work. However, when manually applied to all.js in an
installation of 1.5b1, it:
* passes the pre-checkin test
* blocks the display of all but one of the test cases, which I believe is a
separate issue from that addressed in this bug, also related to IDN
normalization, and will file as a new bug

Can someone with better Mozilla-fu than me confirm this, please, and generate a
proper patch prior to reviewing it?

Comment 42

[Daniel Veditz [:dveditz]]

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

Comment 43

[Asa Dotzler [:asa]]

Gerv, can you drive this one? Looks like we need a proper patch and some reviews. 

Comment 44

[Neil Harris]

See also
* Bug 309128
* Bug 309133

and bug 301694 as well, or there will some difficulty in adding these characters
in a maintainable way. 

Perhaps a half-way house would be representing trhe codepoints in the blocklist
in hex, and having a special-case decoder that only decodes bare hex number
separated by spaces just for this purpose?

Comment 45

[Gervase Markham [:gerv]]

Is it that much more complex to just fix it properly by decoding \u and \x
sequences?

Gerv

Comment 46

[Daniel Veditz [:dveditz]]

(In reply to comment #45)
> Is it that much more complex to just fix it properly by decoding \u and \x
> sequences?

That's bug 307438. Not going to decode \x, pref files are supposed to be utf8
and \x is just going to be misused.

Comment 47

[Gervase Markham [:gerv]]

Bug 309311 has the canonical list of chars we are going to ban, including all of
those from this bug. We are going to use that set of chars as soon as bug
307438, which allows us to insert them into prefs.js in a sensible way, is fixed.

Gerv

*** This bug has been marked as a duplicate of 309311 ***