Bug 479413 (CVE-2009-1834)

SSL spoofing using IDN characters that appear like spaces on Windows (invisible)

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
Networking
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: JasnaPaka, Assigned: smontagu)

Tracking

(Blocks: 1 bug, {fixed1.9.1, verified1.9.0.11})

unspecified
mozilla1.9.1
x86
Windows Vista
fixed1.9.1, verified1.9.0.11
Points:
---
Bug Flags:
blocking1.9.1 +
wanted1.9.0.x +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate spoof], URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
All important is described on URL: http://tinyurl.com/am8gjy

Comment 1

8 years ago
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre

I see

www.google.comOOOOOOOOOOOOOOOOOOOOO.phreedom.org

where each O is a "1158" hexbox.  Do you see hexboxes or something different?

Comment 2

8 years ago
FWIW, the tinyURL redirects to:
https://www.google.xn--com-edoaaaaaaaaaaaaaaaaaaaaaaaaaaaa.phreedom.org/
(Reporter)

Comment 3

8 years ago
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090224 Minefield/3.2a1pre (.NET CLR 3.5.30729)

Still see the same problem. I will attach screenshot.
(Reporter)

Comment 4

8 years ago
Created attachment 364356 [details]
Screenshot

Updated

8 years ago
Blocks: 425480
Flags: blocking-firefox3.1?
Summary: SSL site spoofing using an IDN homograph attack → SSL spoofing using IDN characters that appear like spaces on Windows (invisible)
Whiteboard: [sg:moderate spoof]

Updated

8 years ago
No longer blocks: 425480
Component: Security → Networking
Flags: blocking-firefox3.1?
Product: Firefox → Core
QA Contact: firefox → networking
Seems like \u1158 needs to be added to the IDN blacklist?
Flags: blocking1.9.1?

Comment 6

8 years ago
Sorry, it's 115A, not 1158.  And it doesn't make sense to blacklist it by itself, since it's one of several invalid unicode characters near each other:

http://www.fileformat.info/info/unicode/char/115b/index.htm

Comment 7

8 years ago
Why is it invisible on Windows?
I also see the hexbox on XP.

Updated

8 years ago
Component: Networking → Layout: Text
QA Contact: networking → layout.fonts-and-text
batang.ttc and gulim.ttc (on Vista by default) provide blank glyphs for U+115A-115E.
Component: Layout: Text → GFX: Thebes
QA Contact: layout.fonts-and-text → thebes

Updated

8 years ago
Blocks: 480450

Comment 10

8 years ago
Filed bug 480450 for creating tests capable of catching this kind of issue.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #9)
> batang.ttc and gulim.ttc (on Vista by default) provide blank glyphs for
> U+115A-115E.

I see the blanks on Mac OS X 10.5, but I believe this is using fonts that I
have installed personally, not ones shipped with the system by default.

It's a long time since I looked at IDN rules, but I'm surprised unassigned
Unicode characters would be permitted at all.
http://tools.ietf.org/html/rfc3490#section-4

   1) Decide whether the domain name is a "stored string" or a "query
      string" as described in [STRINGPREP].  If this conversion follows
      the "queries" rule from [STRINGPREP], set the flag called
      "AllowUnassigned".


http://tools.ietf.org/html/rfc3454#section-7

   In general, "stored strings"
   are strings that are used in protocol identifiers and named entities,
   such as names in digital certificates and DNS domain name parts.


   All code points not assigned in the character repertoire named in a
   stringprep profile are called "unassigned code points".  Stored
   strings using the profile MUST NOT contain any unassigned code
   points.  Queries for matching strings MAY contain unassigned code
   points.  


So existing domain name parts should not contain unassigned code points.

It seems that this would be a sensible verification to perform.
Even though the verification can only use the latest know version of Unicode,
the worst case is that a domain name using a newer version of Unicode is
presented in its ASCII/Punycode form.

If the verification is not performed, a new Unicode assignment may add further
space or control characters or other characters that should be prohibited, and
fonts may appropriately provide support for these characters.  It wouldn't be
sensible to rely on the font system to ensure that all currently unknown
characters are visible, or always look different from known characters.
Component: GFX: Thebes → Networking
QA Contact: thebes → networking
(Assignee)

Comment 13

