Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

10 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

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

Comment 11

10 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

10 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

10 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

10 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

10 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.