Bug 601429 (CVE-2010-3770)

x-mac-arabic, x-mac-farsi and x-mac-hebrew are vulnerable to XSS

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: emk, Assigned: smontagu)

Tracking

({fixed1.9.0.20})

Trunk
x86
Windows 7
fixed1.9.0.20
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 -, status1.9.2 .13-fixed, blocking1.9.1 -, status1.9.1 .16-fixed)

Details

(Whiteboard: [sg:moderate], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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).
(Reporter)

Comment 1

7 years ago
Are those charsets even required? No other browsers support them.
Also, Mac encodings was proposed to remove far back (bug 203838).
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
(Reporter)

Comment 2

7 years ago
Urr, GFX seems to use Mac encodings to decode Macintosh font names...
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontUtils.cpp#1504
Whiteboard: [sg:high]
Assignee: smontagu → nobody
(Assignee)

Comment 3

7 years ago
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

7 years ago
(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?
(Assignee)

Comment 5

7 years ago
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

7 years ago
> We don't have a mechanism 

We should create one, imo...
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.
(Reporter)

Comment 8

7 years ago
(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.

Updated

7 years ago
blocking1.9.1: ? → -
blocking1.9.2: ? → -
status1.9.1: --- → wanted
status1.9.2: --- → wanted
(Assignee)

Comment 9

7 years ago
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.
Assignee: nobody → smontagu
Attachment #481645 - Flags: review?(VYV03354)
(Assignee)

Comment 10

7 years ago
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.
Attachment #481653 - Flags: review?(VYV03354)
(Reporter)

Comment 11

7 years ago
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?
(Reporter)

Comment 12

7 years ago
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.
(Reporter)

Updated

7 years ago
Attachment #481653 - Flags: review?(VYV03354) → review-
(Assignee)

Comment 13

7 years ago
> 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.
(Reporter)

Comment 14

7 years ago
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.
(Assignee)

Updated

7 years ago
Depends on: 603423
(Assignee)

Comment 15

7 years ago
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
Attachment #481645 - Attachment is obsolete: true
Attachment #481653 - Attachment is obsolete: true
Attachment #482325 - Flags: review?(VYV03354)
Attachment #481645 - Flags: review?(VYV03354)
Was there an answer to the last paragraph of comment 7? Can an attacker coerce a site into using these charsets?
(Reporter)

Updated

7 years ago
Attachment #482325 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 18

7 years ago
Comment on attachment 482325 [details] [diff] [review]
Simpler patch

Requesting sr for API changes
Attachment #482325 - Flags: superreview?(dveditz)
(Assignee)

Comment 20

7 years ago
This patch is unlikely to be accepted for branches, right? Maybe we can band-aid around it as suggested in comment 3.
(Reporter)

Comment 21

7 years ago
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.
blocking2.0: ? → betaN+
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.]
Attachment #482325 - Flags: superreview?(dveditz) → superreview+
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/97aca07b7f32
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 24

7 years ago
Should we have follow ups for l10n?
(Assignee)

Comment 25

7 years ago
Patch was backed out, and relanded in http://hg.mozilla.org/mozilla-central/rev/21d0486d955f
(Assignee)

Comment 26

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

Bug 610638
(Assignee)

Comment 27

7 years ago
(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?
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

7 years ago
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.
(Assignee)

Comment 30

7 years ago
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.
(Assignee)

Comment 31

7 years ago
Created attachment 490593 [details] [diff] [review]
patch for 1.9.2. branch
Attachment #490593 - Flags: superreview?(dveditz)
Attachment #490593 - Flags: review?(VYV03354)
Comment on attachment 490593 [details] [diff] [review]
patch for 1.9.2. branch

sr=dveditz
Attachment #490593 - Flags: superreview?(dveditz) → superreview+
(Reporter)

Updated

7 years ago
Attachment #490593 - Flags: review?(VYV03354) → review+
(Assignee)

Updated

7 years ago
Attachment #490593 - Flags: approval1.9.2.13?
Attachment #490593 - Flags: approval1.9.1.16?
Whiteboard: [sg:high] → [sg:moderate]

Comment 33

7 years ago
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
Attachment #490593 - Flags: approval1.9.2.13?
Attachment #490593 - Flags: approval1.9.2.13+
Attachment #490593 - Flags: approval1.9.1.16?
Attachment #490593 - Flags: approval1.9.1.16+
(Assignee)

Comment 34

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bd25c99ebb74
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/869ddde104b7
status1.9.1: wanted → .16-fixed
status1.9.2: wanted → .13-fixed
Alias: CVE-2010-3770
Created attachment 496164 [details] [diff] [review]
Backport for 1.9.0

Only difference is in nsScanner::SetDocumentCharset
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.
Attachment #496164 - Flags: approval1.9.0.next+
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
Keywords: fixed1.9.0.20
You need to log in before you can comment on or make changes to this bug.