Make it possible to blacklist characters in domain names

RESOLVED FIXED in mozilla1.8beta4

Status

()

defect
P1
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla1.8beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b3 -
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 12 obsolete attachments)

12.64 KB, image/png
Details
6.58 KB, patch
masayuki
: review+
masayuki
: superreview+
benjamin
: approval1.8b4+
Details | Diff | Splinter Review
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.
Blocks: punycode
Group: security

Updated

14 years ago
Blocks: IDN
Summary: IDN problem: using 'DIVISION SLASH'(U+2215) in sub-domain → IDN problem: using 'DIVISION SLASH'(U+2215) or 'FLACTION SLASH'(U+2044) in sub-domain

Comment 6

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

http://weblogs.mozillazine.org/gerv/archives/007785.html
Summary: IDN problem: using 'DIVISION SLASH'(U+2215) or 'FLACTION SLASH'(U+2044) in sub-domain → IDN problem: using 'DIVISION SLASH'(U+2215) or 'FRACTION SLASH'(U+2044) in sub-domain
Posted patch Patch rv1.0 (obsolete) — Splinter Review
Assignee: darin → masayuki
Status: NEW → ASSIGNED
Attachment #186812 - Flags: review?(gerv)
This is security problem. We MUST fix before next release.
Flags: blocking1.8b3?
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta3

Comment 9

14 years ago
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
Attachment #186812 - Flags: superreview+
Posted patch Patch rv1.1 (obsolete) — Splinter Review
Oops... Thank you for your superreview!
Attachment #186812 - Attachment is obsolete: true
Attachment #186828 - Flags: superreview+
Attachment #186828 - Flags: review?(gerv)

Comment 11

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


>+  NS_ConvertUTF8toUCS2 ustr(_retval);

nit: pls, use NS_ConvertUTF8toUTF16
Posted patch Patch rv1.2 (obsolete) — Splinter Review
Attachment #186828 - Attachment is obsolete: true
Attachment #186890 - Flags: superreview+
Attachment #186890 - Flags: review?(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 on attachment 186890 [details] [diff] [review]
Patch rv1.2

O.K. Please wait.
Attachment #186890 - Flags: review?(gerv) → review-
Attachment #186890 - Attachment is obsolete: true
Attachment #187383 - Flags: review?(gerv)
Attachment #187383 - Attachment is obsolete: true
Attachment #187383 - Flags: review?(gerv)

Updated

14 years ago
Attachment #187384 - Flags: review?(gerv) → review?(darin)

Updated

14 years ago
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking1.8b3-
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
Attachment #187384 - Flags: review?(darin) → review+

Comment 20

14 years ago
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.
Attachment #187384 - Flags: superreview?(darin) → superreview-
Posted patch Patch rv3.0 (obsolete) — Splinter Review
Attachment #187384 - Attachment is obsolete: true
Attachment #188680 - Flags: superreview?(darin)
Attachment #188680 - Flags: review+

Comment 22

14 years ago
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.
Attachment #188680 - Flags: superreview?(darin) → superreview-
Posted patch Patch rv3.1 (obsolete) — Splinter Review
> 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|.
Attachment #188680 - Attachment is obsolete: true
Attachment #188682 - Flags: superreview?(darin)
Attachment #188682 - Flags: review+

Comment 24

14 years ago
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.
Attachment #188682 - Flags: superreview?(darin) → superreview-
Posted patch Patch rv3.2 (obsolete) — Splinter Review
Attachment #188682 - Attachment is obsolete: true
Attachment #188700 - Flags: superreview?(darin)
Attachment #188700 - Flags: review+

Comment 27

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

Yup, sr=darin
Attachment #188700 - Flags: superreview?(darin) → superreview+
Comment on attachment 188700 [details] [diff] [review]
Patch rv3.2

The risk is low. But this is security problem (So, this is important).
Attachment #188700 - Flags: approval1.8b4?
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?
I get the same results. Masayuki: can you investigate what is going on here?

Gerv
Posted patch Patch rv4.0 (obsolete) — Splinter Review
Attachment #189070 - Flags: superreview?(darin)
Attachment #189070 - Flags: review?(gerv)

Comment 36

14 years ago
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|.
Attachment #189070 - Flags: superreview?(darin)
Attachment #189070 - Flags: review?(gerv)
Attachment #189070 - Flags: review-
Posted patch Patch rv4.1 (obsolete) — Splinter Review
Attachment #189070 - Attachment is obsolete: true
Attachment #189179 - Flags: superreview?(darin)
Attachment #189179 - Flags: review?(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
Attachment #189179 - Flags: review?(gerv) → review+
Posted patch Patch rv4.2 (obsolete) — Splinter Review
Attachment #189179 - Attachment is obsolete: true
Attachment #189315 - Flags: superreview?(darin)
Attachment #189315 - Flags: review+
Target Milestone: mozilla1.8beta3 → mozilla1.8beta4
Darin:

Please re-superreview it.

Comment 41

14 years ago
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
Attachment #189315 - Flags: superreview?(darin) → superreview+
Posted patch Patch rv4.3Splinter Review
The risk is low. But this is security problem. We should fix this.
Attachment #189315 - Attachment is obsolete: true
Attachment #190045 - Flags: superreview+
Attachment #190045 - Flags: review+
Attachment #190045 - Flags: approval1.8b4?
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
Summary: IDN problem: using 'DIVISION SLASH'(U+2215) or 'FRACTION SLASH'(U+2044) in sub-domain → Make it possible to blacklist characters in domain names
(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.

Updated

14 years ago
Attachment #190045 - Flags: approval1.8b4? → approval1.8b4+
checked-in. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.