The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Firefox
General
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Michael Krax, Assigned: dveditz)

Tracking

({verified1.7.13})

unspecified
x86
Windows XP
verified1.7.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0.8 +
blocking1.8b5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:spoof], URL)

Attachments

(3 attachments)

(Reporter)

Description

12 years ago
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:Ha8T4wc@www.mikx.de/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);
(Assignee)

Updated

12 years ago
Blocks: 256195
(Assignee)

Comment 1

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b4+
Whiteboard: [sg:spoof]

Comment 2

12 years ago
Created attachment 188675 [details] [diff] [review]
Trunk patch

This seems to fix it on the trunk but I don't think it applies on the branch.
Attachment #188675 - Flags: superreview?(bzbarsky)
Attachment #188675 - Flags: review?(cbiesinger)
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

12 years ago
(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.
So add |&& request.imageStatus| to my version?

Comment 6

12 years ago
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.
LOAD_PARTIAL means that the image is not done loading yet.  The flags are not
mutually exclusive...

Comment 8

12 years ago
(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

12 years ago
Created attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE
Comment on attachment 188675 [details] [diff] [review]
Trunk patch

sr=bzbarsky with STATUS_SIZE_AVAILABLE
Attachment #188675 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE

r=biesi
Attachment #188699 - Flags: review+
Comment on attachment 188675 [details] [diff] [review]
Trunk patch

I assume this patch is obsolete now..
Attachment #188675 - Flags: review?(cbiesinger)
(Assignee)

Updated

12 years ago
Flags: blocking-aviary1.0.6+

Comment 13

12 years ago
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

12 years ago
Comment on attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE

Transferring bz's sr.
Attachment #188699 - Flags: superreview+
Attachment #188699 - Flags: approval-aviary1.1a2?
(Assignee)

Comment 15

12 years ago
(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.

Updated

12 years ago
Attachment #188699 - Flags: approval-aviary1.1a2? → approval1.8b4+

Comment 16

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

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 301329
(Assignee)

Comment 17

12 years ago
Caused regression bug 301329

Updated

11 years ago
Flags: testcase+

Comment 18

11 years ago
Dan will create the right patch for 1.0.8 and others.

Comment 19

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

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

11 years ago
Taking, hard to triage bugs for update releases when they're assigned to nobody.
Assignee: nobody → dveditz
Status: REOPENED → NEW
(Assignee)

Comment 21

11 years ago
Created attachment 211253 [details] [diff] [review]
aviary101 branch backport, incorporating fix for 301329 regression
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago11 years ago
Keywords: fixed-aviary1.0.8, fixed1.7.13
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Keywords: fixed-aviary1.0.8, fixed1.7.13 → verified-aviary1.0.8, verified1.7.13
(Assignee)

Comment 23

11 years ago
> >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)
(Assignee)

Comment 24

11 years ago
fixes for regression on regression on the aviary/moz17 branch ultimately wind up in bug 333305
(Assignee)

Updated

11 years ago
Group: security

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.