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

RESOLVED DUPLICATE of bug 408153

Status

enhancement
RESOLVED DUPLICATE of bug 408153
15 years ago
3 years ago

People

(Reporter: bugzilla, Assigned: emk)

Tracking

Trunk
x86
Windows XP
Bug Flags:
blocking-aviary1.5 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

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

15 years ago
->File Handling

I think this might be a better component.
Assignee: download-manager → file-handling
Component: Download Manager → File Handling
QA Contact: ian

Comment 3

15 years ago
*** Bug 238079 has been marked as a duplicate of this bug. ***
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

15 years ago
(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)?).

Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 255351 has been marked as a duplicate of this bug. ***

Comment 7

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

15 years ago
it probably wont go into 1.1, but maybe we are lucky ;)
asking blocking-aviary1.1 ?
Flags: blocking-aviary1.1?
(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...]                                    |
-------------------------------------------------------------
I thought files were flagged with the source URI, not the zone id.

Comment 11

14 years ago
Please don't implement this without a pref to turn it off - those dialogs drive
me crazy.
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.
it'd be nice to have, but not going to make Deer Park without someone writing a
patch.
Flags: blocking-aviary1.1? → blocking-aviary1.1-
(Assignee)

Comment 14

13 years ago
Posted patch Patch rv 1.0 (xpcom) (obsolete) — Splinter Review
Adding markAsSaved() method
Assignee: file-handling → VYV03354
Status: NEW → ASSIGNED
Attachment #245604 - Flags: superreview?(darin.moz)
Attachment #245604 - Flags: review?(darin.moz)
(Assignee)

Comment 15

13 years ago
Posted patch Patch rv 1.0 (toolkit) (obsolete) — Splinter Review
Calling markAsSaved from download manager
Attachment #245605 - Flags: review?(mconnor)

Comment 16

13 years ago
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;
Attachment #245604 - Flags: superreview?(darin.moz)
Attachment #245604 - Flags: superreview-
Attachment #245604 - Flags: review?(darin.moz)
Attachment #245604 - Flags: review-
(Assignee)

Comment 17

13 years ago
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.
Attachment #245605 - Flags: review?(mconnor) → review-
(Assignee)

Comment 19

13 years ago
Posted patch Patch rv 2.0 (xpcom) (obsolete) — Splinter Review
> 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.
Attachment #245604 - Attachment is obsolete: true
Attachment #245691 - Flags: superreview?(darin.moz)
Attachment #245691 - Flags: review?(darin.moz)
(Assignee)

Comment 20

13 years ago
Posted patch Patch rv 2.0 (caller) (obsolete) — Splinter Review
There is no longer toolkit part.
Attachment #245605 - Attachment is obsolete: true
Attachment #245692 - Flags: review?(cbiesinger)

Updated

13 years ago
Attachment #245691 - Flags: superreview?(darin.moz)
Attachment #245691 - Flags: superreview+
Attachment #245691 - Flags: review?(darin.moz)
Attachment #245691 - Flags: review+
(Assignee)

Comment 21

13 years ago
biesi:
Could you review the patch, please?

Comment 22

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

Comment 24

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

Comment 25

13 years ago
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.
Attachment #245691 - Attachment is obsolete: true
Attachment #245692 - Attachment is obsolete: true
Attachment #249243 - Flags: review?(cbiesinger)
Attachment #245692 - Flags: review?(cbiesinger)
(Assignee)

Comment 26

13 years ago
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.
Attachment #249243 - Flags: review?(mozilla)
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 :)
Attachment #249243 - Flags: review?(mozilla) → review+
(Assignee)

Comment 28

13 years ago
Posted patch Removed redundant check (obsolete) — Splinter Review
IAttachmentExecute did the check adequately for us.
Attachment #249433 - Flags: review?(cbiesinger)
(Assignee)

Comment 29

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

I've forgotten to make old patch obsolete
Attachment #249243 - Attachment is obsolete: true
Attachment #249243 - Flags: review?(cbiesinger)
(Assignee)

Comment 30

12 years ago
Posted patch unbitrottedSplinter Review
biesi:
Would you continue to review?
Attachment #249433 - Attachment is obsolete: true
Attachment #262412 - Flags: review?(cbiesinger)
Attachment #249433 - Flags: review?(cbiesinger)

Updated

11 years ago
Duplicate of this bug: 401983
(Assignee)

Comment 32

11 years ago
Comment on attachment 262412 [details] [diff] [review]
unbitrotted

Clearing review request because of duplication
Attachment #262412 - Flags: review?(cbiesinger)
(Assignee)

Comment 33

11 years ago
Merged into bug 408153.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 408153
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.