316444 - Mozilla IDN code should set UseSTD3ASCIIRules to prevent subtle IDN attacks

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect)

Description

[Neil Harris]

The current IDN code does not set UseSTD3ASCIIRules during the preparation of the IDN. This risks letting through devious attacks on IDN processing.

Although RFC 3490 makes the setting of UseSTD3ASCIIRules a choice by the application designer, not setting UseSTD3ASCIIRules makes it possible for the IDN code to generate non-RFC-1035-compliant domain names with labels containing characters outside the LDH set, including cases where the non-LDH characters are not present in the input, but generated by NAMEPREP. For RFC 1035 compliance, we should be setting UseSTD3ASCIIRules during IDN processing, and rejecting nonconformant labels as invalid. This would then be conformant with both RFC 3490 and RFC 1035.

The fix for bug 309671 goes most, but not all, of the way to fixing this: however, the code allows for a number of characters which are OK in complete domain name/IP addresses which are _not_ OK in RFC-1035-compliant DNS labels. 

For example, dots generated by NAMEPREP normalization provide a way of "leaking" dots into domain name labels, causing the prepared DNS string to appear to contain extra labels, something which could possibly be used to bypass DNS name filtering on the unprocessed label. 

However, I suggest relaxing the UseSTD3ASCIIRules rules for just one non-RFC-1035 character, the underscore, which is known to be used in real-world DNS labels, since blocking it would break known existing applications, and I cannot yet see any scope for devious attacks using this character. (We would of course still need to block any visual spoofs of it in the visual spoof blacklist.)

Note: even with UseSTD3ASCIIRules set, I would still recommend that the character whitelist in the fix for bug 309671 be kept; IDN processing is so complex, and so difficult to prove correct, that the presence of a simple cheap double-check at the end of DNS processing is still a good idea.

Comment 1

[Neil Harris]

A thought; if the recommendations above are followed, the bug 309671 character check could be left in the codebase, but made into a fatal assertion, since strings which failed it "could not happen" if the code above works OK; it would then function as a double-check on the rest of the code, with any failed assertions being reported back via talkback as a demonstration that the upper-level address/name parsing code was defective.

Comment 2

[Neil Harris]

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

Comment 3

[Neil Harris]

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

Comment 4

[Neil Harris]

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

Comment 5

[Neil Harris]

Attachment: Patch to apply UseSTD3ASCII check in ToUnicode processing

Note: this patch has only had cursory testing -- due to the wiggly nature of the behavior in question, which is buried beneath Nameprep conversion, this needs more testing on both correct and incorrect IDNs.

Comment 6

[Neil Harris]

Attachment: new, more lenient version of patch to allow for SRV RRs

In newer RFCs, SRV RRs with names _starting with_ underscores are now used.
Changed patch to be more lenient to reflect this usage. Kept the hyphen
restriction, since this is still necessary to prevent Unix command-line option
ambiguity.

Comment 7

[Darin Fisher]

It seems to me that the proposed patch is somewhat similar to what is currently implemented.  See net_IsValidHostName in nsURLHelper.cpp.  The differences are that net_IsValidHostName checks against a "black-list" of characters, whereas this new code checks against a "white-list."  Also, the existing code allows '-' characters to appear anywhere in the hostname.  I think it would make sense to unify the code in these two locations.  If it is really desirable to add code to the punycode generator, then it should be the same code that is executed by net_IsValidHostName.

Comment 8

[Darin Fisher]

In summary, I don't think this bug needs to be kept private.  This bug doesn't reveal any attack vectors AFAICT since net_IsValidHostName exists in the latest versions of our shipping products.

Comment 9

[Darin Fisher]

Not going to block for FF2.

Comment 10

[Neil Harris]

I'm now considering a new approach to the issues addressed in this bug, so I've marked both of the patches above as obsolete. 

Please see bug 355181 for an improvement to net_IsValidHostName that catches several of the cases mentioned here, and is generally more robust: however, it's not a complete solution to this bug. Another reason for not closing this bug just yet is that investigating has raised an number of other interesting issues with the current DNS code, which will probably get their own bugs filed in due course.

Comment 11

[Darin Fisher]

-> reassign to default owner

Comment 12

[Nochum Sossonko [:Natch]]

Requesting blocking-1.9.2? just because of the blocking-1.9a1 flag...