This is IDN problem. You should look UTF-8 for this report. The 'DIVISION SLASH'(U+2215) is similar SLASH("/"). If this is used in sub-domain, it is danger. For example: http://foo.com∕bar.com/ This domain is "foo.com∕bar.com", not "foo.com". This problem is not related to domain registry. Because it is sub-domain. This problem is not reproduced latest builds. Because bug 282270 is fixed. However, if we find another way for IDN problems, this problem should be considered.

Sorry, the exapmle is wrong. For example: http://foo.com∕sub.bar.com/ This domain is "foo.com∕sub.bar.com", not "foo.com".

Thank you for pointing this out. We'll certainly take this into account in our IDN thinking. Gerv

clearing confidential flag: the IDN problems are public and keeping all the sub-problems in full view will only help in crafting solutions.

An IAB Working Group is working on Stringprep/Nameprep revisions that may be related to this: http://weblogs.mozillazine.org/gerv/archives/007785.html

This is security problem. We MUST fix before next release.

Comment on attachment 186812 [details] [diff] [review] Patch rv1.0 >Index: netwerk/dns/src/nsIDNService.cpp >+ return NS_OK; >+ } else > _retval.Append(decodedBuf); nit: You can always remove an 'else' after a 'return' If gerv is happy with this patch, then sr=darin

Oops... Thank you for your superreview!

Comment on attachment 186828 [details] [diff] [review] Patch rv1.1 >+ NS_ConvertUTF8toUCS2 ustr(_retval); nit: pls, use NS_ConvertUTF8toUTF16

