309128 - More IDN DNS lookup and URL display problems: potential for spoofing

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Neil Harris]

Nasty broken DNS lookups are being performed when certain Unicode characters are
present in URLs. Combined with the display of these URLs in their normalized
forms, this may potentially create a spoofing problem, although at the moment I
have not yet found an exploit.

How to reproduce problem:
1 Start up a packet sniffer
2 Open Firefox, and go to the web page provided in the attachment below
3 Click on some links
4 Wince at the garbage being sent in DNS lookups
5 Note that some of the displayed domain names in URLs contain spaces,
potentially allowing domain spoofing if a large number of space-generating
characters were appended to an apparent domain name: eg:

 www.google.comXXXXXXXXXXXXXXXXXXXXXX....XXXXXXXX

where X represents a space-generating character


What happens:
* DNS lookups are generated with characters other than ASCII letters, digits or
hyphen, violating RFC 1035.

What should happen instead:
Either:
* the bogus characters should never get as far as the DNS logic
Or:
* The invalid DNS lookups should fail
Preferably both fixes should be applied, if possible.

Comment 1

[Neil Harris]

Attachment: HTML page with test cases: see description above

This is the HTML page referred to above, with the links to click on to generate
the DNS lookup and/or name display problems mentioned.

Comment 2

[Neil Harris]

IMPORTANT: please note that not all of the links given cause problems: however,
most of them cause either a bad display or an invalid DNS lookup to occur. 

Comment 3

[Neil Harris]

By the way, the Unicode characters used to make this list are all of the
non-iso-8559-1 characters whose nameprep()'d form contains an iso-8859-1
character, as of the version of nameprep() in Python 2.3.3.

Comment 4

[Darin Fisher]

Can this bug also be solved by displaying these domain names using punycode, or
do we need to extend the net_IsValidHostName check?  (see bug 304904)

Comment 5

[Neil Harris]

Well, probably both, in a belt-and-braces way.

That is to say, the existence of any blacklisted character in the source Unicode
FQDN string should result in the string forced to be displayed as Punycode, due
to display issues.

AND

since the Unicode -> domain name code is clearly not watertight yet, any
character other than 0-9a-zA-Z and '-' in a value that might be passed to the
name resolver library should result both in lookups failing at the low level,
and the domain name being reported as broken at the high level.

THEN

We should then file a bug to fix the upstream issue: namely, that normalization
of some FQDN labels can give rise to all-ASCII strings which contain illegal
values for domain names. Presumably the the bug arises from the illegal
character check currently being carried out before NAMEPREP normalization of the
FQDN labels: it should clearly be carried out _after_ NAMEPREP normalization. Or
perhaps the code is even cruder, and just blindly trusts the URL parser to catch
all illegal characters at parse time -- which would be a bad idea, since such a
parser is inevitably complex, and likely to be a place where obscure bugs may
remain hidden for long periods.

FINALLY

Even when the Unicode -> domain name code is fixed, the low-level LDH check just
before sending to the name resolver should still remain: it's cheap, enforces
compliance with RFC 1035, on the "be careful what you send" principle of
engineering, and may catch any errors introduced in later amendments to the
higher-level code. Perhaps, when the upstream code is (thought to be) finally
fixed, it should be left in as an assert?

Comment 6

[Neil Harris]

More specific bug for the ASCII leakage issue now filed at Bug 310361, as per
comment above.

Comment 7

[Darin Fisher]

Usenet: That sounds like a good plan to me.  I particularly like the idea of
applying a whitelist before sending a string off to the host resolver.

Comment 8

[Gervase Markham [:gerv]]

We now have an ASCII character blacklist which is applied before DNS resolving.
This was added in bug 304904. Here is the branch version of the patch:

+    const char* bad = net_FindCharInSet(hostname, end,
+                                        "\x01\x02\x03\x04\x05\x06\x07\x08"
+                                        "\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10"
+                                        "\x11\x12\x13\x14\x15\x16\x17\x18"
+                                        "\x19\x1a\x1b\x1c\x1d\x1e\x1f\x20"
+                                        "\x21\x22\x23\x25\x26\x27\x28\x29"
+                                        "\x2a\x2b\x2c\x2f\x3b\x3c\x3d\x3e"
+                                        "\x3f\x40\x5c\x5e\x7b\x7c\x7e\x7f");

Even if we have bugs which allow bogus ASCII characters to make it through to
the DNS resolver, this should stop the lookup from happening. Does that deal
with the security issue? Is that character list extensive enough?

Do any of the characters in the HTML testcase attached to this bug need to be
added to the IDN blacklist in bug 309133?

Gerv

Comment 9

[Asa Dotzler [:asa]]

Darin, so are we covered here as far as the security concern goes, as Gerv
suggests in comment #8?

Comment 10

[Asa Dotzler [:asa]]

After talking with Darin, we're going to push this out to after the beta2 (first
RC).

Comment 11

[Asa Dotzler [:asa]]

Masayuki and Dveditz, we could use some help here. Darin's overloaded and this
risks falling off this list.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #8)
> We now have an ASCII character blacklist which is applied before DNS resolving.
> This was added in bug 304904. Here is the branch version of the patch:

This is not what was checked in, as checked in we only catch "%/\\", which in
particular leaves spaces wide open, among other bogus chars.

> Even if we have bugs which allow bogus ASCII characters to make it through to
> the DNS resolver,

They aren't bugs, it's NAMEPREP doing what NAMEPREP does.

> Is that character list extensive enough?

The one checked in isn't. The one you quoted in your comment would be.

Comment 13

[Asa Dotzler [:asa]]

Dan, so what needs to happen with this bug?

Comment 14

[Andreas Otte]

Attachment: patch to update the blacklist

Comment 15

[Darin Fisher]

Comment on attachment 199633 [details] [diff] [review]
patch to update the blacklist

An implementation similar to nsHttpChannel's IsValidToken() would be faster,
but I don't think we need to worry about performance here.

sr=darin

Comment 16

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

Comment on attachment 199633 [details] [diff] [review]
patch to update the blacklist

can you add a comment mentioning which characters this includes? The list isn't
particularly readable...

shouldn't characters > 127 also be excluded?

maybe a whitelist would be easier?

anyway, r=biesi with at least the comment added

Comment 17

[Andreas Otte]

I will put the comment back in, it was in the original patch for bug 304904, but
it got lost on the way. Later today I will check the patch into the trunk, and
into the 1.8 branch after I get approval.

Comment 18

[Darin Fisher]

Yeah, what about characters > 127?  If those are to be excluded, then a
whitelist or some range checks (i.e., a hand rolled loop) would be better.

Comment 19

[Andreas Otte]

extended blacklist checked in on trunk and 1.8 branch

Comment 20

[Darin Fisher]

We can resolve this bug as fixed then, right?

Comment 21

[Neil Harris]

This is _probably_ fixed, given 

a) the new blacklist at the ASCII side of the DNS resolver, which should fix the invalid DNS lookup problem, and
b) the new blacklist at the Unicode side of the URL display code, which should fix the potential DNS spoofing problem.

However, it should still be _tested_ with an up-to-date build before it can be resolved as fixed.

Comment 22

[Darin Fisher]

OK, in that case, we can mark this as FIXED, and then either REOPEN it or mark as VERIFIED once testing is complete.

Comment 23

[Andreas Otte]

I kept it open because of the question about characters > 127. Bug 309671 has a new patch that converts the blacklist to a whitelist and covers those. 

Comment 24

[Andreas Otte]

If we want this in 1.0.8 we also need some code from bug 304904.