Closed Bug 1000030 Opened 6 years ago Closed 6 years ago
crash in Rtl
Expand Environment Strings | kernelbase .dll@0xfc33
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]
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.
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
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.
You need to log in before you can comment on or make changes to this bug.