1523362 - heap buffer overflow read in TabParent::RecvSetCursor

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

I noticed this when looking at the commit for bug 1520502.

https://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#1655-1660

This passes aCursor.BeginReading() and size. However it's never verified that aCursor's length matches size.

This can lead to a heap-buffer-overflow read from a compromised child.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

This was already pre-existing, right? But sure, should be fixable, want to give it a go? Otherwise I can write the patch.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Yes, the previous version of TabParent::RecvSetCustomCursor also had this issue, I just noticed it because of that bug.

I don't know these graphics APIs well, I'd be appreciative if someone else could take a look.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Mkay.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1523362 - Validate cursor data in TabParent::RecvSetCursor. r=tnikkel

I think the stride < width * bpp is the right thing to check for, since I
don't know if there's any guarantee of it of the stride being equal, but let me
know if I'm wrong.

Comment 6

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

https://hg.mozilla.org/integration/autoland/rev/3e903717a513be0e690e6c99bf64435d24b83052
https://hg.mozilla.org/mozilla-central/rev/3e903717a513

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. Also, does this issue pre-date the changes from bug 1520502?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Also, does
this issue pre-date the changes from bug 1520502?

Yes it does. That just moved the code.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

OK, not sure it's worth rebasing for an ESR60 uplift or not. Will defer to Emilio on that one :).

Comment 10

[Emilio Cobos Álvarez [:emilio]]

301 Alex there on whether it's worth uplifting to ESR. Will submit the beta uplift request in a sec.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9039598 [details]
Bug 1523362 - Validate cursor data in TabParent::RecvSetCursor. r=tnikkel

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Not sure

User impact if declined

Compromised content process could make the parent process read out-of-bounds data.

Is this code covered by automated tests?

No

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)

Just sanity-checks the data.

String changes made/needed

none

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

I think the rebase should be straight forward, so +1 on uplifting to ESR -- if it's actually a mess for some reason we can reconsider.

Comment 13

[Liz Henry (:lizzard)]

I think I just saw some related code land on beta yesterday. in https://bugzilla.mozilla.org/show_bug.cgi?id=1431582#c29 it looks like one of the same test is enabled already (b/gfx/wr/wrench/reftests/split/reftest.list). Will that matter for your uplift?

Comment 14

[Emilio Cobos Álvarez [:emilio]]

That code is not related at all. This is not WebRender related whatsoever.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9039598 [details]
Bug 1523362 - Validate cursor data in TabParent::RecvSetCursor. r=tnikkel

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

I guess Alex should fill this, but it's a trivial IPC hardening patch.

User impact if declined

Compromised content process could make the parent process read out-of-bounds data.

Fix Landed on Version

67

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Just sanity-checks the data.

String or UUID changes made by this patch

none

Comment 16

[Liz Henry (:lizzard)]

Comment on attachment 9039598 [details]
Bug 1523362 - Validate cursor data in TabParent::RecvSetCursor. r=tnikkel

Fix for potential sec issue, OK for uplift.
This should land for Monday's beta 5 build.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/575fb73398a7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9039598 [details]
Bug 1523362 - Validate cursor data in TabParent::RecvSetCursor. r=tnikkel

Please create a rebased patch for ESR60 approval consideration.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0001-Validate-cursor-data-in-TabParent-RecvSetCustomCurso.patch

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9047471 [details] [diff] [review]
0001-Validate-cursor-data-in-TabParent-RecvSetCustomCurso.patch

Adds some simple sanity checking to avoid a security situation. Baked on Nightly and Beta for awhile. Approved for 60.6esr.

Comment 21

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

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

Comment 22

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

Backed out for bustage in TabParent.cpp:

https://hg.mozilla.org/releases/mozilla-esr60/rev/8aa97b463235220d12143584febc7bcb567485d5

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&group_state=expanded&revision=e8ac8f5f87ccb21da0881fb5a8da2103dcc875fc
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=231727146&repo=mozilla-esr60

/builds/worker/workspace/build/src/dom/ipc/TabParent.cpp:1601:45: error: cannot initialize a parameter of type 'mozilla::gfx::SurfaceFormat' with an lvalue of type 'const uint8_t' (aka 'const unsigned char')

Comment 23

[Emilio Cobos Álvarez [:emilio]]

Attachment: Fixed ESR patch

Validate aFormat too.

Comment 24

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

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