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

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Abdulrahman Alqabandi]

Attachment: 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.

Comment 1

[Abdulrahman Alqabandi]

One fix (which is what chrome does) is disable the option to copy the image if the image is invalid.

Comment 2

[Abdulrahman Alqabandi]

Attachment: 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

Comment 3

[Abdulrahman Alqabandi]

last pushlog url: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=46045ec8a4aa09a341b7209170089ade3c27c1a5&tochange=71c8d652e638dd67c5c56879df83d76b30033458

Comment 4

[:Gijs (he/him)]

Hector, can you investigate? This looks like a consequence of the requestingPrincipal changes.

Comment 5

[:Gijs (he/him)]

Well, or maybe part 3 that includes a "File" promise...

Comment 6

[Hector Zhao [:hectorz]]

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

Comment 7

[:Gijs (he/him)]

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

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Timothy, can you answer the question in comment 7?

Comment 9

[Timothy Nikkel (: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.

Comment 10

[Hector Zhao [:hectorz]]

Attachment: Patch

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

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8816358 [details] [diff] [review]
Patch

I'd also prefer you check that we don't have STATUS_ERROR just in case.

Comment 12

[Abdulrahman Alqabandi]

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?

Comment 13

[Hector Zhao [:hectorz]]

Attachment: Patch, with imgIRequest::STATUS_ERROR check

(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

Comment 14

[Hector Zhao [:hectorz]]

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?

Comment 15

[Abdulrahman Alqabandi]

Maybe this will help https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 16

[:Gijs (he/him)]

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

Comment 17

[Al Billings [:abillings - ex-MoCo]]

This is probably a sec-moderate at best so I think you're ok to push to try.

Comment 18

[Hector Zhao [:hectorz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6af31c494313e9f23732d04ef0a7b1db871eb8f8

Comment 19

[Gerry Chang [:gchang]]

Track 51+ as regression.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/701f4573997e

Comment 21

[Gerry Chang [:gchang]]

Hi :hectorz, 
Is this worth uplifting to Beta51 & Aurora52?

Comment 22

[Hector Zhao [:hectorz]]

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

Comment 23

[Hector Zhao [:hectorz]]

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

Comment 24

[Hector Zhao [:hectorz]]

Verified with Nightly 53.0a1 build 20161214030231 on Windows 10 64 bit

Comment 25

[Gerry Chang [:gchang]]

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.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ab8259a79df2

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d1fa1d56e38

Comment 28

[Hector Zhao [:hectorz]]

Verified with DevEdition 52.0a2 build 20161218004028 on Windows 10 64 bit

Comment 29

[Hector Zhao [:hectorz]]

Verified with Beta 51.0b9 build 20161219140923 on Windows 10 64 bit