1393362 - Unchecked size can lead to zero byte allocation or undefined behavior

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Julian Hector [:tedd] [:jhector]]

+++ This bug was initially created as a clone of Bug #1393357 +++

As part of the sandboxing effort, printing was remoted to the parent in order to allow enabling the content process sandbox.

For remote printing, the content process writes Moz2D commands to a file which is used in the chrome process to redraw the content that shall be printed.

When deserializing the commands, a compromised content process can supply incorrect or unexpected values as sizes for memory allocations happening in the chrome process, this could be leveraged to allocate zero bytes of memory, where a later write to that region results in a buffer overflow/out of bounds write, or memory leaks.

Depending on how we handle allocation failures (unless specifically marked as fallible and handled properly) supplying a negative value as size could lead to undefined behavior. (Unfortunately, I am not familiar with how we handle negative size values, regular C++ throws an exception when providing a negative size for new [] allocations)

However a zero byte allocation may be more severe if later on a certain size is expected.
The locations in question are [1][2][3] . 

Note, given the architecture complexity and the content <-> parent communication, I haven't debugged this manually. This bug is filed as a result of reading the code.

[1] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/gfx/2d/RecordedEventImpl.h#2433-2435
[2] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/gfx/2d/RecordedEventImpl.h#2572-2575
[3] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/gfx/2d/RecordedEventImpl.h#2746-2748

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Jim, is this going to affect 57 or is sandboxing not riding the trains yet? Should we track this?

Comment 2

[Jim Mathies [:jimm]]

