Closed Bug 1000030 Opened 6 years ago Closed 6 years ago

crash in RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33

Categories

(Core :: XPCOM, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox30 --- verified
firefox31 --- verified
b2g-v1.4 --- fixed

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-dda6ef3c-25a6-4a2c-b233-8191a2140417.
=============================================================

Fallout from bug 328755 which made the empty string's null terminator immutable.  

Like bug 936886, it seems we can't trust Windows not to overwrite the empty string.
Crash Signature: [@ RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33] → [@ RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33] [@ RtlExpandEnvironmentStrings_U | kernel32.dll@0x306bb]
Attached patch patch.diff (obsolete) — Splinter Review
The attached patch only the expands environment string if it expands to something more than a null terminator.

This also includes a preventative fix to ReadBinaryValue which appears to be vulnerable to the same sort of bug.
Attachment #8411630 - Flags: review?(dmajor)
Comment on attachment 8411630 [details] [diff] [review]
patch.diff

Review of attachment 8411630 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not convinced that ReadBinaryValue is affected. The issue with ReadStringValue is that we do some ++/-- tricks and eventually call the API telling it "you may write 1 byte here" when really it's "you may not write any bytes here". As far as I can tell, ReadBinaryValue is honest about the API parameters. I guess the check can't hurt, though.
Attachment #8411630 - Flags: review?(dmajor) → review+
Actually I would remove the comment from ReadBinaryValue unless you're sure it would result in a crash.
Attached patch patch.diffSplinter Review
ReadBinaryValue comment removed.  I don't have any evidence that it is a problem, only that it fits the general pattern of similar crashes (size manipulations excepted).

That being said, the null check does no harm.
Attachment #8411630 - Attachment is obsolete: true
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/766648238bd3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
So I was thinking about this some more, and isn't ReadStringValue still broken for values >1? We tell the API that it can write one more byte than the actual length of the string:

      --resultLen;
      expandedResult.SetLength(resultLen);
      [...]
      resultLen = ExpandEnvironmentStringsW(flatSource.get(),
                                            wwc(begin.get()),
                                            resultLen + 1);
My understanding is that our string length excludes the null terminator, while ExpandEnvironmentStringsW's string length includes it.

Although it may result in the write exceeding the bounds of the string buffer, all it does is overwrite the next character in memory, which is always 0, with another 0.

Although it may technically be a buffer overflow, it should be safe providing we can trust Microsoft to always write the null terminator in the appropriate position.
Comment on attachment 8413169 [details] [diff] [review]
patch.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  Bug 328755
User impact if declined:  Repeated crashes for users with specific registry entries.
Testing completed (on m-c, etc.): On M-C for a week and Aurora for a few days without any apparent issues.
Risk to taking this patch (and alternatives if risky):  Low.  Little more than a null check.
String or IDL/UUID changes made by this patch: None
Attachment #8413169 - Flags: approval-mozilla-beta?
Attachment #8413169 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Marking this verified fixed as there are 0 reports with this signature across all branches in the last week.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.