Closed Bug 1318736 Opened 3 years ago Closed 3 years ago

Tricking user into creating an executable when copy pasting an image into folder

Categories

(Core :: Security, defect)

51 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 + verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: qab, Assigned: hectorz)

References

Details

(Keywords: csectype-spoof, regression, sec-low)

Attachments

(3 files, 1 obsolete file)

Attached file hell.html
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

This is similar to Bug 1317873 but I think has a different fix since it does not work on latest stable, but works on latest nightly (with and without multi process)


1. Open attached PoC and make sure to point the second image to a valid url to a random executable.

2. Right click and choose to copy the image

3. Go to a random directory on your computer and paste

This bug alongside Bug 1317873 makes this a very serious vulnerability.


Actual results:

We end up with an executable with both the filename and its contents are under our control.


Expected results:

If you run this on latest stable, it doesnt work. Seems like there is a regression, will post a regression range soon.
One fix (which is what chrome does) is disable the option to copy the image if the image is invalid.
Attached image mozreg.png
First known bad (where the PoC works) build date is: 2016-08-04

Here is the info from mozregression: (Just to be safe i will post a screenshot of mozregression incase I missed something)

2016-11-18T13:10:16: INFO : application_buildid: 20160802034849
2016-11-18T13:10:16: INFO : application_changeset: 71c8d652e638dd67c5c56879df83d76b30033458
2016-11-18T13:10:16: INFO : application_display_name: Nightly
2016-11-18T13:10:16: INFO : application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
2016-11-18T13:10:16: INFO : application_name: Firefox
2016-11-18T13:10:16: INFO : application_remotingname: firefox
2016-11-18T13:10:16: INFO : application_repository: https://hg.mozilla.org/integration/fx-team
2016-11-18T13:10:16: INFO : application_vendor: Mozilla
2016-11-18T13:10:16: INFO : application_version: 51.0a1
2016-11-18T13:10:16: INFO : platform_buildid: 20160802034849
2016-11-18T13:10:16: INFO : platform_changeset: 71c8d652e638dd67c5c56879df83d76b30033458
2016-11-18T13:10:16: INFO : platform_repository: https://hg.mozilla.org/integration/fx-team
2016-11-18T13:10:16: INFO : platform_version: 51.0a1
2016-11-18T13:10:31: INFO : Narrowed inbound regression window from [46045ec8, 2565f0ee] (3 revisions) to [46045ec8, 71c8d652] (2 revisions) (~1 steps left)
2016-11-18T13:10:31: DEBUG : Starting merge handling...
2016-11-18T13:10:31: DEBUG : Using url: https://hg.mozilla.org/integration/fx-team/json-pushes?changeset=71c8d652e638dd67c5c56879df83d76b30033458&full=1
2016-11-18T13:10:32: DEBUG : Found commit message:
Bug 1291009 - Fix closed caption button icon in hidpi mode. r=jaws

MozReview-Commit-ID: lDumJxyzTr

