1000030 - crash in RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[James Kitchener (:jkitch)]

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.

Comment 1

[James Kitchener (:jkitch)]

Attachment: patch.diff

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.

Comment 2

[(Away)]

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.

Comment 3

[(Away)]

Actually I would remove the comment from ReadBinaryValue unless you're sure it would result in a crash.

Comment 4

[James Kitchener (:jkitch)]

Attachment: patch.diff

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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/766648238bd3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/766648238bd3

Comment 7

[(Away)]

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);

Comment 8

[James Kitchener (:jkitch)]

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 9

[James Kitchener (:jkitch)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c66942faa3b2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c66942faa3b2

Comment 12

[u279076]

Marking this verified fixed as there are 0 reports with this signature across all branches in the last week.