1532525 - (CVE-2019-11693) could be trigger oom problem with WebGLBuffer::BufferData

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[crixer]

Attachment: 1.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.119 Safari/537.36

Steps to reproduce:

void WebGLBuffer::BufferData(GLenum target, size_t size, const void* data,
GLenum usage) {
// Careful: data.Length() could conceivably be any uint32_t, but GLsizeiptr
// is like intptr_t.
if (!CheckedInt<GLsizeiptr>(size).isValid())
return mContext->ErrorOutOfMemory("bad size");

if (!ValidateBufferUsageEnum(mContext, usage)) return;

#ifdef XP_MACOSX
// bug 790879
if (mContext->gl->WorkAroundDriverBugs() && size > INT32_MAX) {
mContext->ErrorOutOfMemory("Allocation size too large.");
return;
}
#endif

const void* uploadData = data;

UniqueBuffer newIndexCache;
if (target == LOCAL_GL_ELEMENT_ARRAY_BUFFER &&
mContext->mNeedsIndexValidation) {
newIndexCache = malloc(size);
if (!newIndexCache) {
mContext->ErrorOutOfMemory("Failed to alloc index cache.");
return;
}
memcpy(newIndexCache.get(), data, size);
uploadData = newIndexCache.get();
}

The bufferdata function does not have an appropriate check for size value.
This coulde cause problems in the attached code.
I think it's almost impossible to exploit, but I think it's a security issue. :)

Comment 1

[crixer]

Attachment: asan.log

this is asan log

Comment 2

[Kelsey Gilbert [:jgilbert]]

Ugh, we need to get rid of size_t.

Comment 3

[Kelsey Gilbert [:jgilbert]]

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

It never occurred to me how dangerous malloc/calloc taking size_t was, but it's obvious now.

Comment 5

[Kelsey Gilbert [:jgilbert]]

I'm not seeing the bug you're hitting in ASAN, except if it's a driver bug. I do see one frame of driver call in the asan stack, so that's possible.

Comment 6

[Kelsey Gilbert [:jgilbert]]

In particular, this keeps everything here safe:

if (!CheckedInt<GLsizeiptr>(size).isValid())
  return mContext->ErrorOutOfMemory("bad size");

This will error out if size_t size is too large for GLsizeiptr. (size_t is unsigned, so it can never be too small)

Comment 7

[crixer]

So what do you think is the root cause of the bug?

I didn't see that properly code, but I knew for sure that oom occurred when i use bufferdata function.

Comment 8

[crixer]

and i didn't get that message when i test a poc

Comment 9

[crixer]

