283016 - Make it possible to blacklist characters in domain names

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

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

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.

Comment 1

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

Attachment: screenshot

Comment 2

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

Attachment: screenshot

Comment 3

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

Sorry, the exapmle is wrong.

For example:
http://foo.com∕sub.bar.com/

This domain is "foo.com∕sub.bar.com", not "foo.com".

Comment 4

[Gervase Markham [:gerv]]

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

Gerv

Comment 5

[Daniel Veditz [:dveditz]]

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

Comment 6

[Erik van der Poel]

An IAB Working Group is working on Stringprep/Nameprep revisions that may be
related to this:

http://weblogs.mozillazine.org/gerv/archives/007785.html

Comment 7

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

Attachment: Patch rv1.0

Comment 8

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

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

Comment 9

[Darin Fisher]

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

Comment 10

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

Attachment: Patch rv1.1

Oops... Thank you for your superreview!

Comment 11

[Jungshik Shin]

Comment on attachment 186828 [details] [diff] [review]
Patch rv1.1


>+  NS_ConvertUTF8toUCS2 ustr(_retval);

nit: pls, use NS_ConvertUTF8toUTF16

Comment 12

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

Attachment: Patch rv1.2

Comment 13

[Gervase Markham [:gerv]]

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 14

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

Comment on attachment 186890 [details] [diff] [review]
Patch rv1.2

O.K. Please wait.

Comment 15

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

Attachment: Patch rv2.0

Comment 16

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

Attachment: Patch rv2.0

Comment 17

[Gervase Markham [:gerv]]

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

Comment 18

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

(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 19

[Gervase Markham [:gerv]]

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 20

[Darin Fisher]

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 21

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

Attachment: Patch rv3.0

Comment 22

[Darin Fisher]

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.

Comment 23

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

Attachment: Patch rv3.1

> 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|.

Comment 24

[Darin Fisher]

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 25

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

Comment on attachment 188682 [details] [diff] [review]
Patch rv3.1

> ConvertACEtoUTF8 *calls* decodeACE

Oh, I understand! Please wait.

Comment 26

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

Attachment: Patch rv3.2

Comment 27

[Darin Fisher]

Comment on attachment 188700 [details] [diff] [review]
Patch rv3.2

Yup, sr=darin

Comment 28

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

Comment on attachment 188700 [details] [diff] [review]
Patch rv3.2

The risk is low. But this is security problem (So, this is important).

Comment 29

[Gervase Markham [:gerv]]

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

Comment 30

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

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?

Comment 31

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

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

Comment 32

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

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

Comment 33

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

Umm..

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

Comment 34

[Gervase Markham [:gerv]]

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

Gerv

Comment 35

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

Attachment: Patch rv4.0

Comment 36

[Jungshik Shin]

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 37

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

Attachment: Patch rv4.1

Comment 38

[Gervase Markham [:gerv]]

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

Comment 39

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

Attachment: Patch rv4.2

Comment 40

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

Darin:

Please re-superreview it.

Comment 41

[Darin Fisher]

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

Comment 42

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

Attachment: Patch rv4.3

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

Comment 43

[Gervase Markham [:gerv]]

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

Comment 44

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

(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.

Comment 45

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

I opened bug 301694.

Comment 46

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

checked-in. Thanks.