1836550 - Potential Integer Overflow from malicious content process

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Tom Ritter [:tjr] (OOTO until April)]

In BrowserParent::RecvInvokeDragSession( we have the following code:

mozilla::ipc::IPCResult BrowserParent::RecvInvokeDragSession(
    nsTArray<IPCTransferableData>&& aTransferables, const uint32_t& aAction,
    Maybe<BigBuffer>&& aVisualDnDData, const uint32_t& aStride,
    const gfx::SurfaceFormat& aFormat, const LayoutDeviceIntRect& aDragRect,
    nsIPrincipal* aPrincipal, nsIContentSecurityPolicy* aCsp,
    const CookieJarSettingsArgs& aCookieJarSettingsArgs,
    const MaybeDiscarded<WindowContext>& aSourceWindowContext,
    const MaybeDiscarded<WindowContext>& aSourceTopWindowContext) {
  
  ...

  if (aVisualDnDData && aVisualDnDData->Size() >= aDragRect.height * aStride) {
    dragStartData->SetVisualization(gfx::CreateDataSourceSurfaceFromData(
        gfx::IntSize(aDragRect.width, aDragRect.height), aFormat,
        aVisualDnDData->Data(), aStride));
  }

aDragRect.height * aStride are attacker controlled, so they ought to be able to overflow, so aVisualDnDData->Size() >= aDragRect.height * aStride can be true even if aStride is very large and aVisualDnDData->Size() is small. If that is the case, we will eventually wind up with a raw pointer and a corresponding stride that is too large which will probably result in a buffer overread or overwrite somewhere.

I'm not certain this is an issue, but I'm doing some code inspection based on patterns of previous sandbox escapes, so I'd appreciate another set of eyes confirming this is or isn't an issue.

Comment 2

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Edgar, please help take a look, thanks.

Comment 3

[Edgar Chen [:edgar]]

Sure thing!

Comment 4

[Edgar Chen [:edgar]]

(In reply to Tom Ritter [:tjr] from comment #0)

so I'd appreciate another set of eyes confirming this is or isn't an issue.

Yes, this is an issue. We should validate the image data received from IPC.

Comment 5

[Tom Schuster]

We should look into using IPCImage for this instead.

Comment 6

[Edgar Chen [:edgar]]

(In reply to Tom Schuster (MoCo) from comment #5)

We should look into using IPCImage for this instead.

Yes, I have filed bug 1782043 for this.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Bug 1836550 - Validate image data received from IPC; r?NeilDeakin!

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 9339573 [details]
Bug 1836550 - Validate image data received from IPC; r?NeilDeakin!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I don't think it is 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?: 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?: should be applied cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely, it just validates IPC image data.
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9339573 [details]
Bug 1836550 - Validate image data received from IPC; r?NeilDeakin!

Approved to request uplift and land

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 9339573 [details]
Bug 1836550 - Validate image data received from IPC; r?NeilDeakin!

Beta/Release Uplift Approval Request

• User impact if declined: potential integer overflow from malicious content process
• 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: None
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Should be low, it just validates IPC image data.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 11

[Edgar Chen [:edgar]]

(In reply to Tom Ritter [:tjr] from comment #0)

In BrowserParent::RecvInvokeDragSession()
aDragRect.height * aStride are attacker controlled, so they ought to be able to overflow, so aVisualDnDData->Size() >= aDragRect.height * aStride can be true even if aStride is very large and aVisualDnDData->Size() is small. If that is the case, we will eventually wind up with a raw pointer and a corresponding stride that is too large which will probably result in a buffer overread or overwrite somewhere.

Hi Christian, do you know if this is IPC fuzzing detectable? Thanks!

Comment 12

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

Validate image data received from IPC; r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/f2602b49e28a4ec7acfef8cd19566af364c489f8
https://hg.mozilla.org/mozilla-central/rev/f2602b49e28a

Comment 13

[Donal Meehan [:dmeehan]]

Comment on attachment 9339573 [details]
Bug 1836550 - Validate image data received from IPC; r?NeilDeakin!

Approved for 115.0b9.

Comment 14

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0aae73035448

Comment 15

[Donal Meehan [:dmeehan]]

:edgar could you add an esr102 uplfit request on this? It will need a patch based on esr102

Comment 16

[Edgar Chen [:edgar]]

Attachment: [esr102] Bug 1836550 - Validate image data received from IPC; r=NeilDeakin!

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 9340479 [details] [diff] [review]
[esr102] Bug 1836550 - Validate image data received from IPC; r=NeilDeakin!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug
• User impact if declined: Potential integer overflow from malicious content process
• Fix Landed on Version: 116
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Should be low, it just validates IPC image data.

Comment 18

[Donal Meehan [:dmeehan]]

Comment on attachment 9340479 [details] [diff] [review]
[esr102] Bug 1836550 - Validate image data received from IPC; r=NeilDeakin!

Approved for 102.13esr.

Comment 19

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/00297c9943e1

Comment 20

[Christian Holler (:decoder)]

(In reply to Edgar Chen [:edgar] from comment #11)

(In reply to Tom Ritter [:tjr] from comment #0)

In BrowserParent::RecvInvokeDragSession()

Hi Christian, do you know if this is IPC fuzzing detectable? Thanks!

It could be and I want to give it a try in a targeted way. However, I found out that this method has no test coverage, is that possible?

See also https://coverage.moz.tools/#revision=latest&path=dom%2Fipc%2FBrowserParent.cpp&view=file&line=3825

Comment 21

[Edgar Chen [:edgar]]

(In reply to Christian Holler (:decoder) from comment #20)

I found out that this method has no test coverage, is that possible?

Yes, It is hard to test drag and drop on test framework.