1188105 - Possible buffer overwrite in SplitDriverVersion

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Eric Rahm [:erahm]]

+++ 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

Comment 1

[Daniel Veditz [:dveditz]]

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!).

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Check for array boundaries, add some more tests. r=botond

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8646516 [details] [diff] [review]
Check for array boundaries, add some more tests. r=botond

Want to add more tests.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Check for array boundaries, add some more tests. r=botond

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee6ba3637a0a

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

This is sec-low, it doesn't need sec-approval+. Just check it in and it will ride the trains!

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95f3db5684c2

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95f3db5684c2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this worth backporting anywhere?

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

(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.