Last Comment Bug 293527 - "Save Image As" context menu allows to silently save executables instead of images
: "Save Image As" context menu allows to silently save executables instead of i...
Status: VERIFIED FIXED
[sg:spoof]
: verified1.7.13
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
http://bugzilla:Ha8T4wc@www.mikx.de/f...
Depends on: 301329
Blocks: sbb?
  Show dependency treegraph
 
Reported: 2005-05-09 14:25 PDT by Michael Krax
Modified: 2007-04-01 15:03 PDT (History)
7 users (show)
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.8b5+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Trunk patch (2.13 KB, patch)
2005-07-08 06:38 PDT, neil@parkwaycc.co.uk
bzbarsky: superreview+
Details | Diff | Review
Use STATUS_SIZE_AVAILABLE (2.14 KB, patch)
2005-07-08 11:55 PDT, neil@parkwaycc.co.uk
cbiesinger: review+
neil: superreview+
asa: approval1.8b4+
Details | Diff | Review
aviary101 branch backport, incorporating fix for 301329 regression (9.92 KB, patch)
2006-02-09 04:55 PST, Daniel Veditz [:dveditz]
no flags Details | Diff | Review

Description Michael Krax 2005-05-09 14:25:20 PDT
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);
Comment 1 Daniel Veditz [:dveditz] 2005-07-07 17:45:17 PDT
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 neil@parkwaycc.co.uk 2005-07-08 06:38:26 PDT
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.
Comment 3 Boris Zbarsky [:bz] 2005-07-08 08:28:13 PDT
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 neil@parkwaycc.co.uk 2005-07-08 09:16:38 PDT
(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 [:bz] 2005-07-08 09:23:37 PDT
So add |&& request.imageStatus| to my version?
Comment 6 neil@parkwaycc.co.uk 2005-07-08 09:49:58 PDT
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 [:bz] 2005-07-08 10:46:31 PDT
LOAD_PARTIAL means that the image is not done loading yet.  The flags are not
mutually exclusive...
Comment 8 neil@parkwaycc.co.uk 2005-07-08 11:53:58 PDT
(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 neil@parkwaycc.co.uk 2005-07-08 11:55:16 PDT
Created attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE
Comment 10 Boris Zbarsky [:bz] 2005-07-08 12:00:06 PDT
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) 2005-07-10 10:14:15 PDT
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) 2005-07-10 10:25:23 PDT
Comment on attachment 188675 [details] [diff] [review]
Trunk patch

I assume this patch is obsolete now..
Comment 13 neil@parkwaycc.co.uk 2005-07-10 14:13:59 PDT
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 neil@parkwaycc.co.uk 2005-07-10 14:14:37 PDT
Comment on attachment 188699 [details] [diff] [review]
Use STATUS_SIZE_AVAILABLE

Transferring bz's sr.
Comment 15 Daniel Veditz [:dveditz] 2005-07-10 23:20:18 PDT
(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 neil@parkwaycc.co.uk 2005-07-13 12:46:03 PDT
Fix checked in to the trunk, but I don't know what to do about the branches.
Comment 17 Daniel Veditz [:dveditz] 2005-09-12 10:32:53 PDT
Caused regression bug 301329
Comment 18 Tim Riley [:timr] 2006-02-06 11:47:17 PST
Dan will create the right patch for 1.0.8 and others.
Comment 19 neil@parkwaycc.co.uk 2006-02-07 17:19:35 PST
(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] 2006-02-08 19:23:58 PST
Taking, hard to triage bugs for update releases when they're assigned to nobody.
Comment 21 Daniel Veditz [:dveditz] 2006-02-09 04:55:55 PST
Created attachment 211253 [details] [diff] [review]
aviary101 branch backport, incorporating fix for 301329 regression
Comment 22 Tracy Walker [:tracy] 2006-02-14 12:45:05 PST
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] 2006-04-06 13:53:59 PDT
> >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] 2006-04-09 10:52:49 PDT
fixes for regression on regression on the aviary/moz17 branch ultimately wind up in bug 333305

Note You need to log in before you can comment on or make changes to this bug.