293527 - "Save Image As" context menu allows to silently save executables instead of images

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Michael Krax]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

The context menu option "Save Image As" does not validate if the src image is
valid. By using absolute positioning and the moz-opacity filter an attacker can
easily fool the user to think he is going to save a valid image but is
downloading an executable file instead.

Since file extensions are default disabled on windows and an exe files can have
any file icon - including the standard image icon - the user can only tell the
difference when he recognizes that the save dialog file type drop down shows
"Application" instead of an image file. Unlikely average users will see that or
realize the consequences. Probably most poeple will just double-click the "image
file" they downloaded when they are going to view it and will run the executable
in that moment. 

Reproducible: Always

Steps to Reproduce:
1. Open http://bugzilla:[e-mail]/firesaving/
2. Follow instructions




It seems Firefox sometimes raises an "the web page might have been removed or
its name changed" error when supplying a larger exe directly as an image source
and then trying to save it (is that a bug or a security feature?). Anyway, an
attacker can work around this problem with a little php script like this:

$fp = fopen("exe.exe", "rb");
header("Content-Type: image/gif");
header("Content-Disposition: inline; filename=booom.exe");
fpassthru($fp);
fclose($fp);

Comment 1

[Daniel Veditz [:dveditz]]

Both IE and Opera recognize the top fake <IMG> didn't load (or, less likely,
that it was invisible) and disable their "Save Image" menu items.

Comment 2

[[e-mail]]

Attachment: Trunk patch

This seems to fix it on the trunk but I don't think it applies on the branch.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 188675 [details] [diff] [review]
Trunk patch

>Index: nsContextMenu.js
>+            if (request && (request.imageStatus & ~request.STATUS_ERROR))

Shouldn't that be |request && !(request.imageStatus & request.STATUS_ERROR)|? 
As written, if any status bit other than STATUS_ERROR is set, this will be
treated as a valid image....

Comment 4

[[e-mail]]

(In reply to comment #3)
>(From update of attachment 188675 [details] [diff] [review] [edit])
>>Index: nsContextMenu.js
>>+            if (request && (request.imageStatus & ~request.STATUS_ERROR))
>Shouldn't that be |request && !(request.imageStatus & request.STATUS_ERROR)|? 
>As written, if any status bit other than STATUS_ERROR is set, this will be
>treated as a valid image....
Your version treats STATUS_NONE as a valid image, which I don't think I want.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So add |&& request.imageStatus| to my version?

Comment 6

[[e-mail]]

Ok, so looking at how imgRequest works, I could choose either of the three
status flags depending on exactly what I want.
STATUS_SIZE_AVAILABLE - the image header was validated
STATUS_FRAME_COMPLETE - the image can be completely displayed
STATUS_LOAD_COMPLETE - the image has been completely downloaded

I couldn't understand what STATUS_LOAD_PARTIAL means.

Comment 7

[Boris Zbarsky [:bzbarsky]]

LOAD_PARTIAL means that the image is not done loading yet.  The flags are not
mutually exclusive...

Comment 8

[[e-mail]]

(In reply to comment #7)
>LOAD_PARTIAL means that the image is not done loading yet.
Hmm... actually it seems to me that it means that the load was stopped.
>The flags are not mutually exclusive...
So we only need the one that gets set first on a valid image.

Comment 9

[[e-mail]]

Attachment: Use STATUS_SIZE_AVAILABLE

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 188675 [details] [diff] [review]
Trunk patch

sr=bzbarsky with STATUS_SIZE_AVAILABLE

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE

r=biesi

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 188675 [details] [diff] [review]
Trunk patch

I assume this patch is obsolete now..

Comment 13

[[e-mail]]

Comment on attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE

Someone remind me to patch both xpfe and browser versions of this code when
actually checking this in.

Comment 14

[[e-mail]]

Comment on attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE

Transferring bz's sr.

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to comment #13)
> (From update of attachment 188699 [details] [diff] [review] [edit])
> Someone remind me to patch both xpfe and browser versions of this code when
> actually checking this in.

Don't forget the mail version, too.

Comment 16

[[e-mail]]

Fix checked in to the trunk, but I don't know what to do about the branches.

Comment 17

[Daniel Veditz [:dveditz]]

Caused regression bug 301329

Comment 18

[Tim Riley [:timr]]

Dan will create the right patch for 1.0.8 and others.

Comment 19

[[e-mail]]

(In reply to comment #18)
>Dan will create the right patch for 1.0.8 and others.
But hopefully without causing the same regression ;-)

Comment 20

[Daniel Veditz [:dveditz]]

Taking, hard to triage bugs for update releases when they're assigned to nobody.

Comment 21

[Daniel Veditz [:dveditz]]

Attachment: aviary101 branch backport, incorporating fix for 301329 regression

Comment 22

[Tracy Walker [:tracy]]

verified with:
Windows:
Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Firefox/1.0.8

Comment 23

[Daniel Veditz [:dveditz]]

> >Dan will create the right patch for 1.0.8 and others.
> But hopefully without causing the same regression ;-)

And I didn't, I created a completely different regression (bug 333035)

Comment 24

[Daniel Veditz [:dveditz]]

fixes for regression on regression on the aviary/moz17 branch ultimately wind up in bug 333305