Closed
Bug 1473161
Opened 6 years ago
Closed 6 years ago
Missing bounds checks in nsContentUtils::DataTransferItemToImage
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
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.)
Updated•6 years ago
|
Priority: -- → P2
Updated•6 years ago
|
Keywords: csectype-bounds,
sec-high
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → agakhokidze
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: 3dq5yWZwJiG
Updated•6 years ago
|
Attachment #8991321 -
Attachment is obsolete: true
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: 3dq5yWZwJiG
Comment 3•6 years ago
|
||
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)
Updated•6 years ago
|
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
status-firefox63:
--- → affected
status-firefox-esr52:
--- → affected
status-firefox-esr60:
--- → affected
tracking-firefox62:
--- → +
tracking-firefox63:
--- → +
tracking-firefox-esr60:
--- → 62+
Comment 4•6 years ago
|
||
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+
Assignee | ||
Comment 5•6 years ago
|
||
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?
Comment 6•6 years ago
|
||
(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)
Comment 7•6 years ago
|
||
> 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)
Comment 8•6 years ago
|
||
Sec-approval+ for trunk. We'll want beta and ESR60 patches made and nominated as well.
Updated•6 years ago
|
Updated•6 years ago
|
Attachment #8991330 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 9•6 years ago
|
||
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)
Comment 10•6 years ago
|
||
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.
Comment 11•6 years ago
|
||
(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)
Comment 12•6 years ago
|
||
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.
Assignee | ||
Comment 13•6 years ago
|
||
MozReview-Commit-ID: BE8e0lBjMRt
Assignee | ||
Comment 14•6 years ago
|
||
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?
Assignee | ||
Comment 15•6 years ago
|
||
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?
Comment 16•6 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/a9f3535ee3fd0d5834490478f3f72e7e197d6bdf
![]() |
||
Comment 17•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a9f3535ee3fd
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 18•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8992650 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
![]() |
||
Comment 19•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a85b6c7cc418
Comment 20•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/e48cec0a46e2
Comment 21•6 years ago
|
||
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+
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Updated•6 years ago
|
Depends on: CVE-2018-18498
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•6 years ago
|
Attachment #8992650 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8991330 -
Attachment is obsolete: true
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•