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

use of default anti-virus scanner when downloading email and executables

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Toolkit
Downloads API
P2
enhancement
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: Brian Staples, Assigned: robarnold)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9alpha8
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: CON-006a)

Attachments

(3 attachments, 10 obsolete attachments)

4.25 KB, text/plain
Details
13.50 KB, application/octet-stream
Details
34.82 KB, patch
robarnold
: review+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9.4+) Gecko/20011005
BuildID:    2001100503

It would greatly improve computer security and stability if Mozilla used the
default anti-virus scanner to scan for viruses when downloading e-mail or other
files from the internet. This goes one extra step to keeping everyone's computer
virus-free and it helps keep some people who open every e-mail attachment they
get out of trouble. 

Reproducible: Always
Steps to Reproduce: n/a

For instance when GetRight is done downloading a file it scans the file for
viruses with the default anti-virus scanner, before you can open the file. Doing
something similar to this would be great.

Comment 1

16 years ago
sounds like a good idea.
Assignee: asa → pchen
Status: UNCONFIRMED → NEW
Component: Browser-General → XP Apps
Ever confirmed: true
QA Contact: doronr → sairuh
Hardware: PC → All
spam: over to File Handling. i have not changed the assigned developer [or the
other fields for that matter], so if anyone realizes that a bug should have a
more appropriate owner, go ahead and change it. :)
Component: XP Apps → File Handling

Comment 3

16 years ago
->future/helpwanted
Keywords: helpwanted
Target Milestone: --- → Future

Updated

16 years ago
Blocks: 75364

Comment 4

16 years ago
->default owner
Assignee: pchen → law
Target Milestone: Future → ---

Comment 5

16 years ago
restoring target milestone
Target Milestone: --- → Future

Comment 6

15 years ago
*** Bug 163357 has been marked as a duplicate of this bug. ***

Comment 7

15 years ago
Bug 163357 is a dupe, but wants to have a manual 'scan for Virus' button in the
download manager.

Comment 8

15 years ago
possible ways a virus can go to come into a system (win32):    1- being downloaded and run  2- being run as attachement  3- being run as script  4- being installed as additional software (XPI -> what is only a special case  of 3)    the case 1 is the most frequent one, 2 is second and 3 and 4 are not that  frequent in non-MS-programms    as i mentioned in bug 163357 - there should be a way to scan incomming  downloads easily, and the easiest way is to add a button to  Download-Manager/Download-Properties to run the default virus-scan - i  suggested that in the Preferences there should be the option to specify them  either by specifying the widely-spread ones (McAffee($), AntiVir(free),...) or  giving a commandline for the unknown ones --- and this solution maybe done in a soon TargetMilestone, what is sooner than 'Future' :-) 
QA Contact: sairuh → petersen

Comment 9

14 years ago
*** Bug 221377 has been marked as a duplicate of this bug. ***
*** Bug 241664 has been marked as a duplicate of this bug. ***

Comment 11

13 years ago
*** Bug 241756 has been marked as a duplicate of this bug. ***
*** Bug 271881 has been marked as a duplicate of this bug. ***

Comment 13

13 years ago
especially do this on encrypted mails (see RFE Bug 263761)

Comment 14

13 years ago
are there any broadly used and standardized APIs beyond Microsoft's MAPI, VS
API, ESE API (see 
"Extensible Storage Engine API" or the Exchange virus-scanning API 2.0.
http://support.microsoft.com/default.aspx/kb/328841/EN/? and
http://www.trendmicro.com/en/products/email/smex/evaluate/vsapi.htm)?
How far is ICAP as per http://www.faqs.org/rfcs/rfc3507.html and
http://www.i-cap.org/home.html
(see http://issues.apache.org/bugzilla/show_bug.cgi?id=27193 for server-side
discussion)

Comment 15

13 years ago
*** Bug 280196 has been marked as a duplicate of this bug. ***

Comment 16

13 years ago
There's an extension that does this : http://downloadstatusbar.mozdev.org/downscan/
*** Bug 290935 has been marked as a duplicate of this bug. ***

Comment 18

12 years ago
Everyone seems to agree with this, So do I. I think it should be in the most obvious place <Options -> Advanced -> Security, and just let you enter your AV scan filename/path/varibles for a silent scan.>

And might furthermore ask the user on install if he wants firefox to scan for AV software and auto configure it.

up until now, FireFox merely _Warns_the_user_ for downloading from secure sites... It is intolerant in todays world to try make a secure browser without scannind downloaded files, IMHO :)

Comment 19