(In reply to crixer from comment #8)

and i couldn't get that message when i test a poc

Comment 10

[Daniel Veditz [:dveditz]]

crixer: Why do you keep saying "oom" (out of memory)? is there some other data you haven't provided?

Jeff: are you saying this is not a security problem in comment 6, or is that just part of what's going on?

Comment 11

[Kelsey Gilbert [:jgilbert]]

There's definitely issues here on 32bit builds, but I think on 64bit we should be doing everything correctly on our end. There's a chance the driver has a bug here though.

FWIW, WebGLintptr and WebGLsizeiptr are both int64_t, while GLintptr and GLsizeiptr are equivalent to intptr_t, which is int64_t on 64bit, but int32_t on 32bit. size_t is naturally uint64_t on 64bit, uint32_t on 32bit.

There is also an issue with handling GL_OUT_OF_MEMORY resulting from the glBufferData call, but I don't see how the POC could trigger it.

Comment 12

[crixer]

(In reply to Daniel Veditz [:dveditz] from comment #10)

crixer: Why do you keep saying "oom" (out of memory)? is there some other data you haven't provided?

Jeff: are you saying this is not a security problem in comment 6, or is that just part of what's going on?

the reason why i can say this is oom that i tested poc code on ubuntu and confirmed oom through dmesg.

Comment 13

[crixer]

Attachment: testcase.html

but if test this case that will cause a heap overflow.
we also need to process this case too.

Comment 14

[crixer]

Attachment: ASAN.txt

this is asan log for the new case.

Comment 15

[Kelsey Gilbert [:jgilbert]]

I'll be spinning up an ASAN env to dig into this better.

Comment 16

[Jessie [:jbonisteel] pls NI]

Hey Sotaro - would you be able to help with this bug this week?

Comment 17

[Sotaro Ikeda [:sotaro]]

Yes, I am going to take a look.

Comment 18

[Sotaro Ikeda [:sotaro]]

I tested Attachment 9049736 [details] on Windows10, I did not see the problem. Log had the following error.

WARN: handleError(8102): Error: 0x00000505, in c:/firefox_bld_dsk_2/mozilla-central/gfx/angle/checkout/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp, allocate:404. Internal D3D11 error: HRESULT: 0x8007000E: Error allocating Buffer
JavaScript warning: https://bug1532525.bmoattachments.org/attachment.cgi?id=9049736&t=kdFtTLYAqSqFIDisgxpZu9, line 7: Error: WebGL warning: bufferData: Error from driver: 0x0505

Comment 19

[Sotaro Ikeda [:sotaro]]

When I tested attachment 9049736 [details] on ubuntu 18.04.1 on VMWare, I could reproduce the problem, though I did not see the problem on ubuntu 18.04.1 that run as native on PC. From it, it seems like a driver specific problem.

Comment 20

[Sotaro Ikeda [:sotaro]]

about:support had the following driver info on ubuntu 18.04.1 on VMWare

GPU #1

• Description: SVGA3D; build: RELEASE; LLVM;
• Vendor ID: mesa/swrast
• Device ID: 0x0405
• Driver Version: 18.0.5.0

Comment 21

[Sotaro Ikeda [:sotaro]]

I tested attachment 9049736 [details] by modifying the size on ubuntu 18.04.1 on VMWare. From it, the driver might use int32_t for the size handling.

size 0x7fffffff : gl->fBufferData() failed
size 0x80000000 : gl->fBufferData() failed
size 0x80000001 : gl->fBufferData() did not exit
size 0xfffffff0 : gl->fBufferData() did not exit
size 0xfffffff1 : gl->fBufferData() caused "AddressSanitizer: stack-buffer-underflow"
size 0xfffffffe : gl->fBufferData() caused "AddressSanitizer: stack-buffer-underflow"
size 0xffffffff : gl->fBufferData() caused "AddressSanitizer: stack-buffer-underflow"

Comment 22

[Sotaro Ikeda [:sotaro]]

crixer, can you provide a graphics driver information from about:support?

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1532525 - Enable size limit on linux

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment 9060325 [details] is a simple fix for the problem.

Comment 25

[crixer]

Attachment: Screenshot from 2019-04-24 01-41-06.png

driver information from about:support

Comment 26

[crixer]

Attachment: Screenshot from 2019-04-24 01-44-57.png

Comment 27

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 28

[Kelsey Gilbert [:jgilbert]]

We should figure out a path towards re-enabling this on supportive drivers, but that's a P3.

Comment 29

[Sotaro Ikeda [:sotaro]]

I will create a bug of comment 28.

Comment 30

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9060325 [details]
Bug 1532525 - Enable size limit on linux

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It seems relatively easy.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Since WebGL supprot
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It is easy to create the backports.
• How likely is this patch to cause regressions; how much testing does it need?: It seems not cause regressions, since it just added max size limit on Linux that was already done on mac.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It seems relatively easy.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Since WebGL supprot
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It is easy to create the backports.
• How likely is this patch to cause regressions; how much testing does it need?: It seems not cause regressions, since it just added max size limit on Linux that was already done on mac.

Comment 31

[Sotaro Ikeda [:sotaro]]

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

Comment 32

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

Comment on attachment 9060325 [details]
Bug 1532525 - Enable size limit on linux

sec-approval+ for trunk after discussion with Liz in release management. We'll want Beta and ESR60 patches made and nominated.

Comment 33

[Sotaro Ikeda [:sotaro]]

The same patch could be applied to Beta and ESR60.

Comment 34

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9060325 [details]
Bug 1532525 - Enable size limit on linux

Beta/Release Uplift Approval Request

• User impact if declined: Malicious content could freeze or crash tab when a PC has a specific graphic driver on linux.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It added max size limit on Linux that was already done on mac.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Malicious content could freeze or crash tab when a PC has a specific graphic driver on linux.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It added max size limit on Linux that was already done on mac.
• String or UUID changes made by this patch: none

Comment 35

[Pascal Chevrel:pascalc]

This is not on trunk yet, will it land in time for our last betas this week?

Comment 36

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/053685dc2ce746e373ec6146621b231bed610874

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/053685dc2ce7

Comment 38

[Pascal Chevrel:pascalc]

Comment on attachment 9060325 [details]
Bug 1532525 - Enable size limit on linux

Uplift approved for 67 beta 15, thanks.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ea1286c1f2bb

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9060325 [details]
Bug 1532525 - Enable size limit on linux

Fix for sec-high issue, seems low risk, linux-only.
OK for esr 60.7.

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/beb8e256f682