Possible buffer overwrite in SplitDriverVersion

RESOLVED FIXED in Firefox 43

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: erahm, Assigned: milan)

Tracking

({coverity, csectype-bounds, sec-low})

unspecified
mozilla43
coverity, csectype-bounds, sec-low
Points:
---

Firefox Tracking Flags

(firefox41 wontfix, firefox42 wontfix, firefox43 fixed, firefox-esr38 wontfix, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master unaffected)

Details

(Whiteboard: [CID 1299530][post-critsmash-triage][adv-main43+])

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #791742 +++

Coverity indicates a possible buffer overwrite in |SplitDriverVersion| [1]. We bail if |destIdx| > 4 [2], but if |destIdx| == 4 we'll overwrite the |dest| buffer.

Rough breakdown from Coverity:

> 184inline bool SplitDriverVersion(const char *aSource, char *aAStr, char *aBStr, char *aCStr, char *aDStr)
> 185{
> 186  // sscanf doesn't do what we want here to we parse this manually.
> 187  int len = strlen(aSource);
> 188  char *dest[4] = { aAStr, aBStr, aCStr, aDStr };
> 189  unsigned destIdx = 0;
> 190  unsigned destPos = 0;
> 191
>    1. Condition i < len, taking true branch
>    5. Condition i < len, taking true branch
> 192  for (int i = 0; i < len; i++) {
>    2. Condition destIdx > 4UL /* mozilla::ArrayLength(dest) */, taking false branch
>    6. Condition destIdx > 4UL /* mozilla::ArrayLength(dest) */, taking false branch
>    7. cond_at_most: Checking destIdx > 4UL implies that destIdx may be up to 4 on the false branch.
> 193    if (destIdx > ArrayLength(dest)) {
> 194      // Invalid format found. Ensure we don't access dest beyond bounds.
> 195      return false;
> 196    }
> 197
>    3. Condition aSource[i] == '.', taking true branch
>    8. Condition aSource[i] == '.', taking true branch
> 198    if (aSource[i] == '.') {
>    CID 1299530 (#1 of 1): Out-of-bounds read (OVERRUN)9. overrun-local: Overrunning array dest of 4 8-byte elements at element index 4 (byte offset 32) using index destIdx++ (which evaluates to 4).
> 199      dest[destIdx++][destPos] = 0;
> 200      destPos = 0;
>    4. Continuing loop
> 201      continue;
> 202    }
> 203

[1] https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/widget/GfxDriverInfo.h#l185
[2] https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/widget/GfxDriverInfo.h#l194
Calling this "sec-low" because installing a badly-named driver in order to hack Firefox seems like an inefficient way to hack someone (you've got a driver installed!).
Keywords: csectype-bounds, sec-low
Assignee: nobody → milan
Created attachment 8646516 [details] [diff] [review]
Check for array boundaries, add some more tests. r=botond
Attachment #8646516 - Flags: review?(botond)
Comment on attachment 8646516 [details] [diff] [review]
Check for array boundaries, add some more tests. r=botond

Want to add more tests.
Attachment #8646516 - Flags: review?(botond)
Created attachment 8646536 [details] [diff] [review]
Check for array boundaries, add some more tests. r=botond
Attachment #8646516 - Attachment is obsolete: true
Attachment #8646536 - Flags: review?(botond)

Updated

3 years ago
Attachment #8646536 - Flags: review?(botond) → review+
Comment on attachment 8646536 [details] [diff] [review]
Check for array boundaries, add some more tests. r=botond

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very difficult - you'd have to convince the user to install a driver with a bad version, then figure out what to do with the memory overwrite.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?
This has been around forever.  I think it can just ride the trains, it's sec-low.

How likely is this patch to cause regressions; how much testing does it need?
The unit test is covering the old problems and the new changes.
Attachment #8646536 - Flags: sec-approval?
This is sec-low, it doesn't need sec-approval+. Just check it in and it will ride the trains!
Attachment #8646536 - Flags: sec-approval?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/95f3db5684c2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Is this worth backporting anywhere?
Flags: needinfo?(milan)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Is this worth backporting anywhere?

Doubt it.  As Al mentions in comment 1, not the most efficient attack vector.
Flags: needinfo?(milan)
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-master: --- → unaffected
status-firefox41: --- → wontfix
status-firefox42: --- → wontfix
status-firefox-esr38: --- → wontfix

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [CID 1299530] → [CID 1299530][post-critsmash-triage]
Whiteboard: [CID 1299530][post-critsmash-triage] → [CID 1299530][post-critsmash-triage][adv-main43+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.