12 years ago
*** Bug 327081 has been marked as a duplicate of this bug. ***
This is on the Firefox 3 PRD; adding appropriate dependencies as well as the PRD line item number into the status whiteboard.
Assignee: law → nobody
Blocks: 372972
No longer blocks: 75364
Whiteboard: CON-006b
IE7 uses IOfficeAntiVirus Interface (http://msdn2.microsoft.com/en-us/library/ms537369.aspx) as indicated here:
http://windowsvistablog.com/blogs/windowsvista/archive/2006/04/21/426003.aspx

This should work for windows at least - not sure if this is much use on other platforms (the bug request that is) or even if api's exist for them.

Updated

10 years ago
Flags: blocking1.9?
OK, so it's not quite as easy as I originally thought.  There is not a stock implementation of this in Windows.  Windows defender includes an implementation, and virus scanners might (I still need to research this).  So, I guess we need to see if this is interface is defined at runtime, and go from there.  So much for the easy way out :/

Updated

10 years ago
Assignee: nobody → sdwilsh
Keywords: helpwanted
Target Milestone: Future → mozilla1.9beta1

Comment 23

10 years ago
Hey Shawn, where are we at on this? Looks like we could add a new rich list entry type with a "Virus Scanning..." message, launch this off in a seperate thread and update the list once that's complete? 
Jim, that pretty much the idea I was thinking of.  I, however, have little to no time to work on this (not to mention that I have no experience using windows API's or MS COM) before M7 or even M8.

Updated

10 years ago
Assignee: sdwilsh → robarnold
Component: File Handling → Download Manager
Flags: blocking1.9?
Product: Core → Firefox
Target Milestone: mozilla1.9beta1 → Firefox 3 M8

Updated

10 years ago
QA Contact: chrispetersen → download.manager
(Assignee)

Comment 25

10 years ago
Created attachment 274502 [details] [diff] [review]
Proposed solution
Attachment #274502 - Flags: review?(sdwilsh)
(Assignee)

Comment 26

10 years ago
Created attachment 274514 [details] [diff] [review]
Full backend patch...thanks CVS
Attachment #274502 - Attachment is obsolete: true
Attachment #274502 - Flags: review?(sdwilsh)
(Assignee)

Updated

10 years ago
Attachment #274514 - Attachment description: Full patch...thanks CVS → Full backend patch...thanks CVS
Attachment #274514 - Flags: review?(sdwilsh)
Comment on attachment 274514 [details] [diff] [review]
Full backend patch...thanks CVS

>Index: toolkit/components/downloads/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v
>retrieving revision 1.13
>diff -u -8 -p -r1.13 Makefile.in
>--- toolkit/components/downloads/src/Makefile.in	27 Jun 2007 16:52:15 -0000	1.13
>+++ toolkit/components/downloads/src/Makefile.in	30 Jul 2007 21:28:04 -0000
>@@ -60,18 +60,23 @@ REQUIRES  = xpcom \
>             intl \
>             windowwatcher \
>             webbrowserpersist \
>             appshell \
>             dom \
>             embed_base \
>             alerts \
>             storage \
>+            xulapp \
>             $(NULL)
> 
> CPPSRCS   = \
>     nsDownloadManager.cpp \
>     $(NULL)
> 
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    "UPDATE moz_downloads "
>+    "SET name = ?1, source = ?2, target = ?3, startTime = ?4, endTime = ?5,"
>+    "state = ?6 "
>+    "WHERE id = ?7"), getter_AddRefs(mUpdateDownloadStatement));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
Just to clarify to other reviewers - I had him move this up here because it needs to be above the observer calls.  Shame on me for not catching it when I reviewed that patch...