2016-11-18T13:10:32: INFO : The bisection is done.
2016-11-18T13:10:32: INFO : Stopped
Hector, can you investigate? This looks like a consequence of the requestingPrincipal changes.
Flags: needinfo?(bzhao)
Well, or maybe part 3 that includes a "File" promise...
(In reply to :Gijs Kruitbosch from comment #5)
> Well, or maybe part 3 that includes a "File" promise...

I think it's part 3.

Is adding sth. like a `nsContentUtils::IsFlavorImage` check inside `AppendImagePromise` good enough? I'm not quite sure what's the best way to decide whether an image is valid.
Flags: needinfo?(bzhao)
(In reply to Hector Zhao [:hectorz] from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > Well, or maybe part 3 that includes a "File" promise...
> 
> I think it's part 3.
> 
> Is adding sth. like a `nsContentUtils::IsFlavorImage` check inside
> `AppendImagePromise` good enough? I'm not quite sure what's the best way to
> decide whether an image is valid.

The context menu code has an onLoadedImage check which is based on whether the image has size available, and another one for onCompletedImage that checks whether the image has been loaded without an error ( https://dxr.mozilla.org/mozilla-central/rev/0534254e9a40b4bade2577c631fe4cfa0b5db41d/browser/base/content/nsContextMenu.js#697-705 ). I assume you could employ the same checks in the C++ code that adds the image promise here. I don't know if there's something better. Seth's gone, so not sure who to ask.

That said, I wonder if we should also just disable the 'copy image' context menu if we're not on a completed / loaded image. Apparently 'copy image' is available even for non-loaded images because it used to be possible to do image blocking (it's still... kind of possible... but we've removed some of the main UI from the preferences/settings) - that happened back in bug 301329. I wonder if we can tell the 'blocked' and 'error, wrong mimetype / broken file' case apart in the frontend...
Group: firefox-core-security → core-security
Component: Untriaged → Security
Product: Firefox → Core
Version: 52 Branch → 51 Branch
Group: core-security → dom-core-security
Timothy, can you answer the question in comment 7?
Flags: needinfo?(tnikkel)
(In reply to :Gijs Kruitbosch from comment #7)
> The context menu code has an onLoadedImage check which is based on whether
> the image has size available, and another one for onCompletedImage that
> checks whether the image has been loaded without an error (
> https://dxr.mozilla.org/mozilla-central/rev/
> 0534254e9a40b4bade2577c631fe4cfa0b5db41d/browser/base/content/nsContextMenu.
> js#697-705 ). I assume you could employ the same checks in the C++ code that
> adds the image promise here. I don't know if there's something better.
> Seth's gone, so not sure who to ask.

Yeah, I think that's the best way.

Note that STATUS_LOAD_COMPLETE only means we have all the data of the image from the network. If you want to make sure the whole image decodes successfully there is STATUS_DECODE_COMPLETE. STATUS_SIZE_AVAILABLE means that the header of the image is well-formed enough to get a size from it. Not all images get decoded, but if the image is visible for a context menu to appear on it then it should at least have started decoding.
Flags: needinfo?(tnikkel)
Attached patch Patch (obsolete) — Splinter Review
(In reply to Timothy Nikkel (:tnikkel) from comment #9)
> 
> Note that STATUS_LOAD_COMPLETE only means we have all the data of the image
> from the network. If you want to make sure the whole image decodes
> successfully there is STATUS_DECODE_COMPLETE. STATUS_SIZE_AVAILABLE means
> that the header of the image is well-formed enough to get a size from it.
> Not all images get decoded, but if the image is visible for a context menu
> to appear on it then it should at least have started decoding.

Thanks for the explanations! I wanted to use the most strict STATUS_DECODE_COMPLETE, but worried about it taking too long for an animated gif. I'm using STATUS_FRAME_COMPLETE in my patch, hopefully it's the right balance here.
Attachment #8816358 - Flags: review?(tnikkel)
Assignee: nobody → bzhao
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8816358 [details] [diff] [review]
Patch

I'd also prefer you check that we don't have STATUS_ERROR just in case.
Attachment #8816358 - Flags: review?(tnikkel)
Would using the approach used in bug 279945 bypass the potential fix here? It uses a GIF/batch file polyglot I believe. 

Would it be too much to change any known executable extensions to blank/the image type sniffed? There is a behavior when we try drag and dropping valid images that end with say '.exe' where the resulting file does not have that extension. Wonder if something like that could be implemented here as well?
(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> 
> I'd also prefer you check that we don't have STATUS_ERROR just in case.

Updated

(In reply to Abdulrahman Alqabandi[test] from comment #12)
> Would using the approach used in bug 279945 bypass the potential fix here?
> It uses a GIF/batch file polyglot I believe. 

I think the fix for bug 279945 was superseded by bug 280947[1], whose logic is copied into bug 664717[2].

[1]: https://github.com/mozilla/gecko-dev/commit/e4a310c9c76ba4b5b9ad0ecaf201399b3c27b270
[2]: https://hg.mozilla.org/mozilla-central/rev/457ebb2b73ad
Attachment #8816358 - Attachment is obsolete: true
Attachment #8816370 - Flags: review?(tnikkel)
Attachment #8816370 - Flags: review?(tnikkel) → review+
I'm not familiar with handling of security related bugs. Is it okay to push the patch to try server? Or should I request sec-approval first?
(In reply to Hector Zhao [:hectorz] from comment #14)
> I'm not familiar with handling of security related bugs. Is it okay to push
> the patch to try server? Or should I request sec-approval first?

This needs a sec rating. Personally I don't expect sec-high/sec-crit, so you should be OK, but let's check with Dan and Al.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
This is probably a sec-moderate at best so I think you're ok to push to try.
Flags: needinfo?(abillings)
Flags: needinfo?(dveditz)
Track 51+ as regression.
https://hg.mozilla.org/mozilla-central/rev/701f4573997e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hi :hectorz, 
Is this worth uplifting to Beta51 & Aurora52?
Flags: needinfo?(bzhao)
(In reply to Gerry Chang [:gchang] from comment #21)
> Hi :hectorz, 
> Is this worth uplifting to Beta51 & Aurora52?

Yes, and I'll request the approvals once I verify the fix with a Nightly build.
Flags: needinfo?(bzhao)
Comment on attachment 8816370 [details] [diff] [review]
Patch, with imgIRequest::STATUS_ERROR check

Approval Request Comment
[Feature/Bug causing the regression]: Bug 664717
[User impact if declined]: See comment 0
[Is this code covered by automated tests?]: No, no automated tests were created in bug 664717
[Has the fix been verified in Nightly?]: Yes, manually with attachment 8812274 [details].
[Needs manual test from QE? If yes, steps to reproduce]: No? (I can do the verifications unless it's required to be verified by someone other than the patch author)
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just one more check inside the logics newly introduced in bug 664717
[String changes made/needed]: None
Attachment #8816370 - Flags: approval-mozilla-beta?
Attachment #8816370 - Flags: approval-mozilla-aurora?
Verified with Nightly 53.0a1 build 20161214030231 on Windows 10 64 bit
Status: RESOLVED → VERIFIED
Group: dom-core-security → core-security-release
Comment on attachment 8816370 [details] [diff] [review]
Patch, with imgIRequest::STATUS_ERROR check

Fix a regression and was verified. Beta51+ & Aurora52+. Should be in 51 beta 8.
Attachment #8816370 - Flags: approval-mozilla-beta?
Attachment #8816370 - Flags: approval-mozilla-beta+
Attachment #8816370 - Flags: approval-mozilla-aurora?
Attachment #8816370 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Verified with DevEdition 52.0a2 build 20161218004028 on Windows 10 64 bit
Verified with Beta 51.0b9 build 20161219140923 on Windows 10 64 bit
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.