Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

()

Firefox
General
VERIFIED FIXED
12 years ago
11 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);
Blocks: 256195
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 3

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

Comment 5

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

Comment 7

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

12 years ago
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)
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?
(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
Caused regression bug 301329

Updated

12 years ago
Flags: testcase+

Comment 18

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

Comment 19

12 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 ;-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Taking, hard to triage bugs for update releases when they're assigned to nobody.
Assignee: nobody → dveditz
Status: REOPENED → NEW
Created attachment 211253 [details] [diff] [review]
aviary101 branch backport, incorporating fix for 301329 regression
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 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
> >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)
fixes for regression on regression on the aviary/moz17 branch ultimately wind up in bug 333305
Group: security

Updated

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