>+    case nsIDownloadManager::DOWNLOAD_FINISHED:
>+    {
>+      // Master pref to control this function. 
>+      PRBool showTaskbarAlert = PR_TRUE;
>+      if (pref)
>+        pref->GetBoolPref(PREF_BDM_SHOWALERTONCOMPLETE, &showTaskbarAlert);
>+
>+      if (showTaskbarAlert) {
>+        PRInt32 alertInterval = -1;
>+        pref->GetIntPref(PREF_BDM_SHOWALERTINTERVAL, &alertInterval);
you need to check if pref exists first!

>+
>+        PRInt64 alertIntervalUSec = alertInterval * PR_USEC_PER_MSEC;
>+        PRInt64 goat = PR_Now() - mStartTime;
>+        showTaskbarAlert = goat > alertIntervalUSec;
>+       
>+        PRInt32 size = mDownloadManager->mCurrentDownloads.Count();
The problem with this of course is that downloads that are being scanned are not in this array.  Perhaps we should have an alert for each completed download now?  Talk to mconnor/beltzner about this please.

>   // We need to update mDownloadState before updating the dialog, because
>   // that will close and call CancelDownload if it was the last open window.
>-  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
>       (void)mDownloadManager->FinishDownload(this,
>+#ifdef XP_WIN
>+                                             nsIDownloadManager::DOWNLOAD_SCANNING,
>+#else
>                                              nsIDownloadManager::DOWNLOAD_FINISHED,
>+#endif
>                                              "dl-done");
This isn't right.  dl-done means we are done with the download.  Perhaps another stage?  That also means you will have to call FinishDownload when you change it to DOWNLOAD_FINISHED yourself.

>Index: toolkit/components/downloads/src/nsDownloadScanner.cpp
>+ * The Original Code is mozilla.org code.
No, it's download manager code
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
really?!?!

>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
you wrote this in 1998? :p

>+nsresult nsDownloadScanner::Init()
return type, newline, function name please

>+  hr = CoCreateInstance(CLSID_StdComponentCategoriesMgr, NULL, CLSCTX_INPROC, IID_ICatInformation, getter_AddRefs(catInfo));
nit: 80 chars line wrapping

>+  if (hr != S_OK)
>+  {
nit: ew - { on same line as if please

>+    //printf("Could not create category information class\n");
don't comment this out - use NS_WARNING

>+  mDownload->AddRef();
Let's try NS_ADDREF(mDownload) instead please.
Also, why don't you just use an nsCOMPtr as the member variable?

>+  nsCOMPtr<nsIPrefBranch> prefBranch;
>+  rv = prefService->GetBranch("browser.download.antivirus.", getter_AddRefs(prefBranch));
Please make this a constant

>+  if (prefBranch)
>+    rv = prefBranch->GetBoolPref("donotclean", &mIsReadOnlyRequest);
Constant

>+
>+  nsCOMPtr<nsILocalFile> file;
>+  mDownload->GetTargetFile(getter_AddRefs(file));
You should check the return value of this - it can fail.

>+  rv = file->GetPath(mPath);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIXULAppInfo> appinfo =
>+    do_GetService(XULAPPINFO_SERVICE_CONTRACTID);
Please use the two parameter version and check the result

>+  nsCAutoString name;
>+  appinfo->GetName(name);
if you don't care about the return variable please cast to void.

>+  rv = uri->GetSpec(origin);
>+  NS_ENSURE_SUCCESS(rv, rv);
I don't think we generally check this in DM code actually

>+  CopyUTF8toUTF16(origin, mOrigin);
>+
>+  PRBool isHttp(PR_FALSE), isFtp(PR_FALSE), isHttps(PR_FALSE);
>+  nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(uri);
>+  rv = innerURI->SchemeIs("http", &isHttp);
>+  rv = innerURI->SchemeIs("ftp", &isFtp);
>+  rv = innerURI->SchemeIs("https", &isHttps);
>+  mIsHttpDownload = isHttp || isFtp || isHttps;
You assign to rv, but never check it...

>+void nsDownloadScanner::Scan::Abort()
>+{
>+  mDownload->Release();
>+
>+  // We can only do this internally
>+  this->AddRef();
>+  this->Release();
Why, exactly, are you doing this?

>+  PRInt16 downloadState = 0;
We have a typedef of DownloadState for this....

>+  switch(mStatus)
>+  {
>+  default:
>+  case AVSCAN_FAILED:
>+  case AVSCAN_GOOD:
>+  case AVSCAN_UGLY:
>+    downloadState = nsIDownloadManager::DOWNLOAD_FINISHED;
>+    break;
>+  case AVSCAN_BAD:
>+    downloadState = nsIDownloadManager::DOWNLOAD_BLOCKED;
>+    break;
>+  }
nit: could you put default at the end of it's list, along with that whole block being below the AVSCAN_BAD bits?
nit: brace after parens, space after switch, case/default one tab in

>+  mDownload->Release();
NS_RELEASE
Attachment #274514 - Flags: review?(sdwilsh) → review-

Comment 28

10 years ago
A couple notes from doing some testing - 

User friendly names are hard to come by. You can retrieve the user type class via OleRegGetUserType, however, you'll get stuff like this:

Vista/Defender:
Windows Defender IOfficeAntiVirus implementation

XP/Defender:
IOfficeAntiVirus implementation

XP/Norton:
Symantec Norton AntiVirus OfficeAntiVirus Class

Since under most circumstances users are only going to have one or two of these installed, rather than go through the trouble of implementing a UI for choosing one, (and warning the user after an upgrade that they need to choose one or risk using an out of date scanner) I'd suggest we consider running files through all available IOfficeAntiVirus scan interfaces and add a simple check box in downloads that enables/disables the behavior. 

Overall though, I'm not convinced we need to implement this for every download or even attachments. Files downloaded by Firefox aren't exempt from virus scanning under say Norton. Was there a specific security related reason for implementing this? As far as attachments go, Norton supports attachment scanning, although I'm not sure if Thunderbird is safe. My guess though is that they do this through the proxy, so Thunderbird would be included under their security umbrella.

Also based on some testing, Norton and Defender update through Windows Update, so the risk of being out of date is low with the bigger vendors.
(In reply to comment #28)
> Overall though, I'm not convinced we need to implement this for every download
> or even attachments. Files downloaded by Firefox aren't exempt from virus
> scanning under say Norton. Was there a specific security related reason for
> implementing this? As far as attachments go, Norton supports attachment
> scanning, although I'm not sure if Thunderbird is safe. My guess though is that
> they do this through the proxy, so Thunderbird would be included under their
> security umbrella.
It's for feature parity with IE7.  They scan every download as well.
(Assignee)

Comment 30

10 years ago
Created attachment 275017 [details] [diff] [review]
Rough draft

> User friendly names are hard to come by

Ok, I guess we can't show that in the ui.

> I'd suggest we consider running files through all available IOfficeAntiVirus
> scan interfaces and add a simple check box in downloads that enables/disables
> the behavior

I looked into checking on the status of the antivirus software and it seems to be fairly involved (undocumented WMI and registry stuff). Hopefully the virus scans won't take too long (Windows Defender is quick at least).

Thunderbird is not necessarily safe either; imap/pop connections over ssl cannot be run through the proxy I think (it would be like a man-in-the-middle scenario if they could).

Also, we cannot rely on anti-virus updates to be pushed through windows update; my XP laptop has Norton AV 2005 and it does not get its updates through windows update (I have never seen them there).
Attachment #274514 - Attachment is obsolete: true

Comment 31

10 years ago
Scans don't seem to take long at all. Using a scanning test app with Norton and Defender installed, the first run experienced some "startup time" for Norton but after that it flew through a 2MB file in about a second. This was on a VMWare image as well. An 82 MB installer finished in about 6 seconds. 

Updated

10 years ago
Flags: blocking-firefox3?
Priority: -- → P2
Comment on attachment 275017 [details] [diff] [review]
Rough draft

>Index: toolkit/components/downloads/public/nsIDownloadManager.idl
you need to rev the uuid

> nsresult
>-nsDownloadManager::FinishDownload(nsDownload *aDownload, DownloadState aState,
>-                                  const char *aTopic) {
>+nsDownloadManager::FinishDownload(nsDownload *aDownload)
>+{
would CompleteDownload be better name here now?

>   // We don't want to lose access to the download's member variables
>   nsRefPtr<nsDownload> kungFuDeathGrip = aDownload;
We don't need access to our members anymore after we release this, so we don't need this bit


>-      NS_WARNING("Could not get downlaod's database schema version!");
>+      NS_WARNING("Could not get download's database schema version!");
I must really like typos...


>+nsresult
>+nsDownloadManager::SendEvent(nsDownload *aDownload, const char *aTopic)
>+{
>+  return mObserverService->NotifyObservers(aDownload, aTopic, nsnull);
>+}
We don't ever care about the result of this, so why not make it a void function?

> nsDownload::SetState(DownloadState aState)
now, we'll need kungFuDeathGrip in here in case we call FinishDownload because that could remove our last reference and we'll lose access to our member variables

>+  switch (aState) {
>+    case nsIDownloadManager::DOWNLOAD_CANCELED:
>+      (void)mDownloadManager->FinishDownload(this);
hmm, FinishDownload should probably be a void function to now that I think about it...

>+#ifdef XP_WIN
>+    case nsIDownloadManager::DOWNLOAD_SCANNING:
>+    {
>+      nsresult rv = mDownloadManager->mScanner->ScanDownload(this);
>+      // If we failed, then fall through to 'download finished'
>+      if(!NS_FAILED(rv))
>+        break;
>+      mDownloadState = aState = nsIDownloadManager::DOWNLOAD_FINISHED;
We don't actually care about aState at this point anymore

>+        pref->GetIntPref(PREF_BDM_SHOWALERTINTERVAL, &alertInterval);
I know you just copied this code, but can you please put |if (pref)| on the line before it?  We can crash here...
 
>+  default:
>+    break;
you don't need a default here then

>+  switch (aState)
>+  {
use mDownloadState, and { after ( please

>+    case nsIDownloadManager::DOWNLOAD_DOWNLOADING:
>+      (void)mDownloadManager->SendEvent(this, "dl-start");
>+      break;
>+    case nsIDownloadManager::DOWNLOAD_FAILED:
>+      (void)mDownloadManager->SendEvent(this, "dl-failed");
>+      break;
>+    case nsIDownloadManager::DOWNLOAD_CANCELED:
>+      (void)mDownloadManager->SendEvent(this, "oncancel");
>+      break;
>+    case nsIDownloadManager::DOWNLOAD_PAUSED:
>+      (void)mDownloadManager->SendEvent(this, "dl-paused");
>+      break;
>+    case nsIDownloadManager::DOWNLOAD_SCANNING:
>+      (void)mDownloadManager->SendEvent(this, "dl-scanning");
>+      break;
>+    case nsIDownloadManager::DOWNLOAD_FINISHED:
>+      (void)mDownloadManager->SendEvent(this, "dl-done");
>+      break;
>+    case nsIDownloadManager::DOWNLOAD_BLOCKED:
>+      (void)mDownloadManager->SendEvent(this, "dl-blocked");
>+      break;
I don't think we need to add observer notifications for dl-paused (there is no corresponding dl-resumed).  Also, oncancel should be dispatched to the progress listener, not the observer service which is what SendEvent does.

>+    default:
>+      break;
don't need this again

>+      (void)SetState(
> #ifdef XP_WIN
>+             nsIDownloadManager::DOWNLOAD_SCANNING
>+#else
>+             nsIDownloadManager::DOWNLOAD_FINISHED
>+          );
hrm, for readability's sake, can you just do (void)SetState(state);

>+    }
>   }
> 
>   mDownloadManager->NotifyListenersOnStateChange(aWebProgress, aRequest,
>                                                  aStateFlags, aStatus, this);
> 
>   return UpdateDB();
We actually don't need this call anymore, since SetState calls it, and we don't do anything in there anymore.

>   /**
>-   * Removes download from "current downloads," updates download state, and
>-   * notifies observers.
>+   * Removes download from "current downloads". This does not notify any
>+   * observers
Do not need to comment about what it doesn't do

>Index: toolkit/components/downloads/src/nsDownloadScanner.cpp
>===================================================================
>RCS file: toolkit/components/downloads/src/nsDownloadScanner.cpp
>diff -N toolkit/components/downloads/src/nsDownloadScanner.cpp
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/components/downloads/src/nsDownloadScanner.cpp	2 Aug 2007 21:37:12 -0000
>@@ -0,0 +1,304 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: se cin sw=2 ts=2 et : */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.com code.
eh, let's go with download manager code

and usually there is the lines like this:
  * The Initial Developer of the Original Code is
  * Mozilla Corporation.

>+ *
>+ * Contributor(s):
>+ *   Rob Arnold <robarnold@mozilla.com> (Original Author)
>+#include "nsIXULAppInfo.h"
>+#include "nsXULAppAPI.h"
do you need both of those?

>+   There are 3 possible outcomes of the virus scan:
>+      good => the file is clean
>+      bad => the file has a virus
>+      ugly => the file had a virus, but it was cleaned
either include or use the constant names that you use here please

>+   Future enhancements:
nit: Possible future enhancements

>+    * 
nit: extra * for bullet

>+nsresult nsDownloadScanner::Scan::Start()
you didn't fix this nit! You make me sad :(
return type\nmethod name
You actually do this in several places still

>+{
>+  nsresult rv = NS_OK;
please declare at first use - this isn't C ;)

>+  nsCOMPtr<nsIPrefBranch> pref =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  if (pref)
>+    rv = pref->GetBoolPref(PREF_BDA_DONTCLEAN, &mIsReadOnlyRequest);
you don't do anything with rv, so don't assign to it

>+  mDownload->SetState(downloadState);
this has a return type - you dont' care what it is though, so please cast to void

>+  if (hr != S_OK) {
>+    NS_WARNING("Could not instantiate antivirus scanner");
>+    mStatus = AVSCAN_FAILED;
>+  }
>+  else
>+  {
nit: } else {
(In reply to comment #32)
> (From update of attachment 275017 [details] [diff] [review])
> >Index: toolkit/components/downloads/public/nsIDownloadManager.idl
> you need to rev the uuid

Actually, you don't if you're only adding or removing constants on the interface.  If you change the values you should, because code using different objects implementing the interface wouldn't know which value to use with which object.  If you add, old implementations can't correctly have relied on its value before, and if you remove, new code won't use it and the old implementation will deal as though it hadn't been used (which should have been handled regardless whether the constant's present or not).  See also bug 214672 comment 88.
(Assignee)

Comment 34

10 years ago
Created attachment 275165 [details] [diff] [review]
Proposed solution
Attachment #275017 - Attachment is obsolete: true
Attachment #275165 - Flags: review?(sdwilsh)
(Assignee)

Comment 35

10 years ago
Created attachment 275171 [details] [diff] [review]
Appeases sdwilsh's minor gripes...
Attachment #275165 - Attachment is obsolete: true
Attachment #275165 - Flags: review?(sdwilsh)
(Assignee)

Comment 36

10 years ago
Created attachment 275172 [details] [diff] [review]
Fixes some minor style issues
Attachment #275171 - Attachment is obsolete: true
Comment on attachment 275172 [details] [diff] [review]
Fixes some minor style issues

r=sdwilsh for DM stuff
Attachment #275172 - Flags: review+
Comment on attachment 275172 [details] [diff] [review]
Fixes some minor style issues

Jim, can you take a look at the windows stuff here?
Attachment #275172 - Flags: review?(jmathies)

Comment 39

10 years ago
> ULONG nReceived; 
> clsidEnumerator->Next(1, &mScannerCLSID, &nReceived); 
> if (nReceived == 0) { 
>  NS_WARNING("No antivirus seems to be installed\n"); 

I might be missing a mozilla style loop in here someplace :) but afaict, your limiting the scan to Defender. Shouldn't we be saving clsidEnumerator and looping back around to scan using all scanners? We would keep calling Next until it returns something other than S_OK, or nReceived == 0.

> hr = CoCreateInstance(mDLScanner->mScannerCLSID, NULL, CLSCTX_ALL, 

Not sure why we would use CLSCTX_ALL, I guess supporting remote instances is ok with all our threading and the like?

Other than that, the com related stuff looks good to me. 

> #include <msoav.h> 

Aren't we going to run into trouble compiling this on sdk's less than 6.0? I don't use that sdk by default and had to manually define the interface and structures. We do the same thing with the newer Vista calls in shell services.  
(Assignee)

Comment 40

10 years ago
(In reply to comment #39)
> > ULONG nReceived; 
> > clsidEnumerator->Next(1, &mScannerCLSID, &nReceived); 
> > if (nReceived == 0) { 
> >  NS_WARNING("No antivirus seems to be installed\n"); 
> 
> I might be missing a mozilla style loop in here someplace :) but afaict, your
> limiting the scan to Defender. Shouldn't we be saving clsidEnumerator and
> looping back around to scan using all scanners? We would keep calling Next
> until it returns something other than S_OK, or nReceived == 0.

I was going to wait on implementing the loop until beta1 when we can get beta testers in to get this tested.

> 
> > hr = CoCreateInstance(mDLScanner->mScannerCLSID, NULL, CLSCTX_ALL, 
> 
> Not sure why we would use CLSCTX_ALL, I guess supporting remote instances is ok
> with all our threading and the like?

I don't see why not; I could change it to inproc, but all seemed more flexible. I'm not quite sure how an out-of-proc server works for sharing data.

> > #include <msoav.h> 
> 
> Aren't we going to run into trouble compiling this on sdk's less than 6.0? I
> don't use that sdk by default and had to manually define the interface and
> structures. We do the same thing with the newer Vista calls in shell services.  
> 

VC8 ships with the header and the interface I'm using should be available since Win95/NT 4/IE 5. That's very odd that it's not defined. Are you using VC7.1?
(Assignee)

Updated

10 years ago
Attachment #275172 - Flags: review?(gavin.sharp)

Comment 41

10 years ago
>VC8 ships with the header and the interface I'm using should be available since
>Win95/NT 4/IE 5. That's very odd that it's not defined. Are you using VC7.1?

Nope your right, just checked, I was mistaken.
P2 on the PRD, wanted, not blocking
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: CON-006b → CON-006a [wanted-firefox3]

Updated

10 years ago
Attachment #275172 - Flags: review?(jmathies) → review+
(Assignee)

Comment 43

10 years ago
Created attachment 277144 [details] [diff] [review]
unbitrotted patch
Attachment #275172 - Attachment is obsolete: true
Attachment #275172 - Flags: review?(gavin.sharp)
(Assignee)

Comment 44

10 years ago
Created attachment 277417 [details] [diff] [review]
unbitrotted patch (round 2)

Hopefully the last time I have to unbitrot. Also, cvs didn't eat the scanner part this time.
Attachment #277144 - Attachment is obsolete: true
Attachment #277417 - Flags: review?(gavin.sharp)
Comment on attachment 277417 [details] [diff] [review]
unbitrotted patch (round 2)

>   // This has to be done in this exact order to not mess up our invariants
>   // 1) when the state changed listener is dispatched, it must no longer be
>   //    an active download.
>   // 2) when the observer is dispatched, the same conditions for 1 must be
>   //    true as well as the state being up to date.
>   (void)mCurrentDownloads.RemoveObject(aDownload);
The comment above this isn't so accurate anymore...

I also don't see it moved to anywhere else where you do this...

>+#ifdef XP_WIN
>+  mScanner = new nsDownloadScanner();
>+  if (!mScanner)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  rv = mScanner->Init();
>+  if (NS_FAILED(rv)) {
>+    delete mScanner;
>+    return rv;
>+  }
>+#endif
If we fail to init, do we really want to not have a download manager, or should we just not scan?

>   if (NS_FAILED(aStatus)) {
>     // We don't want to lose access to our member variables
>     nsRefPtr<nsDownload> kungFuDeathGrip = this;
> 
>-    (void)mDownloadManager->FinishDownload(this,
>-                                           nsIDownloadManager::DOWNLOAD_FAILED,
>-                                           "dl-failed");
>+    SetState(nsIDownloadManager::DOWNLOAD_FAILED);
cast to void or check result
(Assignee)

Comment 46

10 years ago
Created attachment 277552 [details]
Simple scanner program - Source

This will be useful for testing the return values of different AV providers.
(Assignee)

Comment 47

10 years ago
Created attachment 277554 [details]
Simple scanner program - Binary

Compiled w/vc8 sp1 - release
(Assignee)

Comment 48

10 years ago
Created attachment 277567 [details] [diff] [review]
unbitrotted patch (round 3)

Ok, I really don't want to unbitrot this again
Attachment #277417 - Attachment is obsolete: true
Attachment #277417 - Flags: review?(gavin.sharp)
Comment on attachment 277567 [details] [diff] [review]
unbitrotted patch (round 3)

>+  // This has to be done in this exact order to not mess up our invariants
>+  // 1) when the state changed listener is dispatched, it must no longer be
>+  //    an active download.
>+  // 2) when the observer is dispatched, the same conditions for 1 must be
>+  //    true as well as the state being up to date.
OK, so 1 isn't quite accurate where it is anymore...

When the state changed listener is dispatched, queries to the database and the download manager api should reflect what the nsIDownload object would return.  So, if a download is done (finished, canceled, etc.), it should first be removed from the current downloads.  We will also have to update the database *before* notifying listeners.  At this point, you can safely dispatch to the observers as well.

^^ That comment should be sufficient.

Our unit tests should cover this pretty well too, so make sure you run those.
(Assignee)

Comment 50

10 years ago
Created attachment 277736 [details] [diff] [review]
Latest working revision
Attachment #277567 - Attachment is obsolete: true
Attachment #277736 - Flags: review?(gavin.sharp)
Comment on attachment 277736 [details] [diff] [review]
Latest working revision

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

>+    case nsIDownloadManager::DOWNLOAD_SCANNING:
>+    {
>+      nsresult rv = mDownloadManager->mScanner ? mDownloadManager->mScanner->ScanDownload(this) : NS_ERROR_NOT_INITIALIZED;
>+      // If we failed, then fall through to 'download finished'
>+      if (!NS_FAILED(rv))

NS_SUCCEEDED

>Index: toolkit/components/downloads/src/nsDownloadScanner.cpp

>+nsDownloadScanner::Scan::Run()

>+  DownloadState downloadState = 0;
>+  switch (mStatus) {
>+    case AVSCAN_BAD:
>+      downloadState = nsIDownloadManager::DOWNLOAD_BLOCKED

>+  (void)mDownload->SetState(downloadState);

This produces the correct UI, but doesn't remove the download. I think you need to add DOWNLOAD_BLOCKED to the switch in SetState() and have it behave the same as DOWNLOAD_CANCELED (call CompleteDownload()).

>+nsDownloadScanner::FindCLSID()

>+  clsidEnumerator->Next(1, &mScannerCLSID, &nReceived);
>+  if (nReceived == 0) {
>+    NS_WARNING("No antivirus seems to be installed\n");

This doesn't deserve a warning, just remove this (or make it a comment).
 
r=me with those fixed. Can you file a bug about determining whether we should scan with all scanners, or just the first/last, etc.? Also need to file a bug on the UI to make it more generic, it currently says "blocked by Parental Controls" if your virus scanner causes it to be blocked.
Attachment #277736 - Flags: review?(gavin.sharp) → review+
Created attachment 277783 [details] [diff] [review]
updated patch, review comments addressed

I took the liberty of updating the patch to apply cleanly to current trunk, and addressing my review comments. Can you look over the changes and make sure you're OK with them? The change to SetState is the only real significant one.
Attachment #277736 - Attachment is obsolete: true
Attachment #277783 - Flags: review?(robarnold)
(Assignee)

Comment 53

10 years ago
Comment on attachment 277783 [details] [diff] [review]
updated patch, review comments addressed

Those changes seem fine by me
Attachment #277783 - Flags: review?(robarnold) → review+

Comment 54

10 years ago
>+      downloadState = nsIDownloadManager::DOWNLOAD_BLOCKED

If were going to use this, we should probably change the string displayed in the download manager. "Blocked by Parental Controls" Doesn't really apply here. Or add a new state, like DOWNLOAD_VIRUSDETECTED and create a new string for this state. 
(Assignee)

Updated

10 years ago
Blocks: 393301
mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl 	1.19
mozilla/toolkit/components/downloads/src/Makefile.in 	1.14
mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 	1.106
mozilla/toolkit/components/downloads/src/nsDownloadManager.h 	1.35
mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp 	1.1
mozilla/toolkit/components/downloads/src/nsDownloadScanner.h 	1.1
Status: NEW → RESOLVED
Last Resolved: 10 years ago
OS: All → Windows XP
Hardware: All → PC
Resolution: --- → FIXED
No longer blocks: 393301
Depends on: 393301
(Assignee)

Updated

10 years ago
Blocks: 393303
(Assignee)

Updated

10 years ago
Blocks: 393305

Updated

10 years ago
Blocks: 393556

Updated

10 years ago
No longer blocks: 393556

Updated

10 years ago
Blocks: 393556

Comment 56

10 years ago
Note to all:
People download spyware via download manager more often than viruses. Viruses pray on browser exploits to get in, potentially bypassing download manager. The only time I can think of where users are willingly downloading files containing viruses is for warez.

Whilst I welcome this bug and think it's a great safety net, we also need to be scanning downloads with any available anti-spyware as spyware is 10x the problem viruses are.

I'm not aware of any API that exists on Windows for scanning for spyware. Even on Vista, Windows Defender may be disabled by the presence of Norton / McAffee. Firefox could do two things:

a) Look for the installation of common, well known AS programs, and configure automatically to use one

b) And provide UI in prefs to select the scanning executable, as a fallback

Comment 57

10 years ago
> Windows Defender may be disabled by the presence of Norton / McAffee.

In my testing with Norton, this was not the case. On an XP image, with Norton installed, Defender (the primary purpose of which is to scan for spyware) was still available as one of the scanners listed through the interface.

Updated

10 years ago
Blocks: 393792

Updated

10 years ago
Depends on: 394272

Updated

10 years ago
Blocks: 395092
No longer blocks: 393303, 393305
Depends on: 393303, 393305
No longer blocks: 393556, 393792, 395092
Depends on: 393556, 393792, 395092

Updated

10 years ago
No longer blocks: 372972

Updated

10 years ago
Depends on: 402985
I'm calling this fixed; collectively, we've done a plethora of testing against the vendors, here: https://bugzilla.mozilla.org/show_bug.cgi?id=396553#c16

In addition, bugs against its continual-scanning state have been filed, and hopefully fixed (see bug 401582; if that's truly not fixed, please do reopen and comment with your findings).

Finally, I filed bug 408153 a little while ago, so that we could investigate implementing an API that would work with those vendors' products--usually the 2008 editions--that don't implement IOfficeAntiVirus.
Status: RESOLVED → VERIFIED
Flags: wanted-firefox3+
Whiteboard: CON-006a [wanted-firefox3] → CON-006a

Updated

10 years ago
Depends on: 412565

Updated

10 years ago
Depends on: 408153

Updated

10 years ago
Depends on: 412204

Updated

10 years ago
Depends on: 424608

Comment 59

9 years ago
How does everyone feel about splitting the scanner out for 3.1 as a service where everyone can get access to it? Similar to the service interface for parental controls where the interface is public, but fails if a native impl isn't available. I think there's a bug filed somplace for something like this but I currently can find it.
(In reply to comment #59)
> How does everyone feel about splitting the scanner out for 3.1 as a service
> where everyone can get access to it? Similar to the service interface for
> parental controls where the interface is public, but fails if a native impl
> isn't available. I think there's a bug filed somplace for something like this
> but I currently can find it.
We actually need to do that so exthandler can use it.  Right now if you click "Open" or "Open With..." on a download, it opens it before the scan is complete.

Updated

9 years ago
Blocks: 443215
Product: Firefox → Toolkit

Updated

9 years ago
Duplicate of this bug: 247223
You need to log in before you can comment on or make changes to this bug.