(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #1)
> Jim, is this going to affect 57 or is sandboxing not riding the trains yet?
> Should we track this?

Sounds like this is a hardening bug. cc'ing Bob who worked on the serialization code so he's is aware of it.

Comment 3

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #2)
> (In reply to Panos Astithas [:past] (56 Regression Engineering Owner)
> (please ni?) from comment #1)
> > Jim, is this going to affect 57 or is sandboxing not riding the trains yet?
> > Should we track this?
> 
> Sounds like this is a hardening bug. cc'ing Bob who worked on the
> serialization code so he's is aware of it.

I didn't work on the original serialization code, although I used it for printing.
I had to change a few things to get it into a usable state for printing, mainly around being able to turn it on at any point and things that had been added to the DrawTarget API since it was first implemented.

milan - are you sure this is unaffected, this is used for printing?

I don't mind looking at fixing these if nobody is available.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Jim, is sandboxing at this level riding 57?

If it is, then this bug (and a bunch of others) would be fix-optional, as they seem not to be blocking the release.  If they are blocking the release, then we'd have to back out sandboxing, as we'd have no chance of getting to them all :)

Comment 5

[Bob Owen (:bobowen)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Jim, is sandboxing at this level riding 57?
> 
> If it is, then this bug (and a bunch of others) would be fix-optional, as
> they seem not to be blocking the release.  If they are blocking the release,
> then we'd have to back out sandboxing, as we'd have no chance of getting to
> them all :)

Printing's use of this started (at least on Windows) with Fx50 and is required for pretty much any sandboxing not just the things riding in 56 and 57.

Comment 6

[Haik Aftandilian [:haik]]

(In reply to Bob Owen (:bobowen) from comment #3)
> I don't mind looking at fixing these if nobody is available.

I could help too.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Jim, is sandboxing at this level riding 57?
> 
> If it is, then this bug (and a bunch of others) would be fix-optional, as
> they seem not to be blocking the release.  If they are blocking the release,
> then we'd have to back out sandboxing, as we'd have no chance of getting to
> them all :)

https://wiki.mozilla.org/Security/Sandbox#Current_Status

Yes, we have sandboxing active on all platforms in various release, including 57.

Backing out doesn't fix anything here imo. If we disable sandboxing, then any exploit in printing is available in an insecure content process. 

Flagging this for sandboxing triage, we'll discuss this week.

Comment 8

[Wennie]

Hi Milan:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 9

[Stephanie Ouillon [:arroway] (needinfo me)]

Jim, you mentionned a while ago that this bug would be discussed in the sandboxing triage meeting, what came out of it?

Comment 10

[Jim Mathies [:jimm]]

(In reply to Stephanie Ouillon [:arroway] from comment #9)
> Jim, you mentionned a while ago that this bug would be discussed in the
> sandboxing triage meeting, what came out of it?

This bug is about hardening the boundary between content processes and the chrome process. This work is important but not as high priority as removing attack surface in the content process, which is what we're concentrating on now.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this really sec-high given that it's based on the code inspection and has a number of unanswered questions of whether anything would go wrong?

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

This should not have been classified as sec-audit; which is for investigating a pattern of concern. If a bug is not certain to be valid, it should remain classified with a security rating until it can be definitively shown to be not applicable.

Haik, Bob could I ask you to look at this again to determine it's status?

Comment 13

[Daniel Veditz [:dveditz]]

It appears comment 11 was asking for a re-triage, which can be accomplished by removing the sec- keyword or asking security folks (needinfo, slack, matrix, etc). Doing that now. Moving the bug to a different keyword keeps it off the security team's radar. It does not look like the printing team was actually signing themselves up to do this investigation, nor was one done.

The various integer overflows and trusting of communication from a potentially compromised child process are still present

Comment 14

[Bob Owen (:bobowen)]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #12)

This should not have been classified as sec-audit; which is for investigating a pattern of concern. If a bug is not certain to be valid, it should remain classified with a security rating until it can be definitively shown to be not applicable.

Haik, Bob could I ask you to look at this again to determine it's status?

I'm not too sure about the talk about negative values for allocation in the description, I think these are all unsigned or used to calculate an unsigned, but I think new would convert to unsigned anyway, so either allocation will fail if fallible or we'll OOM crash.

In the examples given in the description two use fallible (now) and handle failure, so I think we can't do any more for them.
The other (and another I found, both below) would OOM crash.
I suppose they could be 0, in which case I think our allocator always allocates 1 byte and never returns null.
So, I think the subsequent reads (and memcpys) would just do nothing.

lsalzman - I guess we might want to make these fallible rather than potentially crash the GPU process or worse the parent process and either way test for 0 size to make it fail more clearly (assuming 0 is invalid), what do you think?
tjr - I let you decide whether this should still stay at sec-high

https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/gfx/2d/RecordedEventImpl.h#2416
https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/gfx/2d/RecordedEventImpl.h#3273

Comment 15

[Lee Salzman [:lsalzman]]

Making it fallible would be fine. I would rather leave open the possibility of mNumGlyphs == 0 as being a valid case. Then check if mNumGlyphs > 0 and mGlyphs == nullptr during playback, and if so, return false, because that would mean we failed to allocate...

Comment 16

[Jim Mathies [:jimm]]

Lee, this sounds like a pretty small change, can you pick this up?

Comment 17

[Lee Salzman [:lsalzman]]

Attachment: Bug 1393362 - Make more RecordedEvent implementations fallible. r?bobowen

Comment 18

[Lee Salzman [:lsalzman]]

Comment on attachment 9248337 [details]
Bug 1393362 - Make more RecordedEvent implementations fallible. r?bobowen

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unknown. Bug report is only hypothetical.
• 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
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely

Comment 19

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9248337 [details]
Bug 1393362 - Make more RecordedEvent implementations fallible. r?bobowen

Approved to land and request uplift

Comment 20

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

Make more RecordedEvent implementations fallible. r=bobowen
https://hg.mozilla.org/integration/autoland/rev/7a92d641690b9737c7a9d1d5cf35e8418d7f4884
https://hg.mozilla.org/mozilla-central/rev/7a92d641690b

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 22

[Lee Salzman [:lsalzman]]

Comment on attachment 9248337 [details]
Bug 1393362 - Make more RecordedEvent implementations fallible. r?bobowen

Beta/Release Uplift Approval Request

• User impact if declined: Potential underflow on deserialized values that could lead to OOMs or overreads.
• 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): This just adds some overflow checking to deserialization in cases that were previously undefined behavior.
• 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: Potential underflow on deserialized values that could lead to OOMs or overreads.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This just adds some overflow checking to deserialization in cases that were previously undefined behavior.
• String or UUID changes made by this patch:

Comment 23

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9248337 [details]
Bug 1393362 - Make more RecordedEvent implementations fallible. r?bobowen

Approved for 95 beta 5, thanks.

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3051a2a76b47

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9248337 [details]
Bug 1393362 - Make more RecordedEvent implementations fallible. r?bobowen

Approved for 91.4esr.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/7683a726aed3