Last Comment Bug 634257 - nsUCS2BEToUnicode fails to adhere to the API contract when given a buffer with one byte
: nsUCS2BEToUnicode fails to adhere to the API contract when given a buffer wit...
Status: RESOLVED FIXED
: [sg:moderate] or high? [hardblocker][...
: privacy
Product: Core
Classification: Components
Component: Internationalization
: Trunk
: All All
: -- critical (vote)
: mozilla2.0
Assigned To: Simon Montagu
: i18n
: http://mxr.mozilla.org/mozilla-centra...
: 638236
:
  Show dependency treegraph
 
Reported: 2011-02-15 06:30 PST by Henri Sivonen (:hsivonen)
Modified: 2011-05-18 18:22 PDT (History)
6 users (show)
smontagu: in‑testsuite?
See Also:
Crash Signature:
  ---
  unaffected
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  .17+
  .17-fixed
  .19+
  .19-fixed


Attachments
Test case (1 bytes, text/html; charset=UTF-16LE)
2011-02-15 06:32 PST, Henri Sivonen (:hsivonen)
no flags Details
Fix for the crash (4.13 KB, patch)
2011-02-17 13:59 PST, Simon Montagu
VYV03354: review+
benjamin: approval2.0+
dveditz: approval1.9.2.17-
dveditz: approval1.9.1.19-
Details | Diff | Splinter Review
Tests (1.35 KB, patch)
2011-02-17 22:53 PST, Simon Montagu
no flags Details | Diff | Splinter Review
Tests (2.29 KB, patch)
2011-02-23 00:42 PST, Simon Montagu
no flags Details | Diff | Splinter Review
Patch for branches including dbaron's fix from bug 638236 (4.29 KB, patch)
2011-03-03 00:28 PST, Simon Montagu
smontagu: review+
clegnitto: approval1.9.2.17+
clegnitto: approval1.9.1.19+
Details | Diff | Splinter Review

Summon comment box

Description Henri Sivonen (:hsivonen) 2011-02-15 06:30:44 PST
The nsIUnicodeDecoder documentation says:
"The eventual incomplete final character data will be stored internally in the converter and used when the method is called again for continuing the conversion. This way, the caller will not have to worry about managing incomplete input data by mergeing it with the next buffer."
and
"When partial input, it will be consumed and cached."

Furthermore, the docs say:
"@param aDestLength [IN/OUT] the length of the destination data buffer; after conversion will contain the number of Unicode characters written"

However, if a one-byte buffer is passed to nsUCS2BEToUnicode, it returns NS_ERROR_ILLEGAL_INPUT in violation of the requirement for decoders to cache partial input and, much worse, it fails to set aDestLength to 0.

This looks to the caller as if the entire output buffer was written to but then an error occurred. This means that uninitialized memory shows to content, because in fact 0 code units were written to the output.

The decoder should work even if it were fed one byte at a time and it should always adjust aDestLength to reflect the actual number of bytes written before it returns.

I've seen crashes that have to be somehow related to this, but I don't have an explanation for how the crashes happen.
Comment 1 Henri Sivonen (:hsivonen) 2011-02-15 06:32:26 PST
Created attachment 512475 [details]
Test case
Comment 2 chris hofmann 2011-02-17 13:28:59 PST
I just saw a crash too, right after display on junk in the content window.  no breakpad
Comment 3 Daniel Veditz 2011-02-17 13:30:04 PST
We're at least leaking chunks of memory. chofmann crashed (without getting breakpad) so could be worse
Comment 4 Simon Montagu 2011-02-17 13:59:15 PST
Created attachment 513251 [details] [review]
Fix for the crash

This returns the correct value in aDestLength and should prevent the crash.

> The decoder should work even if it were fed one byte at a time

I'm deferring this part until after FF 4. The UTF-16 decoder is much too byzantine already, and this will be a relatively complex change.
Comment 5 chris hofmann 2011-02-17 14:22:06 PST
crash w/o breakpad is pretty reproducible for me on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110214 Firefox/4.0b12pre
Comment 6 Simon Montagu 2011-02-17 22:53:35 PST
Created attachment 513382 [details] [review]
Tests
Comment 7 Mike Beltzner [:beltzner] 2011-02-18 08:45:46 PST
Risk/reward?
Comment 8 chris hofmann 2011-02-18 09:18:48 PST
things mentioned on the risk side in critkill:

leaking chunks of memory could revile private info like credit cards, browsing history, etc. if the attacker could figure out how to control the behavior.  do we know if that might be possible?

the simple test case thats resulting in the crash could be bad if the crash is exploitable.  this is also hard to determine  so far since we haven't been able to get a stack though breakpad or someone runniung in the debugger.
Comment 9 Mike Shaver (:shaver) 2011-02-19 10:39:29 PST
is this ready to land?
Comment 10 Simon Montagu 2011-02-20 01:08:44 PST
http://hg.mozilla.org/mozilla-central/rev/aa28638dc457

I'll check in the test case after we release with the patch.
Comment 11 Henri Sivonen (:hsivonen) 2011-02-20 02:23:15 PST
While the test case here does not show the problem in Firefox 3.6, shouldn't the patch also go on branches--just in case there are other ways to get a one-byte buffer passed to the UTF-16 decoders on the branches?
Comment 12 Simon Montagu 2011-02-23 00:42:01 PST
Created attachment 514435 [details] [review]
Tests

Reftests are better than crashtests for this, to ensure that we are really only seeing one character in the results.
Comment 13 Daniel Veditz 2011-03-02 10:53:04 PST
Comment on attachment 513251 [details] [review]
Fix for the crash

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Comment 14 Daniel Veditz 2011-03-02 10:56:13 PST
fwiw the testcase does not crash 3.5.18pre or 3.6.15pre on Mac.
Comment 15 Simon Montagu 2011-03-02 15:24:03 PST
Holding the branch checkin for a fix to bug 638236
Comment 16 Daniel Veditz 2011-03-02 16:00:57 PST
Comment on attachment 513251 [details] [review]
Fix for the crash

On second thought, please don't check these in until we understand bug 638236
Comment 17 Simon Montagu 2011-03-03 00:28:40 PST
Created attachment 516532 [details] [review]
Patch for branches including dbaron's fix from bug 638236

transferring r=emk and r=smontagu from the original patches (attachment 513251 [details] [review] and attachment 516479 [details] [review])
Comment 18 Christian Legnitto [:LegNeato] 2011-03-07 10:24:37 PST
Comment on attachment 516532 [details] [review]
Patch for branches including dbaron's fix from bug 638236

Approved for branch

Note You need to log in before you can comment on or make changes to this bug.