Closed Bug 1473161 Opened Last year Closed Last year

Missing bounds checks in nsContentUtils::DataTransferItemToImage

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ fixed
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed

People

(Reporter: jld, Assigned: annyG)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(3 obsolete files)

(In reply to :Nika Layzell from bug 1467889 comment #1)
> It appears we also don't check the size of the shared memory buffer we use
> for images in our image translation code, which means that a compromised
> content process can probably trigger out of memory reads in the parent
> process:
> 
> https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/dom/base/nsContentUtils.cpp#7894-7906

I'm filing a separate bug for the apparent bounds-checking problems with copy/paste of images, because that will probably take longer to fix than the rest of what's reported in bug 1467889.  (I'm calling that “apparent” because I haven't tried to construct a proof-of-concept, but I looked through the code and I also didn't see any bounds checks.)
Priority: -- → P2
Assignee: nobody → agakhokidze
MozReview-Commit-ID: 3dq5yWZwJiG
Attachment #8991321 - Attachment is obsolete: true
MozReview-Commit-ID: 3dq5yWZwJiG
Before checking-in please request sec-approval as described at https://wiki.mozilla.org/Security/Bug_Approval_Process (also linked to at the bottom of the attachment table).
Flags: needinfo?(agakhokidze)
Comment on attachment 8991330 [details]
Bug 1473161 - Add missing bound check in nsContentUtils::DataTransferItemToImage, r=nika

:Nika Layzell has approved the revision.

https://phabricator.services.mozilla.com/D2074
Attachment #8991330 - Flags: review+
Comment on attachment 8991330 [details]
Bug 1473161 - Add missing bound check in nsContentUtils::DataTransferItemToImage, r=nika

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It's difficult to construct an exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The check in comment mentions that we are adding a missing bound check.

Which older supported branches are affected by this flaw?   This bound check never existed before.

If not all supported branches, which bug introduced the flaw? n/a

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

How likely is this patch to cause regressions; how much testing does it need? This patch is unlikely to cause regression.
Flags: needinfo?(agakhokidze)
Attachment #8991330 - Flags: sec-approval?
(In reply to Anny Gakhokidze [:annyG] from comment #5)

> Which older supported branches are affected by this flaw?   This bound check
> never existed before.
> 
> If not all supported branches, which bug introduced the flaw? n/a
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be? n/a

How does this match the firefox "affected" flags above that say it goes back to at least Firefox 52 as a problem?

Your responses would mean that there is no reason to backport this as this is nightly only code (which would also not need sec-approval). If the bounds checker is fixing a longstanding flaw, the intention of the questions is "how long have we had the flaw and in which branches?"
Flags: needinfo?(agakhokidze)
> Which older supported branches are affected by this flaw?   This bound check
> never existed before.

To clarify this based on the previous comment, this bug affects all branches.

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

anny: You should say something along the lines of "this patch should either already apply or be fairly trivial to apply for all supported branches". Don't say "n/a" if other branches are affected.
Flags: needinfo?(agakhokidze)
Sec-approval+ for trunk. We'll want beta and ESR60 patches made and nominated as well.
Attachment #8991330 - Flags: sec-approval? → sec-approval+
Nika, from what Al said, my understanding is that I need to create patches for Firefox ESR 60.0 and 60.1, as well as Firefox 62 beta. So then, in git terms, I should be checking out branches bookmarks/beta, bookmarks/esr60, and creating a patch for those commits. Is my understanding correct?
Flags: needinfo?(nika)
60.1 is a release branch build. I don't think this needs to be made for release, just Beta and ESR60. We aren't going to emergency ship this as far as I know.
(In reply to Anny Gakhokidze [:annyG] from comment #9)
> Nika, from what Al said, my understanding is that I need to create patches
> for Firefox ESR 60.0 and 60.1, as well as Firefox 62 beta. So then, in git
> terms, I should be checking out branches bookmarks/beta, bookmarks/esr60,
> and creating a patch for those commits. Is my understanding correct?

Yes, you need to create patches for ESR 60 (https://hg.mozilla.org/releases/mozilla-esr60/) and Beta (https://hg.mozilla.org/releases/mozilla-beta/), post the updated patches & request uplift on them.
Flags: needinfo?(nika)
If this code looks basically the same on those other branches, you don't need to bother creating separate patches. Usually people just request uplift, and then if the existing patch fails to apply, they'll update it. Of course, if you know in advance the patch won't apply, it would be good to make the patch yourself before that.
Comment on attachment 8991330 [details]
Bug 1473161 - Add missing bound check in nsContentUtils::DataTransferItemToImage, r=nika

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: potential security in a parent process that is controlled by a child process
[Is this code covered by automated tests?]: the modified code path is encountered by the automated tests
[Has the fix been verified in Nightly?]: no, but this patch could be landed on nightly
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]:no
[Why is the change risky/not risky?]: it's a small change - just adding bounds check
[String changes made/needed]: none

This patch can be applied for both beta and Nightly
Attachment #8991330 - Flags: approval-mozilla-beta?
Comment on attachment 8992650 [details]
Bug 1473161 - Add missing bound check in nsContentUtils::DataTransferItemToImage, r=nika

[Approval Request Comment]
sec-high bug, see above

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8992650 - Flags: approval-mozilla-esr60?
https://hg.mozilla.org/mozilla-central/rev/a9f3535ee3fd
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8991330 [details]
Bug 1473161 - Add missing bound check in nsContentUtils::DataTransferItemToImage, r=nika

Fixes a sec-high. Approved for 62.0b10 and ESR 60.2.
Attachment #8991330 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8992650 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment on attachment 8992650 [details]
Bug 1473161 - Add missing bound check in nsContentUtils::DataTransferItemToImage, r=nika

:Nika Layzell has approved the revision.

https://phabricator.services.mozilla.com/D2185
Attachment #8992650 - Flags: review+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Depends on: CVE-2018-18498
Component: DOM → DOM: Core & HTML
Attachment #8992650 - Attachment is obsolete: true
Attachment #8991330 - Attachment is obsolete: true
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.