Masayuki: these two characters are not the only ones we want to ban. Would it be possible for you to change the patch so that it looked for any character in a list, perhaps using a regular expression or similar? Ideally, the list would be kept in a pref, so we can do a simple security update if necessary (although I would hope it wouldn't be). Gerv

Comment on attachment 186890 [details] [diff] [review] Patch rv1.2 O.K. Please wait.

Comment on attachment 187384 [details] [diff] [review] Patch rv2.0 >Index: netwerk/dns/src/nsIDNService.cpp >+ NS_ConvertUTF8toUTF16 uri(_retval); >+ for (PRInt32 i = 0; i < mIDNBlacklist->Length(); i++) { >+ PRUnichar blacklist_char[2] = {mIDNBlacklist->get()[i], 0x0}; >+ if (uri.Find(blacklist_char) != kNotFound) { >+ _retval.Assign(input); >+ break; >+ } I don't quite understand this. Why does blacklist_char have a null character appended before doing the search? Is this going to work for non-ASCII chars? Also, this algorithm takes n*m time, where n is the number of blacklist chars and m is the number of chars in the uri. Can we do better than that? Also, do you know how to encode such chars in the prefs file? Is it UTF-8? (This may not be your problem to solve; I'm just asking.) Gerv

(In reply to comment #17) > I don't quite understand this. Why does blacklist_char have a null character > appended before doing the search? Is this going to work for non-ASCII chars? I think that we need null-terminated string for |nsAString.Find()|. So I append the null charcter everytime. > Also, this algorithm takes n*m time, where n is the number of blacklist chars > and m is the number of chars in the uri. Can we do better than that? I think that if the URI is valid, we need to check n*m time. > Also, do you know how to encode such chars in the prefs file? Is it UTF-8? > (This may not be your problem to solve; I'm just asking.) I wrote the all.js by UTF-8. It is same to "font.name-list.*".

Comment on attachment 187384 [details] [diff] [review] Patch rv2.0 OK - subject to darin's sr of the performance and algorithm, r=gerv. Gerv

Comment on attachment 187384 [details] [diff] [review] Patch rv2.0 >+ nsString* mIDNBlacklist; Heap allocating this string object doesn't make a lot of sense to me. It would be better to simply have a nsXPIDLString type as the member variable. Then you could read the pref directly into this value instead of copying it into a nsAutoString that you allocate on the heap. You can also use nsString::FindCharInSet to implement the search loop. That does what I think you want with very little code on your end.

Comment on attachment 188680 [details] [diff] [review] Patch rv3.0 >Index: netwerk/dns/src/nsIDNService.cpp >+ NS_ConvertUTF8toUTF16 uri(_retval); >+ for (PRInt32 i = 0; i < mIDNBlacklist.Length(); i++) { >+ PRUnichar blacklist_char[2] = {mIDNBlacklist.get()[i], 0x0}; >+ if (uri.FindCharInSet(blacklist_char) != kNotFound) { >+ _retval.Assign(input); >+ break; >+ } > } > > return NS_OK; > } Why not change this code to be like so: NS_ConvertUTF8toUTF16 temp(_retval); if (temp.FindCharInSet(mIDNBlacklist) != kNotFound) _retval.Assign(input); return NS_OK; Also, it seems like you could move this check into the decodeACE function. That function already does some other runtime checking to ensure that the ACE string is well formed by converting the UTF-8 value back to ACE to ensure that the same result is yielded. Adding this blacklist to that function would make sense as an additional sanity check. The code that calls decodeACE already behaves properly if decodeACE fails, so I think you should add your code there. Then you will avoid additional conversions. Sorry for not noticing this earlier.

> Why not change this code to be like so: Sorry. I couldn't understand |FindCharInSet|'s functional. > Also, it seems like you could move this check into the decodeACE > function. I don't think so. |decodeACE| is calling |ConvertUTF8toACE|, it is not calling |ConvertACEtoUTF8|. |ConvertACEtoUTF8| is called from other classes. So it is public member. I think that my check is always run for security. Therefore, we should make to fail |ConvertACEtoUTF8|.

ConvertACEtoUTF8 calls decodeACE, and ConvertACEtoUTF8 is the only caller of decodeACE. Therefore, you should be able to move your check into decodeACE. When would this not work?

Comment on attachment 188682 [details] [diff] [review] Patch rv3.1 > ConvertACEtoUTF8 *calls* decodeACE Oh, I understand! Please wait.

Comment on attachment 188700 [details] [diff] [review] Patch rv3.2 Yup, sr=darin

Comment on attachment 188700 [details] [diff] [review] Patch rv3.2 The risk is low. But this is security problem (So, this is important).

This patch doesn't seem to work for me. - I downloaded it, applied it to my Firefox tree, and rebuilt with make -f client.mk build - I set "network.IDN.whitelist.com" to true. - I visited http://www.shmoo.com/idn/ and tested that IDN display worked on the first link. - I then set network.IDN.blacklist_chars to oоgr (as in, a mid substring of the spoofing URL listed on that page), and re-visited the link. It still worked. I restarted my browser, but the result was the same. Have I misunderstood? If not, what's wrong? Gerv

I can reproduce too. But if following URI, it works fine. http://bugbug.cocolog-nifty.xn--cominfo-ef0d.nekodama.com/icons/nekodama64b.gif If the URI is written by Unicode text, Isn't |decodeACE| called?

test. http://bugbug.cocolog-nifty.com&#8725;info.nekodama.com/icons/nekodama64b.gif

Sorry, re-test. http://bugbug.cocolog-nifty.com∕info.nekodama.com/icons/nekodama64b.gif

Umm.. Comment 30 works fine, bug Comment 32 doesn't so.

I get the same results. Masayuki: can you investigate what is going on here? Gerv

Comment on attachment 189070 [details] [diff] [review] Patch rv4.0 >+PRBool nsIDNService::isValid(const nsAString& in) >+{ >+ return (mIDNBlacklist.IsEmpty() || >+ nsString(in).FindCharInSet(mIDNBlacklist) == kNotFound); You can make it inline (static). Also, you can avoid |nsString(in) by using |nsAFlatString| instead of |nsAString| because all callers pass |nsAutoString|.

Comment on attachment 189179 [details] [diff] [review] Patch rv4.1 My only comment is that "isValid()" is the wrong name for the function - it's too generic. It really should be something like isOnlySafeChars(). With that change (or a similar, better name), r=gerv. Gerv

Darin: Please re-superreview it.

Comment on attachment 189315 [details] [diff] [review] Patch rv4.2 >Index: netwerk/dns/src/nsIDNService.cpp >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ nsCAutoString utf8; >+ CopyUTF16toUTF8(outUTF16, utf8); >+ >+ if (!isOnlySafeChars(outUTF16, mIDNBlacklist)) >+ return ConvertUTF8toACE(utf8, output); >+ output.Assign(utf8); >+ >+ return NS_OK; > } This code is not very efficient in the case where the string contains safe chars. I think you should optimize for that case. Ideally, we'd have an internal version of ConvertUTF8toACE called ConvertUTF16toACE since we are working with UTF16 here, and ConvertUTF8toACE first converts its input from UTF8 to UTF16. But, even without making that change you can improve things: CopyUTF16toUTF8(outUTF16, output); if (!isOnlySafeChars(outUTF16, mIDNBlacklist)) return ConvertUTF8toACE(output, output); return NS_OK; This works because of the fact that ConvertUTF8toACE first converts its input argument to UTF16, so there is no problem using |output| as both arguments to the function ;-) sr=darin with that change

The risk is low. But this is security problem. We should fix this.

So the next thing we need is an initial list of blacklisted characters. masayuki: as soon as this is checked in, please open a new bug and attach a patch creating a blacklist from 'DIVISION SLASH'(U+2215) and 'FRACTION SLASH'(U+2044). Apply for sr= from darin, and mark it as "approval1.8b4?". Do you have checkin privileges? Gerv

(In reply to comment #43) > masayuki: as soon as this is checked in, please open a new bug and attach a > patch creating a blacklist from 'DIVISION SLASH'(U+2215) and 'FRACTION > SLASH'(U+2044). Apply for sr= from darin, and mark it as "approval1.8b4?". Oh, sorry. I forgot the blacklist after Patch rv3.2. I will do it, thanks. > Do you have checkin privileges? Yes.

I opened bug 301694.

checked-in. Thanks.