Faulty .properties file results in uninitialized memory being used

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: glazou, Assigned: smontagu)

Tracking

({fixed1.8.1.15})

1.8 Branch
x86
Windows XP
fixed1.8.1.15
Points:
---
Bug Flags:
blocking1.8.1.13 -
blocking1.8.1.15 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] 1.8 branch, requires faulty localization/addon)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 281857 [details]
faulty properties file, bad encoding

I have hit a weird bug writing an extension for Firefox. The extension is localized in both french and english.
One of the french *.properties file was not UTF-8 but ISO8859-encoded.
The window opened by my extension retrieves 3 strings from that properties file.
Surprisingly, the first retrieval (string 'textLayouts' in the attached file)
did not fail, while the two others did fail.
BUT what I got back from that first |GetStringFromName()| call was absolutely not
what's in the properties file but some text coming from a MS Word process also
running on my machine !!! My XUL window was showing me a bit of the text I was
editing in Word...
Fixing the encoding of the file of course resolved the issue.
Could you provide a minimal XUL testcase that shows the problem (when run with privileges)? Running the following code snippet gives me just "Mon":

var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]. getService(Components.interfaces.nsIStringBundleService); var sb = sbs.createBundle("file:///C:/Users/Gavin/Desktop/layout2.properties"); prompt('',sb.GetStringFromName("textLayouts"));
I also don't understand how it could have shown you data from another process - Windows' memory protection should prevent that from happening, and even if it didn't, the odds of our code having read memory from Word in such a way that it then displayed text from the opened Word file is rather low. Are you sure it wasn't a fluke of some sort? Can you reproduce the problem?
(Reporter)

Comment 3

