971695 - Faulty: the compositor needs to check that the memory buffers it receives are large enough

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Faulty session

Found by Christoph Diehl's "Faulty" fuzzer, see bug 777067

The compositor receives memory buffers (either Shmem or plain old memory buffers) from the client, and is asked to interprete them as bitmaps with specified width and height and pixel format. But before doing that, it should verify that these memory buffers are large enough to read the correponding number of pixels from them.

The attached session shows a crash happening due to failing to verify this. Faulty tweaks the size value for a memory buffer:

[Faulty] pickle field {size_t} of value: 3200016 changed to: 1

See below how we have width=800, height=1000, and 4 bytes per pixels, making 3200000 the byte size of the image. So the above tweaked value was probably the size of a buffer, and by tweaking it to 1 we make the parent process allocate only 1 byte for that buffer. Subsequently, we crash as we tell OpenGL to read 3200000 bytes out of it in glTexImage2D.

Comment 1

[Daniel Veditz [:dveditz]]

Assuming OpenGL doesn't try to write into that too-small buffer this shouldn't be too bad. I guess worst case the process /doesn't/ crash and random memory gets incorporated into the image? I assume an attacker could then interrogate the canvas to get that information back out? Given that this is somewhere between sec-high and sec-moderate.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Assuming OpenGL doesn't try to write into that too-small buffer this
> shouldn't be too bad.

Indeed, this is read-only.

> I guess worst case the process /doesn't/ crash and
> random memory gets incorporated into the image?

Correct.

> I assume an attacker could
> then interrogate the canvas to get that information back out? Given that
> this is somewhere between sec-high and sec-moderate.

In principle, it is not possible for unprivileged Web content to read back the output of the compositor.

In practice, enforcement of that rule has often been brittle, especially under timing attacks, so it has often been possible to read, at least partially and approximately, some of the output of the compositor.

But yeah, sec-moderate seems fair here.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: make ImageDataSerializer check data size

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: make YCbCrImageDataSerializer check data size

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8378237 [details] [diff] [review]
Part 1: make ImageDataSerializer check data size

*** Note: I'm writing only one sec-approval request for both patches here  ***


[Security approval request comment]
How easily could an exploit be constructed based on the patch?


The present bug is not AFAICS exploitable by itself. By itself, it's only allowing a client to crash the parent process, i.e. a mere DOS. But maybe the more worrying aspect of this bug is as a vector of exploiting other bugs. This bug allows making the compositor use too-small memory buffers as sources i.e. it could be used to cause the compositor to read past the end of a buffer, possibly reading sensitive data, and then exploit another bug to get at the compositor output, to read that sensitive data.

The present patch makes gives a hint as to how to use the vector (how to feed bad data to the compositor) but does not contain a hint about any other bug that this vector could be used on, to gain access to sensitive data.

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?

All of them

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No backports; would be medium complexity to backport (two patches, not tiny ones)

How likely is this patch to cause regressions; how much testing does it need?

Medium risk (two patches, not tiny ones)

Comment 6

[Daniel Veditz [:dveditz]]

I assume we don't need this on old Firefox branches, we're not using IPC/sandboxing yet. Unsure if we want to backport this on older b2g given the size of the patch and the moderate security risk (unlike some of the others you found using faulty). More worthwhile on 1.3 than 1.2 I'd think, because that seems to be the branch with carrier interest.

sec-approval=dveditz

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e05ca48e6621
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5149dd54f49a

Comment 8

[Carsten Book [:Tomcat]]

landed on central
https://hg.mozilla.org/mozilla-central/rev/e05ca48e6621
https://hg.mozilla.org/mozilla-central/rev/5149dd54f49a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

If we were going to backport these to 1.2 and 1.3, that time was a long time ago.

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for desktop QA verification. FxOS QA may choose to verify at a later date.