601429 - (CVE-2010-3770) x-mac-arabic, x-mac-farsi and x-mac-hebrew are vulnerable to XSS

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Masatoshi Kimura [:emk]]

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]]

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]]

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]

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)]

(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]

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 [:bzbarsky]]

> We don't have a mechanism 

We should create one, imo...

Comment 7

[Daniel Veditz [:dveditz]]

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]]

(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]

Attachment: 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]

Attachment: 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]]

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]]

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]

> 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]]

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]

Attachment: 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]]

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]

Comment on attachment 482325 [details] [diff] [review]
Simpler patch

Requesting sr for API changes

Comment 20

[Simon Montagu :smontagu]

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]]

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]]

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]

http://hg.mozilla.org/mozilla-central/rev/97aca07b7f32

Comment 24

[Axel Hecht [:Pike]]

Should we have follow ups for l10n?

Comment 25

[Simon Montagu :smontagu]

Patch was backed out, and relanded in http://hg.mozilla.org/mozilla-central/rev/21d0486d955f

Comment 26

[Simon Montagu :smontagu]

(In reply to comment #24)
> Should we have follow ups for l10n?

Bug 610638

Comment 27

[Simon Montagu :smontagu]

(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]]

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]]

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]

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]

Attachment: patch for 1.9.2. branch

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 490593 [details] [diff] [review]
patch for 1.9.2. branch

sr=dveditz

Comment 33

[LegNeato]

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 34

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bd25c99ebb74
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/869ddde104b7

Comment 35

[Mike Hommey [:glandium]]

Attachment: Backport for 1.9.0

Only difference is in nsScanner::SetDocumentCharset

Comment 36

[Daniel Veditz [:dveditz]]

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.

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed the 1.9.0 patch:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=CoreTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=alqahira%25ardisson.org&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2011-02-27+21%3A55&maxdate=2011-02-27+21%3A57&cvsroot=%2Fcvsroot