Closed
Bug 795241
Opened 12 years ago
Closed 7 years ago
Charset XSS vulnerability property check is not thread-safe but used from multiple threads
Categories
(MailNews Core :: Internationalization, defect)
MailNews Core
Internationalization
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: hsivonen, Unassigned)
Details
(Keywords: sec-moderate)
nsCharsetAlias.cpp is used from the main thread (various places) and from the HTML parser thread. nsCharsetAlias.cpp checks for the XSS vulnerability property of a charset by calling into nsCharsetConverterManager: http://mxr.mozilla.org/mozilla-central/source/intl/locale/src/nsCharsetAlias.cpp#54 Internally, this check checks a property: http://mxr.mozilla.org/mozilla-central/source/intl/uconv/src/nsCharsetConverterManager.cpp#119 And the property check ends up loading a property file lazily: http://mxr.mozilla.org/mozilla-central/source/intl/uconv/src/nsCharsetConverterManager.cpp#99 - - Previously, when I made the alias data itself thread-safe, I made the build system convert the relevant .properties files into .h files so that the data ends up statically in the data segment of the object code. See: http://mxr.mozilla.org/mozilla-central/source/intl/locale/src/props2arrays.py I suggest using the same method to bake charsetData.properties into the data segment so that the dynamic linker takes care of its safe loading. See also bug 493981. Filing as security-sensitive just in case.
Comment 1•12 years ago
|
||
Marking moderate, can reassess if we get a test case.
Keywords: sec-moderate
Comment 2•10 years ago
|
||
Bug 943268 removed nsCharsetConverterManager and nsCharsetAlias. Does the code that replaced them also have issues?
Flags: needinfo?(hsivonen)
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #2) > Bug 943268 removed nsCharsetConverterManager and nsCharsetAlias. > Does the code that replaced them also have issues? No, but the old bogus code moved to c-c, so this became a c-c bug.
Flags: needinfo?(hsivonen)
Product: Core → MailNews Core
Summary: Charset XSS vulnerability property check in not thread-safe but used from multiple threads → Charset XSS vulnerability property check is not thread-safe but used from multiple threads
Comment 4•10 years ago
|
||
Said code is now only called via nsCharsetConverterManager itself: <http://dxr.mozilla.org/comm-central/search?q=regexp%3A%22GetPreferred\b%22&case=true>. I don't think this is an issue anymore, especially since comm-central itself doesn't make much of a distinction between internal and non-internal charsets (since we need to be able to handle UTF-7 incoming email, most of our charset checks have to accept internal charsets--of course this may be a minor security bug in and of itself).
Updated•9 years ago
|
Group: core-security → mail-core-security
Comment 5•8 years ago
|
||
(In reply to Joshua Cranmer [:jcranmer] from comment #4) > Said code is now only called via nsCharsetConverterManager itself: > <http://dxr.mozilla.org/comm-central/ > search?q=regexp%3A%22GetPreferred\b%22&case=true>. I don't think this is an > issue anymore, especially since comm-central itself doesn't make much of a > distinction between internal and non-internal charsets (since we need to be > able to handle UTF-7 incoming email, most of our charset checks have to > accept internal charsets--of course this may be a minor security bug in and > of itself). jorg, mangnus, do you agree with this assessment? And, is it still a security issue?
Flags: needinfo?(mozilla)
Flags: needinfo?(mkmelin+mozilla)
Reporter | ||
Comment 8•8 years ago
|
||
This code is no longer used by the HTML parser (off the main thread), so if all these https://mxr.mozilla.org/comm-central/ident?i=nsICharsetConverterManager are on the main thread, this shouldn't be a security problem anymore.
Comment 9•8 years ago
|
||
So there is no objection to closing this bug?
Comment 10•7 years ago
|
||
(In reply to Kent James (:rkent) from comment #9) > So there is no objection to closing this bug? apparently none
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Updated•7 years ago
|
Group: mail-core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•