10 years ago
(In reply to comment #1)
> Could you provide a minimal XUL testcase that shows the problem (when run with
> privileges)? Running the following code snippet gives me just "Mon":
> 
> var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"].
> getService(Components.interfaces.nsIStringBundleService); var sb =
> sbs.createBundle("file:///C:/Users/Gavin/Desktop/layout2.properties");
> prompt('',sb.GetStringFromName("textLayouts"));

My code is exactly similar to yours. But I got back a string starting with that
"Mod" (and not "Mon") followed immediately by text coming from the other
process.
I am running 2.0.0.7 on winXP sp2.
(Reporter)

Comment 4

10 years ago
(In reply to comment #2)

> Can you reproduce the problem?

Partly yes. I am not this time seeing text coming from another process
but from the MIT license plate of one of my JS files. That license text is of
course contained in a JS comment... Just to be clear, that JS file is not the
one retrieving the localized strings.
OK, I can reproduce the problem in a branch build (I was testing trunk earlier). 
(Reporter)

Comment 6

10 years ago
Created attachment 282232 [details]
screenshot

Updated

10 years ago
Assignee: dveditz → smontagu
Component: Security → Internationalization
Flags: blocking1.8.1.12?
QA Contact: toolkit → i18n
Summary: Firefox showing data from other win process → Faulty .properties file results in data from other parts of memory being used
Whiteboard: [sg:moderate] 1.8 branch
Thanks for raising the visibility on this one, Jesse, but probably not enough time to fix for the upcoming 2.0.0.12 release.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Flags: blocking1.8.1.13? → blocking1.8.1.13+
(Assignee)

Comment 8

9 years ago
Created attachment 306514 [details] [diff] [review]
Branch patch

This is a deliberately minimal patch for branch so that no property in a property file which currently works will stop working.

For trunk I would rather return an error and fail to load the entire property file if it's not valid UTF-8.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Whiteboard: [sg:moderate] 1.8 branch → [sg:moderate] 1.8 branch, need r=bsmedberg
Comment on attachment 306514 [details] [diff] [review]
Branch patch

I'm not going to review this as a branch patch until an equivalent trunk patch is here with unit tests.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Summary: Faulty .properties file results in data from other parts of memory being used → Faulty .properties file results in uninitialized memory being used
Whiteboard: [sg:moderate] 1.8 branch, need r=bsmedberg → [sg:moderate] 1.8 branch, requires faulty localization/addon, need r=bsmedberg
(Assignee)

Comment 10

9 years ago
Created attachment 307421 [details] [diff] [review]
Trunk patch
Attachment #307421 - Flags: superreview?(benjamin)
Attachment #307421 - Flags: review?(benjamin)
(Assignee)

Comment 11

9 years ago
Comment on attachment 306514 [details] [diff] [review]
Branch patch

I already said in comment 8 that the trunk patch wasn't going to be equivalent. The patch I just attached for trunk rejects the whole properties file if it encounters invalid UTF-8, but on branch I think we should preserve the status quo that properties earlier in the file without encoding errors can still be retrieved.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Moving to the next release since this doesn't yet have reviews.
Flags: blocking1.8.1.14+
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+
Comment on attachment 306514 [details] [diff] [review]
Branch patch

Still want a test for this. Even a binary test in xpcom/tests would help me feel comfortable.
Attachment #306514 - Flags: superreview?(benjamin)
Attachment #306514 - Flags: review?(benjamin)
Attachment #307421 - Flags: superreview?(benjamin)
Attachment #307421 - Flags: superreview+
Attachment #307421 - Flags: review?(benjamin)
Attachment #307421 - Flags: review+
(Assignee)

Comment 14

9 years ago
Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Requesting approval for the trunk patch.
Attachment #307421 - Flags: approval1.9?
Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Let's get a test per bsmedberg's suggestion.  Re-request approval once tests are included.
Attachment #307421 - Flags: approval1.9?
I could be wrong, but I think bsmedberg was requesting a test for the branch patch. The trunk patch has a test already...
(Assignee)

Comment 17

9 years ago
Comment on attachment 307421 [details] [diff] [review]
Trunk patch

Yes, what Samuel said. This is the trunk patch, with a test, which Ben already gave r+sr to. (This should really be blocking 1.9+ too)
Attachment #307421 - Flags: approval1.9?
(Assignee)

Comment 18

9 years ago
Created attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp
Attachment #306514 - Attachment is obsolete: true
Attachment #308810 - Flags: superreview?(benjamin)
Attachment #308810 - Flags: review?(benjamin)
Comment on attachment 307421 [details] [diff] [review]
Trunk patch

a1.9+=damons
Attachment #307421 - Flags: approval1.9? → approval1.9+
Attachment #308810 - Flags: superreview?(benjamin)
Attachment #308810 - Flags: superreview+
Attachment #308810 - Flags: review?(benjamin)
Attachment #308810 - Flags: review+
(Assignee)

Comment 20

9 years ago
Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

Very safe fix ensuring that the length returned by UTF8InputStream::Read doesn't exceed the number of characters successfully converted.
Attachment #308810 - Flags: approval1.8.1.14?
Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

Approved for 1.8.1.14. a=ss for release-drivers.
Attachment #308810 - Flags: approval1.8.1.14? → approval1.8.1.14+
(Assignee)

Comment 22

9 years ago
Trunk and branch patches are both checked in.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.8.1.14
Resolution: --- → FIXED
http://mxr.mozilla.org/mozilla/source/intl/strres/tests/unit/test_bug397093.js
Flags: in-testsuite+
Why is this sg:moderate? text from .properties files isn't normally accessible to web content in any way even if there is such a malformed file. We also don't know of any .properties files shipped with Firefox that had this problem.
Whiteboard: [sg:moderate] 1.8 branch, requires faulty localization/addon, need r=bsmedberg → [sg:low] 1.8 branch, requires faulty localization/addon
Group: security
(Reporter)

Comment 25

9 years ago
(In reply to comment #24)
> Why is this sg:moderate? text from .properties files isn't normally accessible
> to web content in any way even if there is such a malformed file. We also don't
> know of any .properties files shipped with Firefox that had this problem.

Extensions ?

Updated

9 years ago
Attachment #308810 - Flags: approval1.8.0.next+

Comment 26

9 years ago
Comment on attachment 308810 [details] [diff] [review]
Branch patch with test added to StringBundleTest.cpp

a=asac for 1.8.0

Updated

9 years ago
Flags: blocking1.8.0.next+
You need to log in before you can comment on or make changes to this bug.