Last Comment Bug 479413 - (CVE-2009-1834) SSL spoofing using IDN characters that appear like spaces on Windows (invisible)
(CVE-2009-1834)
: SSL spoofing using IDN characters that appear like spaces on Windows (invisible)
Status: RESOLVED FIXED
[sg:moderate spoof]
: fixed1.9.1, verified1.9.0.11
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86 Windows Vista
: P2 normal (vote)
: mozilla1.9.1
Assigned To: Simon Montagu :smontagu
:
:
Mentors:
http://tinyurl.com/am8gjy
Depends on:
Blocks: 480450
  Show dependency treegraph
 
Reported: 2009-02-20 04:15 PST by Pavel Cvrcek [:JasnaPaka]
Modified: 2009-06-11 15:18 PDT (History)
20 users (show)
jst: blocking1.9.1+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x+
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot (22.69 KB, image/png)
2009-02-26 12:01 PST, Pavel Cvrcek [:JasnaPaka]
no flags Details
Patch (3.32 KB, patch)
2009-03-05 04:11 PST, Simon Montagu :smontagu
cbiesinger: superreview+
Details | Diff | Splinter Review
patch v.2 (14.04 KB, patch)
2009-03-19 08:40 PDT, Simon Montagu :smontagu
jduell.mcbugs: review+
jst: superreview+
dveditz: approval1.9.0.11+
Details | Diff | Splinter Review
Patch for 1.8 branch (40.76 KB, patch)
2009-06-03 03:10 PDT, Simon Montagu :smontagu
smontagu: approval1.8.1.next?
Details | Diff | Splinter Review

Description Pavel Cvrcek [:JasnaPaka] 2009-02-20 04:15:26 PST
All important is described on URL: http://tinyurl.com/am8gjy
Comment 1 Jesse Ruderman 2009-02-26 02:50:04 PST
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 Jesse Ruderman 2009-02-26 02:51:05 PST
FWIW, the tinyURL redirects to:
https://www.google.xn--com-edoaaaaaaaaaaaaaaaaaaaaaaaaaaaa.phreedom.org/
Comment 3 Pavel Cvrcek [:JasnaPaka] 2009-02-26 11:59:34 PST
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.
Comment 4 Pavel Cvrcek [:JasnaPaka] 2009-02-26 12:01:13 PST
Created attachment 364356 [details]
Screenshot
Comment 5 Dão Gottwald [:dao] 2009-02-26 15:17:29 PST
Seems like \u1158 needs to be added to the IDN blacklist?
Comment 6 Jesse Ruderman 2009-02-26 15:52:20 PST
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 Jesse Ruderman 2009-02-26 15:52:51 PST
Why is it invisible on Windows?
Comment 8 Dão Gottwald [:dao] 2009-02-26 16:02:56 PST
I also see the hexbox on XP.
Comment 9 Karl Tomlinson (:karlt) 2009-02-26 16:29:25 PST
batang.ttc and gulim.ttc (on Vista by default) provide blank glyphs for U+115A-115E.
Comment 10 Jesse Ruderman 2009-02-26 16:40:49 PST
Filed bug 480450 for creating tests capable of catching this kind of issue.
Comment 11 Jonathan Kew (:jfkthame) 2009-02-26 16:49:30 PST
(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.
Comment 12 Karl Tomlinson (:karlt) 2009-02-26 19:08:04 PST
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.
Comment 13 Simon Montagu :smontagu 2009-02-27 05:19:02 PST
(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.
Comment 14 Simon Montagu :smontagu 2009-02-27 05:21:14 PST
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 Masahiro YAMADA 2009-02-27 07:38:46 PST
I can reproduce same problem on Windows XP SP3 / Japanase.
And, "Security" pane in Page info is also spoofed.
Comment 16 Masahiro YAMADA 2009-02-27 07:58:55 PST
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?
Comment 17 Daniel Veditz [:dveditz] 2009-02-27 08:00:56 PST
(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 Masahiro YAMADA 2009-03-01 07:03:01 PST
I filed bug 480818 for problem of Comment #16.
Comment 19 Johnny Stenback (:jst, jst@mozilla.com) 2009-03-04 15:47:37 PST
Simon, is this something you can work on? If not, please let me know. I think we should block on this.
Comment 20 Simon Montagu :smontagu 2009-03-04 23:48:03 PST
(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.
Comment 21 Simon Montagu :smontagu 2009-03-05 04:11:56 PST
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?
Comment 22 Karl Tomlinson (:karlt) 2009-03-05 12:00:24 PST
(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.)
Comment 23 Jason Duell [:jduell] (needinfo me) 2009-03-18 13:44:43 PDT
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.
Comment 24 Simon Montagu :smontagu 2009-03-19 08:40:29 PDT
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.
Comment 25 Jason Duell [:jduell] (needinfo me) 2009-03-25 11:44:14 PDT
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.
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2009-03-25 17:23:19 PDT
Comment on attachment 368266 [details] [diff] [review]
patch v.2

Looks good. sr=jst
Comment 27 Simon Montagu :smontagu 2009-03-29 00:58:24 PDT
http://hg.mozilla.org/mozilla-central/rev/689d75970aeb
Comment 28 Simon Montagu :smontagu 2009-04-01 06:59:39 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c251ab25e061
Comment 29 Daniel Veditz [:dveditz] 2009-04-13 15:31:35 PDT
Comment on attachment 368266 [details] [diff] [review]
patch v.2

Approved for 1.9.0.10, a=dveditz for release-drivers
Comment 30 Al Billings [:abillings] 2009-05-11 17:09:14 PDT
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).
Comment 31 Daniel Veditz [:dveditz] 2009-05-29 10:29:20 PDT
This appears to affect the 1.8 branch as well.
Comment 32 Simon Montagu :smontagu 2009-05-31 07:51:37 PDT
Backporting this to 1.8 will require changing the nsIIDNService interface. Is that OK?
Comment 33 Daniel Veditz [:dveditz] 2009-05-31 14:06:32 PDT
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
Comment 34 Simon Montagu :smontagu 2009-06-03 03:10:59 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.