40.08 KB, patch
|Details | Diff | Splinter Review|
31.61 KB, patch
|Details | Diff | Splinter Review|
11.35 KB, patch
|Details | Diff | Splinter Review|
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).
Are those charsets even required? No other browsers support them. Also, Mac encodings was proposed to remove far back (bug 203838).
Urr, GFX seems to use Mac encodings to decode Macintosh font names... http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFontUtils.cpp#1504
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.
(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?
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).
> 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.
(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.
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.
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 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 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.
> 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.
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.
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
Was there an answer to the last paragraph of comment 7? Can an attacker coerce a site into using these charsets?
Comment on attachment 482325 [details] [diff] [review] Simpler patch Requesting sr for API changes
This patch is unlikely to be accepted for branches, right? Maybe we can band-aid around it as suggested in comment 3.
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.
Should we have follow ups for l10n?
Patch was backed out, and relanded in http://hg.mozilla.org/mozilla-central/rev/21d0486d955f
(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.
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.
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.
Created attachment 490593 [details] [diff] [review] patch for 1.9.2. branch
Comment on attachment 490593 [details] [diff] [review] patch for 1.9.2. branch sr=dveditz
Comment on attachment 490593 [details] [diff] [review] patch for 1.9.2. branch a=LegNeato for 22.214.171.124 and 126.96.36.199
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.
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