236771 - Support Attachment Execution Service (AES) in XP Service Pack 2 (SP2)

Status: RESOLVED DUPLICATE of bug 408153

(Core Graveyard :: File Handling, enhancement)

Description

[Lewis Jardine]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.6) Gecko/20040113

Service Pack 2 will feature a new 'Attachment Execution Service', which checks
with the computer's policy and antivirus software to determine whether or not to
execute an attachment. SP2 is in beta at the moment; it would probably be good
PR if Mozilla used AES (where available) to vet attachments, downloaded files.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Bill Mason]

->File Handling

I think this might be a better component.

Comment 2

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

replacing URI with a usable one

Comment 3

[Bill Mason]

*** Bug 238079 has been marked as a duplicate of this bug. ***

Comment 4

[Hixie (not reading bugmail)]

While this could theoretically be good, its reliance on IE security zones 
is a bit worrying IMHO. We should probably only use this API in 
_addition_ to everything we're doing now, certainly not instead of it.

Comment 5

[ghost16825]

(In reply to comment #4)
> While this could theoretically be good, its reliance on IE security zones 
> is a bit worrying IMHO. We should probably only use this API in 
> _addition_ to everything we're doing now, certainly not instead of it.

Completely agree. I think Mozilla should stay away from any form of security
zones. The best option IMHO would be to have a single user defined setting for
for all exes regardless of location (I disagree with poster of #255351's idea).
Perhaps by default have this setting on the strictest setting (?untrusted (4)?).

Comment 6

[Matthias Versen [:Matti]]

*** Bug 255351 has been marked as a duplicate of this bug. ***

Comment 7

[u49640]

(In reply to comment #5)
> Completely agree. I think Mozilla should stay away from any form of security
> zones. 

Mozilla itself shouldnt rely on IE's zones, but there is IMO nothing wrong in
telling the OS that a downloaded file is a downloaded file and the OS should
notify the user if he/she executes it directly from the Windows Explorer.

it would only improve security as it doesnt affect mozilla itself, but only
interactions with the file after its been downloaded. (after finishing the DL
mozilla should set the Flag (or whatever it is) to notify Windows that its a
Downloaded file)

Comment 8

[u49640]

it probably wont go into 1.1, but maybe we are lucky ;)
asking blocking-aviary1.1 ?

Comment 9

[Mathieu Deaudelin-Lemay (previous account)]

(In reply to comment #5)
> Completely agree. I think Mozilla should stay away from any form of security
> zones. The best option IMHO would be to have a single user defined setting for
> for all exes regardless of location (I disagree with poster of #255351's idea).
> Perhaps by default have this setting on the strictest setting (?untrusted (4)?).

Windows will deny access to any executable file flagged with the strictest
setting (ZoneID=4, untrusted). The best thing to do is to flag ALL downlaoded
files (including documents) with a ZoneID of 3 (Internet Zone). If the file can
be dangerous, Windows will ask the user if he is sure instead of blocking
everything.

Optionally, a setting could be added to the "Options" dialog to leave the user
choose the default level. Something like...

-------------------------------------------------------------
| When I download a file, flag it as...                     |
|  [o] potentially unsafe (recommended)                     |
|  [ ] untrusted (you cannot run untrusted executables)                        
                                |
|                                                           |
| You can also define a list of the sites you do not trust. |
| Downloads from these sites will be flagged as untrusted.  |
|                                                           |
| [Edit my blacklist...]                                    |
-------------------------------------------------------------

Comment 10

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

I thought files were flagged with the source URI, not the zone id.

Comment 11

[Doug Wright]

Please don't implement this without a pref to turn it off - those dialogs drive
me crazy.

Comment 12

[Mathieu Deaudelin-Lemay (previous account)]

A pref in about:config to disable this could be a good idea, however, a normal
user should not see it. it would place some users at risk. at least, this is my
opinion.

Comment 13

[Mike Connor [:mconnor]]

it'd be nice to have, but not going to make Deer Park without someone writing a
patch.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.0 (xpcom)

Adding markAsSaved() method

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.0 (toolkit)

Calling markAsSaved from download manager

Comment 16

[Darin Fisher]

Comment on attachment 245604 [details] [diff] [review]
Patch rv 1.0 (xpcom)

>Index: xpcom/io/nsILocalFileWin.idl

>+    void markAsSaved(in string aSource);

I recommend using AUTF8String instead of "string" as the parameter type.
That is more consistent with the string representation used by Necko w/
URLs.


>Index: xpcom/io/nsLocalFileWin.cpp

>+    hr = pAE->SetLocalPath(mWorkingPath.get());

mWorkingPath may be the path to a shortcut file?  Are you sure
you do not want to use the "target" of the file path instead?
That way, shortcut links are resolved properly, and the setting
is associated with the actual file instead of the shortcut file.


>+    if (aSource) {
>+        hr = pAE->SetSource(NS_ConvertUTF8toUTF16(aSource).get());

This definitely argues in favor of using AUTF8String as the input
parameter instead of "string"


>+    hr = pAE->Save();
>+    if (FAILED(hr)) {
>+        pAE->Release();
>+        return NS_ERROR_FAILURE;
>+    }
>+
>+    pAE->Release();
>+    return NS_OK;

Nit: you could call Release() before checking FAILED(hr), e.g.:

      hr = pAE->Save();
      pAE->Release();
      return FAILED(hr) ? NS_ERROR_FAILURE : NS_OK;

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 245605 [details] [diff] [review]
Patch rv 1.0 (toolkit)

IAttachmentExecute::Save may run malware scanners (for example, Windows Defender), but it was too late to run the scanners.

Comment 18

[Masatoshi Kimura [:emk]]

Changing URL to a usable one.

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 2.0 (xpcom)

> mWorkingPath may be the path to a shortcut file?  Are you sure
> you do not want to use the "target" of the file path instead?
> That way, shortcut links are resolved properly, and the setting
> is associated with the actual file instead of the shortcut file.
This method is intented for marking downloaded files. It's unwise to follow the untrusted link.

Other points are resolved.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 2.0 (caller)

There is no longer toolkit part.

Comment 21

[Masatoshi Kimura [:emk]]

biesi:
Could you review the patch, please?

Comment 22

[Jesse Ruderman]

See also bug 249951, which suggests a solution that could work on both Windows and Mac.  If we like the AES UI on Windows, maybe we'll only use bug 249951's solution on Mac (and older versions of Windows?).

Comment 23

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

Comment on attachment 245692 [details] [diff] [review]
Patch rv 2.0 (caller)

why a pref?

fwiw, I hate how this needs a different interface for each platform

Comment 24

[Masatoshi Kimura [:emk]]

Comment on attachment 245692 [details] [diff] [review]
Patch rv 2.0 (caller)

> why a pref?
Per comment #11. I for one have no objection to removing a pref. 

> fwiw, I hate how this needs a different interface for each platform
nsILocalFile is frozen. Do I have to add a new interface?

Comment 25

[Masatoshi Kimura [:emk]]

Attachment: Make |nsILocalFileOS2::SetFileSource| XP

This patch will reduce platform dependent #ifdefs from XP code.
I picked a function name from nsILocalFileOS2 because it looks more appropriate.
I also removed a pref since I realized IExplorer too doesn't give an option.

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 249243 [details] [diff] [review]
Make |nsILocalFileOS2::SetFileSource| XP

I don't know much about OS/2. Please take a look about OS/2 specific part.

Comment 27

[Mike Kaply [:mkaply]]

Comment on attachment 249243 [details] [diff] [review]
Make |nsILocalFileOS2::SetFileSource| XP

r=mkaply for the OS/2 part. Nice to see OS/2 changes being used for everyone :)

Comment 28

[Masatoshi Kimura [:emk]]

Attachment: Removed redundant check

IAttachmentExecute did the check adequately for us.

Comment 29

[Masatoshi Kimura [:emk]]

Comment on attachment 249243 [details] [diff] [review]
Make |nsILocalFileOS2::SetFileSource| XP

I've forgotten to make old patch obsolete

Comment 30

[Masatoshi Kimura [:emk]]

Attachment: unbitrotted

biesi:
Would you continue to review?

Comment 32

[Masatoshi Kimura [:emk]]

Comment on attachment 262412 [details] [diff] [review]
unbitrotted

Clearing review request because of duplication

Comment 33

[Masatoshi Kimura [:emk]]

Merged into bug 408153.