1186725 - (CVE-2015-7177) Memory-safety bug in InitTextures

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[q1]

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: }

Comment 1

[Andrea Marchesini [:baku]]

Attachment: crash2.patch

Comment 2

[Andrea Marchesini [:baku]]

Matt, can you help me to define the security level of this bug? How easy is to exploit this issue?

Comment 3

[Matt Woodrow (:mattwoodrow)]

I'm not sure sorry.

Comment 4

[q1]

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.

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Andrea Marchesini [:baku]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c33ca97b63

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/e5c33ca97b63

Comment 8

[Ryan VanderMeulen [:RyanVM]]

How far back does this go? Should we consider backporting?

Comment 9

[Andrea Marchesini [:baku]]

Mar 23 2015. Which version was it?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

39+. Please request Aurora/Beta approval on this then :)

Comment 11

[Andrea Marchesini [:baku]]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f12ffbcf03fd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/44608b5dde92