Closed Bug 1186725 (CVE-2015-7177) Opened 5 years ago Closed 5 years ago

Memory-safety bug in InitTextures

Categories

(Core :: Graphics: Layers, defect)

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
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

People

(Reporter: q1, Assigned: baku)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41+])

Attachments

(1 file)

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: }
Attached patch crash2.patchSplinter Review
Attachment #8637693 - Flags: review?(matt.woodrow)
Attachment #8637693 - Flags: review?(matt.woodrow) → review+
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)
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: nobody → amarchesini
https://hg.mozilla.org/mozilla-central/rev/e5c33ca97b63
Status: NEW → RESOLVED
Closed: 5 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)
Mar 23 2015. Which version was it?
Flags: needinfo?(amarchesini) → needinfo?(ryanvm)
39+. Please request Aurora/Beta approval on this then :)
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+
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.