Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧ Reporter
	Description • 6 years ago

(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.)

	Anny Gakhokidze [:annyG] Assignee
	Comment 1 • 6 years ago

MozReview-Commit-ID: 3dq5yWZwJiG

	Anny Gakhokidze [:annyG] Assignee
	Comment 2 • 6 years ago

MozReview-Commit-ID: 3dq5yWZwJiG

	Daniel Veditz [:dveditz]
	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).

	Phabricator Automation
	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

	Anny Gakhokidze [:annyG] 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.

	Al Billings [:abillings - ex-MoCo]
	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?"

	Nika Layzell [:nika] (ni? for response)
	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.

	Al Billings [:abillings - ex-MoCo]
	Comment 8 • 6 years ago

Sec-approval+ for trunk. We'll want beta and ESR60 patches made and nominated as well.

	Anny Gakhokidze [:annyG] 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?

	Al Billings [:abillings - ex-MoCo]
	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.

	Nika Layzell [:nika] (ni? for response)
	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.

	Andrew McCreight [:mccr8]
	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.

	Anny Gakhokidze [:annyG] Assignee
	Comment 13 • 6 years ago

MozReview-Commit-ID: BE8e0lBjMRt

	Anny Gakhokidze [:annyG] 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

	Anny Gakhokidze [:annyG] 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago

https://hg.mozilla.org/integration/autoland/rev/a9f3535ee3fd0d5834490478f3f72e7e197d6bdf

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 17 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/a9f3535ee3fd

	Ryan VanderMeulen [:RyanVM]
	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.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 19 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/a85b6c7cc418

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago
uplift

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

	Phabricator Automation
	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