Bug 1186725 (CVE-2015-7177)

Memory-safety bug in InitTextures

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({sec-moderate})

39 Branch
mozilla43
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox40 wontfix, firefox41 fixed, firefox42 fixed, firefox43 fixed, firefox-esr38 unaffected, 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 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
InitTextures (gfx\layers\IMFYCbCrImage.cpp) can cause its callers to use a wild pointer to read and/or write data to memory. This is because it calls IDirect3DSurface9::LockRect without checking the return code. Instead, it checks whether the resulting aLockedRect.pBits member == nullptr. It then returns aLockedRect to its callers, which use its members to read and/or write data into memory that (on success) would be mapped to a GPU buffer. 

The problem is that InitTextures receives aLockedRect from its caller, and does not itself initialize it. As it happens, its only known caller (UploadData) passes a reference to an uninitialized automatic D3DLOCKED_RECT to InitTextures. D3DLOCKED_RECT also lacks a constructor, so it does not initialize itself.

The upshot is that if LockRect fails (as under OOM conditions), UploadData will overwrite (probably a considerable amount of) memory at some arbitrary place in Firefox's address space.

116: static bool UploadData(IDirect3DDevice9* aDevice,
117:                        RefPtr<IDirect3DTexture9>& aTexture,
118:                        HANDLE& aHandle,
119:                        uint8_t* aSrc,
120:                        const gfx::IntSize& aSrcSize,
121:                        int32_t aSrcStride)
122: {
123:   RefPtr<IDirect3DSurface9> surf;
124:   D3DLOCKED_RECT rect;
125:   aTexture = InitTextures(aDevice, aSrcSize, D3DFMT_A8, surf, aHandle, rect);
126:   if (!aTexture) {
127:     return false;
128:   }
...

59: static TemporaryRef<IDirect3DTexture9>
60: InitTextures(IDirect3DDevice9* aDevice,
61:              const IntSize &aSize,
62:             _D3DFORMAT aFormat,
63:             RefPtr<IDirect3DSurface9>& aSurface,
64:             HANDLE& aHandle,
65:             D3DLOCKED_RECT& aLockedRect)
66: {
...
92:   aSurface->LockRect(&aLockedRect, nullptr, 0);
93:   if (!aLockedRect.pBits) {
94:     NS_WARNING("Could not lock surface");
95:     return nullptr;
96:   }
97:
98:   return result;
99: }
(Assignee)

Comment 1

4 years ago
Posted patch crash2.patchSplinter Review
Attachment #8637693 - Flags: review?(matt.woodrow)
Attachment #8637693 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 2

4 years ago
Matt, can you help me to define the security level of this bug? How easy is to exploit this issue?
Flags: needinfo?(matt.woodrow)
I'm not sure sorry.
Flags: needinfo?(matt.woodrow)
(Reporter)

Comment 4

4 years ago
I think the exploitability depends upon where the source surface comes from.

If it's directly derived from something under external control (e.g., it's the result of rendering a web page) then an attacker could manipulate the contents of the page to cause the derivation of surface contents that she likes, which this bug would then spray across memory.

If, on the other hand, the source surface is independent of external control (e.g., it's just filled with a constant color), then the exploitability is lower. But it is still nonzero, because overwriting arbitrary memory with anything at all can cause not just incorrect operation, but insecure operation, as by modifying privilege masks, causing the wrong functions to gain control, etc.

Similarly, exploitability depends upon which memory is likely to get sprayed, which, in turn, depends upon the contents of the stack locations that underlie rect. This then depends upon the call order, the build parameters (including compiler/linker versions), etc.

I think it's easier to get a good insight into where the source surface comes from, but I don't have that insight.
Flags: sec-bounty?
It seems unlikely to time OOM perfectly so that the LockRect() fails due to OOM but you could still have enough memory to get through this code path successfully, so downgrading to sec-moderate.
Keywords: sec-moderate
(Assignee)

Updated

4 years ago
Assignee: nobody → amarchesini
https://hg.mozilla.org/mozilla-central/rev/e5c33ca97b63
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Flags: sec-bounty? → sec-bounty+
How far back does this go? Should we consider backporting?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 9

4 years ago
Mar 23 2015. Which version was it?
Flags: needinfo?(amarchesini) → needinfo?(ryanvm)
39+. Please request Aurora/Beta approval on this then :)
(Assignee)

Comment 11

4 years ago
Comment on attachment 8637693 [details] [diff] [review]
crash2.patch

Approval Request Comment
[Feature/regressing bug #]:bug 1145513
[User impact if declined]: a crash could occur.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]:  none, we just check the return code of LockRect.
[String/UUID change made/needed]: none
Attachment #8637693 - Flags: approval-mozilla-beta?
Attachment #8637693 - Flags: approval-mozilla-aurora?
Comment on attachment 8637693 [details] [diff] [review]
crash2.patch

Andrea, I don't believe that a risk can be "None". Every changes introduce a risk.
However, this seems quite safe. Taking it.
Attachment #8637693 - Flags: approval-mozilla-beta?
Attachment #8637693 - Flags: approval-mozilla-beta+
Attachment #8637693 - Flags: approval-mozilla-aurora?
Attachment #8637693 - Flags: approval-mozilla-aurora+

Updated

4 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Alias: CVE-2015-7177
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.