Charset XSS vulnerability property check is not thread-safe but used from multiple threads

RESOLVED INVALID

Status

MailNews Core
Internationalization
--
critical
RESOLVED INVALID
6 years ago
a year ago

People

(Reporter: hsivonen, Unassigned)

Tracking

({sec-moderate})

Firefox Tracking Flags

(Not tracked)

Details

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.
Marking moderate, can reassess if we get a test case.
Keywords: sec-moderate
Bug 943268 removed nsCharsetConverterManager and nsCharsetAlias.
Does the code that replaced them also have issues?
Flags: needinfo?(hsivonen)
(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.
Component: Internationalization → Internationalization
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
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

3 years ago
Group: core-security → mail-core-security

Comment 5

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

Comment 6

2 years ago
Sorry, I don't know what this is about.
Flags: needinfo?(mozilla)

Comment 7

2 years ago
I don't really know either.
Flags: needinfo?(mkmelin+mozilla)
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

2 years ago
So there is no objection to closing this bug?
(In reply to Kent James (:rkent) from comment #9)
> So there is no objection to closing this bug?

apparently none
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INVALID
Group: mail-core-security
You need to log in before you can comment on or make changes to this bug.