Closed Bug 634257 Opened 9 years ago Closed 9 years ago

nsUCS2BEToUnicode fails to adhere to the API contract when given a buffer with one byte

Categories

(Core :: Internationalization, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
firefox5 --- unaffected
blocking1.9.2 --- .17+
status1.9.2 --- .17-fixed
blocking1.9.1 --- .19+
status1.9.1 --- .19-fixed

People

(Reporter: hsivonen, Assigned: smontagu)

References

()

Details

(Keywords: privacy, Whiteboard: [sg:moderate] or high? [hardblocker][has patch])

Attachments

(4 files, 1 obsolete file)

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.
Attached file Test case
I just saw a crash too, right after display on junk in the content window.  no breakpad
We're at least leaking chunks of memory. chofmann crashed (without getting breakpad) so could be worse
Keywords: privacy
Whiteboard: [sg:moderate] or high?
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.
Attachment #513251 - Flags: review?(VYV03354)
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
Attachment #513251 - Flags: review?(VYV03354) → review+
Attached patch Tests (obsolete) — Splinter Review
Attachment #513251 - Flags: approval2.0?
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.
Attachment #513251 - Flags: approval2.0? → approval2.0+
blocking2.0: --- → final+
Whiteboard: [sg:moderate] or high? → [sg:moderate] or high? [hardblocker]
Whiteboard: [sg:moderate] or high? [hardblocker] → [sg:moderate] or high? [hardblocker][has patch]
http://hg.mozilla.org/mozilla-central/rev/aa28638dc457

I'll check in the test case after we release with the patch.
Status: NEW → RESOLVED
blocking2.0: final+ → ---
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
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?
Attachment #513251 - Flags: approval1.9.2.14?
Attachment #513251 - Flags: approval1.9.1.17?
Attached patch TestsSplinter Review
Reftests are better than crashtests for this, to ensure that we are really only seeing one character in the results.
Attachment #513382 - Attachment is obsolete: true
Attachment #513251 - Flags: approval1.9.2.15?
Attachment #513251 - Flags: approval1.9.2.14?
Attachment #513251 - Flags: approval1.9.1.18?
Attachment #513251 - Flags: approval1.9.1.17?
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Comment on attachment 513251 [details] [diff] [review]
Fix for the crash

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Attachment #513251 - Flags: approval1.9.2.15?
Attachment #513251 - Flags: approval1.9.2.15+
Attachment #513251 - Flags: approval1.9.1.18?
Attachment #513251 - Flags: approval1.9.1.18+
fwiw the testcase does not crash 3.5.18pre or 3.6.15pre on Mac.
Depends on: 638236
Holding the branch checkin for a fix to bug 638236
Comment on attachment 513251 [details] [diff] [review]
Fix for the crash

On second thought, please don't check these in until we understand bug 638236
Attachment #513251 - Flags: approval1.9.2.15-
Attachment #513251 - Flags: approval1.9.2.15+
Attachment #513251 - Flags: approval1.9.1.18-
Attachment #513251 - Flags: approval1.9.1.18+
transferring r=emk and r=smontagu from the original patches (attachment 513251 [details] [diff] [review] and attachment 516479 [details] [diff] [review])
Attachment #516532 - Flags: review+
Attachment #516532 - Flags: approval1.9.2.15?
Attachment #516532 - Flags: approval1.9.1.18?
Comment on attachment 516532 [details] [diff] [review]
Patch for branches including dbaron's fix from bug 638236

Approved for branch
Attachment #516532 - Flags: approval1.9.2.15?
Attachment #516532 - Flags: approval1.9.2.15+
Attachment #516532 - Flags: approval1.9.1.18?
Attachment #516532 - Flags: approval1.9.1.18+
blocking1.9.1: ? → .18+
blocking1.9.2: ? → .16+
blocking1.9.2: .15+ → .16+
Group: core-security
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.