Last Comment Bug 601429 - (CVE-2010-3770) x-mac-arabic, x-mac-farsi and x-mac-hebrew are vulnerable to XSS
(CVE-2010-3770)
: x-mac-arabic, x-mac-farsi and x-mac-hebrew are vulnerable to XSS
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.9.0.20
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
: Makoto Kato [:m_kato]
Mentors:
https://twitter.com/hasegawayosuke/st...
Depends on: 603423
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-02 20:18 PDT by Masatoshi Kimura [:emk]
Modified: 2011-02-27 22:02 PST (History)
10 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
-
.13-fixed
-
.16-fixed


Attachments
Patch part 1: add API to return charset type flags (61.84 KB, patch)
2010-10-07 14:38 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch part 2: implement a flag for non-websafe charsets (26.48 KB, patch)
2010-10-07 14:53 PDT, Simon Montagu :smontagu
VYV03354: review-
Details | Diff | Splinter Review
Simpler patch (40.08 KB, patch)
2010-10-11 13:27 PDT, Simon Montagu :smontagu
VYV03354: review+
dveditz: superreview+
Details | Diff | Splinter Review
patch for 1.9.2. branch (31.61 KB, patch)
2010-11-15 09:40 PST, Simon Montagu :smontagu
VYV03354: review+
dveditz: superreview+
christian: approval1.9.2.13+
christian: approval1.9.1.16+
Details | Diff | Splinter Review
Backport for 1.9.0 (11.35 KB, patch)
2010-12-08 09:09 PST, Mike Hommey [:glandium]
dveditz: approval1.9.0.next+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2010-10-02 20:18:54 PDT
x-mac-farsi exploit:
<meta charset="x-mac-farsi">シscript セalert(1)//シ/script セ
I don't mark this bug as security sensitive because the exploit is already public (see the URL).
Comment 1 Masatoshi Kimura [:emk] 2010-10-02 20:30:29 PDT
Are those charsets even required? No other browsers support them.
Also, Mac encodings was proposed to remove far back (bug 203838).
Comment 2 Masatoshi Kimura [:emk] 2010-10-02 21:09:00 PDT
Urr, GFX seems to use Mac encodings to decode Macintosh font names...
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontUtils.cpp#1504
Comment 3 Simon Montagu :smontagu 2010-10-03 01:03:22 PDT
If we really still need the Mac encodings for localized font name tables, we can probably get away with undefining all codepoints that decode to ASCII values.
Comment 4 John Daggett (:jtd) 2010-10-03 06:19:35 PDT
(In reply to comment #2)
> Urr, GFX seems to use Mac encodings to decode Macintosh font names...
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontUtils.cpp#1504

I don't think this relates, these are encodings of name table data within font, they are *not* related to content.  Could you explain a little more why this is a problem?
Comment 5 Simon Montagu :smontagu 2010-10-03 07:03:01 PDT
We don't have a mechanism for only using nsIUnicodeDecoder decoders internally and not exposing them to content. Another alternative would be to reimplement the conversion for gfx using character sinks (as I did for UTF-7 in bug 587475).
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2010-10-03 19:34:50 PDT
> We don't have a mechanism 

We should create one, imo...
Comment 7 Daniel Veditz [:dveditz] 2010-10-04 10:46:02 PDT
We really should have a mechanism for internal-only (en|de)coders, would have saved us a lot of grief with utf-7 and imap folders. We can't really afford to do a patch like bug 587475 each time we run into this.

But why are those characters interpreted as standard angle brackets? Seems like a very unfortunate conversion, but I admit I know nothing of those encodings.

Is this really [sg:high]? If a page is using these charsets that only work in Firefox it's a bit of a self-inflicted problem with a work-around. As far as I know we've now fixed the bugs that allowed attackers to force pages on other sites to use an encoding of the attacker's choosing.
Comment 8 Masatoshi Kimura [:emk] 2010-10-04 15:32:42 PDT
(In reply to comment #7)
> why are those characters interpreted as standard angle brackets? Seems like
> a very unfortunate conversion, but I admit I know nothing of those encodings.

These encodings will convert 0x3C to <LR>+U+003C and 0xBC to <RL>+U+003C. Mac RTL encodings do not have a separate RTL control characters.
Comment 9 Simon Montagu :smontagu 2010-10-07 14:38:41 PDT
Created attachment 481645 [details] [diff] [review]
Patch part 1: add API to return charset type flags

This is designed to be extendable, so that we can leverage it for other things, such as bug 475351.
Comment 10 Simon Montagu :smontagu 2010-10-07 14:53:43 PDT
Created attachment 481653 [details] [diff] [review]
Patch part 2: implement a flag for non-websafe charsets

I didn't include UTF-7 in this yet, so as not to break mailnews.
Comment 11 Masatoshi Kimura [:emk] 2010-10-07 17:54:29 PDT
Comment on attachment 481645 [details] [diff] [review]
Patch part 1: add API to return charset type flags

> diff --git a/intl/uconv/util/nsUCConstructors.h b/intl/uconv/util/nsUCConstructors.h
> +                        PRUint32 aCharsetType,
> +                   PRUint32 aCharsetType,
> +                     PRUint32 aCharsetType,
nit: Why is the argument name not "aFlags" only in this file?
Comment 12 Masatoshi Kimura [:emk] 2010-10-07 17:56:34 PDT
Comment on attachment 481653 [details] [diff] [review]
Patch part 2: implement a flag for non-websafe charsets

> +  if ((*aResult)->IsCharsetType(CHARSET_FOR_INTERNAL_USE_ONLY))
> +    return NS_ERROR_UCONV_NOCONV;
Will *aResult be released automatically? Caller will think *aResult do not contain a valid pointer.

> +// Support for this charset was removed in bug 601429, so it should fall back
> +// to default.
I think there should be a way to test charsets for internal use only. Otherwise we will not notice when gfx is broken silently.
Comment 13 Simon Montagu :smontagu 2010-10-08 01:39:46 PDT
> I think there should be a way to test charsets for internal use only. Otherwise
> we will not notice when gfx is broken silently.

Good point. In general there should probably be an option to use the internal-only charsets from nsIScriptableUnicodeConverter. I'll add this.
Comment 14 Masatoshi Kimura [:emk] 2010-10-08 09:09:19 PDT
Also, internal charsets still appears above UTF-16 and on charset menu cache area.
You need to set "notForBrowser" property for internal charsets. Or internal charsets should not be enumerated by default.
Comment 15 Simon Montagu :smontagu 2010-10-11 13:27:45 PDT
Created attachment 482325 [details] [diff] [review]
Simpler patch

I'm glad you mentioned .notForBrowser: I realized that we can add another property to charsetData.properties and do this much more simply.

I gave the charsets the .notForBrowser property anyway so as to avoid two calls to RemoveFlaggedCharsets in nsCharsetMenu.cpp
Comment 17 Daniel Veditz [:dveditz] 2010-10-11 14:24:20 PDT
Was there an answer to the last paragraph of comment 7? Can an attacker coerce a site into using these charsets?
Comment 18 Simon Montagu :smontagu 2010-10-12 10:24:45 PDT
Comment on attachment 482325 [details] [diff] [review]
Simpler patch

Requesting sr for API changes
Comment 20 Simon Montagu :smontagu 2010-10-14 02:27:01 PDT
This patch is unlikely to be accepted for branches, right? Maybe we can band-aid around it as suggested in comment 3.
Comment 21 Masatoshi Kimura [:emk] 2010-10-14 04:41:06 PDT
We can just remove encodings for the 191 branch (gfx didn't use uconv there).
However we will need a workaround for the 192 branch.
Comment 22 Daniel Veditz [:dveditz] 2010-11-04 10:52:09 PDT
Comment on attachment 482325 [details] [diff] [review]
Simpler patch

>+     * The "Internal" version will return a decoder for any charset; the others
>+     *  will return NS_ERROR_UCONV_NOCONV if the requested charsets is
>+     *  vulnerable to XSS attacks and should not be used with untrusted input
>+    nsIUnicodeDecoder getUnicodeDecoderInternal(in string charset);
>+    nsIUnicodeDecoder getUnicodeDecoderRawInternal(in string charset);

I'm fine with this, but is there a chance that in the future you'll add other bits or want to restrict the result set? Instead of adding a new method to essentially pass in one bit of information you could add a form with an extra flags parameter for various features you want (and define some standard combined flags). Just a thought, maybe should go in a new bug after we fix this one.


> x-imap4-modified-utf7.notForBrowser = true
> utf-7.notForBrowser                     = true
>+x-mac-arabic.notForBrowser              = true
>+x-mac-farsi.notForBrowser               = true
>+x-mac-hebrew.notForBrowser              = true
>+
>+x-mac-arabic.isXSSVulnerable            = true
>+x-mac-farsi.isXSSVulnerable             = true
>+x-mac-hebrew.isXSSVulnerable            = true

Should utf-7 and x-imap4-modified-utf7 also get the isXSSVulnerable flag? I suppose not that important if bug 414064 is a blocker, but is that one (which also requires bug 587475 for comm-central) going to be back-ported to the branches? If setting the property works that should be good enough, right?

sr=dveditz

I don't see any reason not to take this fix on the older branches. We'd have to avoid changing the existing interface (it's public, right?) and instead add a nsICharsetConverterManager_1_9_BRANCH subclass to hold the new entry points.

[Half a dozen addons appear to use nsICharsetConverterManager at a quick glance. All from JavaScript so the extra entry points wouldn't get in their way, but I'm unable to scan any binary addons and have no visibility into toolbars not hosted at AMO. Better safe than sorry, we've been made sorry more than once.]
Comment 23 Simon Montagu :smontagu 2010-11-07 01:59:21 PDT
http://hg.mozilla.org/mozilla-central/rev/97aca07b7f32
Comment 24 Axel Hecht [:Pike] 2010-11-07 03:37:20 PST
Should we have follow ups for l10n?
Comment 25 Simon Montagu :smontagu 2010-11-08 02:59:42 PST
Patch was backed out, and relanded in http://hg.mozilla.org/mozilla-central/rev/21d0486d955f
Comment 26 Simon Montagu :smontagu 2010-11-09 04:47:45 PST
(In reply to comment #24)
> Should we have follow ups for l10n?

Bug 610638
Comment 27 Simon Montagu :smontagu 2010-11-09 09:03:11 PST
(In reply to comment #22)
> Instead of adding a new method to
> essentially pass in one bit of information you could add a form with an extra
> flags parameter for various features you want (and define some standard
> combined flags). Just a thought, maybe should go in a new bug after we fix this
> one.

I like this idea, but I think I'll hold off on it until after 2.0. I might move all the existing flags over to the mechanism from the previous patch at the same time. I'll test for performance impact before making a decision on this.

> Should utf-7 and x-imap4-modified-utf7 also get the isXSSVulnerable flag? I
> suppose not that important if bug 414064 is a blocker, but is that one (which
> also requires bug 587475 for comm-central) going to be back-ported to the
> branches? If setting the property works that should be good enough, right?

Yes, I plan to do this instead of the current patch for bug 414064, and remove the dependency on bug 587475. comm-central will still need a patch (which is why I didn't do this immediately) but it will be a whole lot less big and scary.

> I don't see any reason not to take this fix on the older branches.

What about the l10n impact?
Comment 28 Daniel Veditz [:dveditz] 2010-11-11 18:46:22 PST
What happens if you leave those strings on the menu on the branches? They just won't work, right? I can live with that, dunno what Axel thinks.
Comment 29 Axel Hecht [:Pike] 2010-11-12 02:05:50 PST
The radical path would be to backport bug 610638 and to make the menus non-localizable. As Simon lays out in that bug, most changes to those strings are actually bugs. If we let the entries in intl.properties just there and unused, we may very well just win.
Comment 30 Simon Montagu :smontagu 2010-11-12 04:06:57 PST
In fact I think we can just leave intl.properties unchanged, and the entries will automagically disappear from the menu because of the ".notForBrowser" flag. This will either Just Work or will need a line or two of code in nsCharset.cpp.

Right now I'm a little stuck with the nsICharsetConverterManager_1_9_BRANCH subclassing, but when I get that sorted out we should be ready to go.
Comment 31 Simon Montagu :smontagu 2010-11-15 09:40:27 PST
Created attachment 490593 [details] [diff] [review]
patch for 1.9.2. branch
Comment 32 Daniel Veditz [:dveditz] 2010-11-15 15:45:26 PST
Comment on attachment 490593 [details] [diff] [review]
patch for 1.9.2. branch

sr=dveditz
Comment 33 christian 2010-11-17 10:35:35 PST
Comment on attachment 490593 [details] [diff] [review]
patch for 1.9.2. branch

a=LegNeato for 1.9.2.13 and 1.9.1.16
Comment 35 Mike Hommey [:glandium] 2010-12-08 09:09:41 PST
Created attachment 496164 [details] [diff] [review]
Backport for 1.9.0

Only difference is in nsScanner::SetDocumentCharset
Comment 36 Daniel Veditz [:dveditz] 2010-12-09 13:56:42 PST
Comment on attachment 496164 [details] [diff] [review]
Backport for 1.9.0

Approved for 1.9.0.next, a=dveditz, should someone want to land this.

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