8 years ago
(In reply to comment #12)

> It seems that this would be a sensible verification to perform.
> Even though the verification can only use the latest know version of Unicode,
> the worst case is that a domain name using a newer version of Unicode is
> presented in its ASCII/Punycode form.

There is already an API to do this, idn_nameprep_isunassigned, which we could presumably add to the checks in nsIDNService::stringPrep. I'm not sure how or where the distinction between stored string and queries exists in our code.
(Assignee)

Comment 14

8 years ago
Oh, and the data tables used by nsIDNService deliberately don't get updated when we update Unicode support to the latest version, so we would be using the version of Unicode specified in the IDN RFCs.

Comment 15

8 years ago
I can reproduce same problem on Windows XP SP3 / Japanase.
And, "Security" pane in Page info is also spoofed.

Comment 16

8 years ago
At Windows, title bar of Cert viewer shows incorrect site name when seeing certficate of www.google.xn--com-edoaaaaaaaaaaaaaaaaaaaaaaaaaaaa.phreedom.org .
(U+11a5 becomes to "Latin small letter a with acute" and "middle single dot").
Should I file it as another bug?
(In reply to comment #14)
> so we would be using the version of Unicode specified in the IDN RFCs.

fwiw those rfc's are getting updated (bug 479520) but even using the old list would be an improvement.

Comment 18

8 years ago
I filed bug 480818 for problem of Comment #16.
Simon, is this something you can work on? If not, please let me know. I think we should block on this.
Assignee: nobody → smontagu
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
(Assignee)

Comment 20

8 years ago
(In reply to comment #19)
> Simon, is this something you can work on? If not, please let me know. I think
> we should block on this.

Yes, taking.
Status: NEW → ASSIGNED
(Assignee)

Comment 21

8 years ago
Created attachment 365641 [details] [diff] [review]
Patch

I think this is sufficient to fix the bug. Are there any other consequences of stringPrep() failing apart from forcing display of the URL in punycode?
Attachment #365641 - Flags: superreview?
Attachment #365641 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #365641 - Flags: superreview?(cbiesinger)
Attachment #365641 - Flags: superreview?
Attachment #365641 - Flags: review?(cbiesinger)
Attachment #365641 - Flags: review?
(In reply to comment #21)
> Are there any other consequences of stringPrep() failing apart from forcing
> display of the URL in punycode?

I'm assuming that conversion of a user provided domain in Unicode form also passes through this code.  That would be consistent with the IDNA ToAscii transformation, http://tools.ietf.org/html/rfc3490#section-4.1 .
But a user provided domain would be a "query string" so the "AllowUnassigned" flag should be true in that case.  However, our code doesn't have the AllowUnassigned flag.

What do you think about doing the unassigned check only in nsIDNService::ConvertToDisplayIDN?
(Or perhaps even nsIDNService::isInWhitelist.)
Attachment #365641 - Flags: review?(cbiesinger) → review?(jduell.mcbugs)
Flags: wanted1.9.0.x+
Attachment #365641 - Flags: superreview?(cbiesinger) → superreview+
Comment on attachment 365641 [details] [diff] [review]
Patch

So the unit test seems to set the "network.IDN.whitelist.com" pref to false if it's not already set?  Just want to make sure that's what we want (assuming I'm reading the code correctly).

The actual code patch seems logical, but I'm waiting for answer comment #22 before approving review.
(Assignee)

Comment 24

8 years ago
Created attachment 368266 [details] [diff] [review]
patch v.2

So the answer to comment 22 is that Karl is right, and we need to do the unassigned check only when converting for display.
Attachment #365641 - Attachment is obsolete: true
Attachment #368266 - Flags: review?(jduell.mcbugs)
Attachment #365641 - Flags: review?(jduell.mcbugs)
Comment on attachment 368266 [details] [diff] [review]
patch v.2

My apologies for taking so long to review--I didn't notice you had a new patch attached to your last comment.

Patch looks good.  Handing off for SR to JST, who should be able to either approve or say who should take it.
Attachment #368266 - Flags: superreview?(jst)
Attachment #368266 - Flags: review?(jduell.mcbugs)
Attachment #368266 - Flags: review+
Comment on attachment 368266 [details] [diff] [review]
patch v.2

Looks good. sr=jst
Attachment #368266 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 27

8 years ago
http://hg.mozilla.org/mozilla-central/rev/689d75970aeb
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 28

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c251ab25e061
Keywords: fixed1.9.1
(Assignee)

Updated

8 years ago
Attachment #368266 - Flags: approval1.9.0.9?
Attachment #368266 - Flags: approval1.9.0.9? → approval1.9.0.10?
Comment on attachment 368266 [details] [diff] [review]
patch v.2

Approved for 1.9.0.10, a=dveditz for release-drivers
Attachment #368266 - Flags: approval1.9.0.10? → approval1.9.0.10+
(Assignee)

Updated

8 years ago
Keywords: fixed1.9.0.10
Verified fixed in 1.9.0.11 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11pre) Gecko/2009051105 GranParadiso/3.0.11pre (.NET CLR 3.5.30729).
Keywords: fixed1.9.0.11 → verified1.9.0.11
This appears to affect the 1.8 branch as well.
Flags: wanted1.8.1.x+
(Assignee)

Comment 32

8 years ago
Backporting this to 1.8 will require changing the nsIIDNService interface. Is that OK?
No, we require that published IDL files not change because we've had bad cases where extensions or other apps depended on them unbeknownst to us. The standard workaround is a new iface inheriting from the old:

  interface nsIIDNService_MOZILLA_1_8_BRANCH : nsIIDNService

Or even just a separate interface to the object if you're not simply adding methods. Either is fine, whichever is easier in the given situation.

Note that when we're strict something that was already extended in 2.0.0.x needs yet another interface when modified again in 2.0.0.y. Note poor nsIDocShell is up to _BRANCH3:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsIDocShell.idl&rev=1.82.2.12&mark=454#454

nsIDocShell didn't bother to inherit, nsIXPConnect did:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/idl/nsIXPConnect.idl&rev=1.35.4.10&mark=725#725
(Assignee)

Comment 34

8 years ago
Created attachment 381260 [details] [diff] [review]
Patch for 1.8 branch

This growed a bit... I took the interface change over from bug 402008, which I thought would be simpler and safer than trying to reinvent it, and then had to include bug 427957 as well, since it fixed a regression from bug 402008.
Attachment #381260 - Flags: approval1.8.1.next?
Alias: CVE-2009-1834
You need to log in before you can comment on or make changes to this bug.