This should be pretty straightforward. After the file is finished being transferred but before it is saved to disk (in its final location) or the helper app launched, call the application reputation query. On a bad verdict, change state to DOWNLOAD_DIRTY. Otherwise, proceed. This can be written after bug 837199 is resolved.

Hi Paolo, I'm not sure where to hook in nsIApplicationReputation.queryReputation into this. It looks throwing in DownloadSaver.execute is a possibility, or maybe the check should be in DownloadIntegration.shouldBlockForMalware() sort of like the parental controls. Do I need to worry about DownloadLegacySaver? It also looks like BackgroundFileSaver.setTarget is sometimes used to write directly to the target file. Should this patch remove the target file if the application reputation check fails, or should setTarget be called with a temp file and not moved until the query is done? Thanks, Monica

Not ready for review since it is missing tests, but what do you think about the placement?

Comment on attachment 810874 [details] [diff] [review] Integrate application reputation check with JS download manager ( I chatted with Paolo. Both DownloadCopySaver and DownloadLegacyTransfer are used, so the best place to hook in is in the DownloadObject itself, near line http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#423 before setting succeeded to true and after the saver (whatever implementation it is) finishes. This involves 1) Setting an optional sha256hash field on DownloadSaver 2) Implementing the function to call application reputation similar to shouldBlockParentalConsent in DownloadIntegration.jsm 3) Calling that in the DownloadObject 4) Mocking out the check, similar to http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#385 in the unittest so that common_test_Download.js doesn't know anything about setting up the application reputation service (as in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_app_rep.js) This work won't happen until next week at the earliest since I'm on PTO tomorrow through Tuesday.

Comment on attachment 824826 [details] [diff] [review] Integrate application reputation check with JS download manager ( Review of attachment 824826 [details] [diff] [review]: ----------------------------------------------------------------- Changes: - Add DownloadSaver.getSha256Hash() - Call ApplicationReputation.queryReputation after download is complete - Enable updates for local block and allowlists for application reputation - Disable remote lookups in ApplicationReputation (pending bug 928536), to only use the local lists Slightly old try: https://tbpl.mozilla.org/?tree=Try&rev=e7845a77795c Newer try: https://tbpl.mozilla.org/?tree=Try&rev=243273004748

Comment on attachment 824826 [details] [diff] [review] Integrate application reputation check with JS download manager ( Review of attachment 824826 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +1683,5 @@ > // If the data transfer completed successfully, indicate to the > // background file saver that the operation can finish. If the > // data transfer failed, the saver has been already stopped. > if (Components.isSuccessCode(aStatusCode)) { > + if (partFilePath) { erroneous whitespace change, will fix

Comment on attachment 824826 [details] [diff] [review] Integrate application reputation check with JS download manager ( Latest try was not green.

Comment on attachment 825004 [details] [diff] [review] Integrate application reputation check with JS download manager ( Review of attachment 825004 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadLegacy.js @@ +236,5 @@ > }.bind(this)).then(null, Cu.reportError); > }, > > + setSha256Hash: function (hash) > + { Hi Paolo, I think I need help understanding why this code is never reached. nsExternalHelperAppService calls nsITransfer.setSha256Hash when the download is done, but it must be reaching a different implementation somehow, and I'm not sure which. ::: uriloader/exthandler/nsExternalHelperAppService.cpp @@ +1844,5 @@ > > LOG(("Notifying progress listener")); > > if (NS_SUCCEEDED(aStatus)) { > + LOG(("Setting hash in nsExternalAppHandler")); I see this line in the logs when running test_DownloadLegacy.js.

Comment on attachment 825004 [details] [diff] [review] Integrate application reputation check with JS download manager ( Review of attachment 825004 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Monica Chew [:mmc] (please use needinfo) from comment #10) > > + LOG(("Setting hash in nsExternalAppHandler")); > > I see this line in the logs when running test_DownloadLegacy.js. I have no idea why the function isn't called, the code looks correct. You may try to log the result code returned by the invocation of SetSha256Hash. ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +134,5 @@ > return OnComplete(false, rv); > } > return NS_OK; > +#else > + // Disable remote lookups until bug 928536 is resolved I think you should file a separate bug to re-enable the remote lookup (which might grow more dependencies than bug 928536 alone). This way we don't lose the correct bug number reference in the source. ::: toolkit/components/downloads/test/unit/test_app_rep.js @@ +108,1 @@ > add_test(function test_shouldBlock() { Bug number reference here. @@ +168,1 @@ > add_test(function test_garbage() { Also here. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +429,5 @@ > + // If we failed during the operation, we report the error but use > + // the original one as the failure reason of the download. Note > + // that on Windows we may get an access denied error instead of a > + // no such file error if the file existed before, and was recently > + // deleted. Hm, do we need this special handling here? The file should always exist before we try to delete it here. @@ +1495,5 @@ > let partFilePath = download.target.partFilePath; > let keepPartialData = download.tryToKeepPartialData; > + // Keep a reference to this DownloadCopySaver so we can manipulate it > + // inside other objects below. > + let dcs = this; Good catch! Maybe we could just use an arrow function for onSaveComplete. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +417,5 @@ > + * > + * @return {Promise} > + * @resolves The boolean indicates to block downloads or not. > + */ > + shouldBlockForApplicationReputation: function DI_shouldBlockForApplicationReputation(aDownload, aSaver) { You should remove the aSaver parameter and use "aDownload.saver". @@ +428,5 @@ > + } catch (ex) { > + // Bail if DownloadSaver doesn't have a hash. > + return Promise.resolve(false); > + } > + let p = Promise.defer(); let deferred = ... ::: toolkit/components/jsdownloads/src/DownloadLegacy.js @@ +82,5 @@ > > onStateChange: function DLT_onStateChange(aWebProgress, aRequest, aStateFlags, > aStatus) > { > + let dlt = this; leftover ::: toolkit/components/url-classifier/SafeBrowsing.jsm @@ +149,4 @@ > listManager.disableUpdate(malwareList); > + listManager.disableUpdate(downloadBlockList); > + listManager.disableUpdate(downloadAllowList); > + } I guess you'll need a separate review for these and the preference changes (that look good to me).

Comment on attachment 825535 [details] [diff] [review] Enable updating local lists for application reputation ( Review of attachment 825535 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gian-Carlo, This patch enables updates for the local lists used by application reputation. If the list name is empty, nsIListManager.enableUpdate doesn't do anything. We want it to be empty for everything except firefox. Thanks, Monica

(In reply to :Paolo Amadini from comment #11) > Comment on attachment 825004 [details] [diff] [review] > Integrate application reputation check with JS download manager ( > > Review of attachment 825004 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Monica Chew [:mmc] (please use needinfo) from comment #10) > > > + LOG(("Setting hash in nsExternalAppHandler")); > > > > I see this line in the logs when running test_DownloadLegacy.js. > > I have no idea why the function isn't called, the code looks correct. You > may try to log the result code returned by the invocation of SetSha256Hash. You are right, the set hash is failing in nsExternalAppHandler with NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS. Now, to figure out the details...

Comment on attachment 825535 [details] [diff] [review] Enable updating local lists for application reputation ( Review of attachment 825535 [details] [diff] [review]: ----------------------------------------------------------------- Any specific reason why it's not applicable to for example Firefox for Android? (I can imagine the download manager work you did is only for desktop maybe?) Permission from Google shouldn't be a problem, see bug 794325.

(In reply to Gian-Carlo Pascutto (:gcp) from comment #15) > Comment on attachment 825535 [details] [diff] [review] > Enable updating local lists for application reputation ( > > Review of attachment 825535 [details] [diff] [review]: > ----------------------------------------------------------------- > > Any specific reason why it's not applicable to for example Firefox for > Android? (I can imagine the download manager work you did is only for > desktop maybe?) Permission from Google shouldn't be a problem, see bug > 794325. Hmm, I can't see that bug, but I don't if the download manager work applies to Android.

Comment on attachment 826062 [details] [diff] [review] Integrate application reputation with download manager, disable remote lookups for application reputation ( Review of attachment 826062 [details] [diff] [review]: ----------------------------------------------------------------- Address Paolo's comments. The error I was getting with DownloadLegacySaver was that nsIExternalHelperAppService isn't always called, so the hash is not always available. Plus I shot myself in the foot by putting a throw in the setSha256 in DownloadLegacySaver and expecting it to break the test if it was called in any way, to try to debug the other problem.

Comment on attachment 826062 [details] [diff] [review] Integrate application reputation with download manager, disable remote lookups for application reputation ( mfinkle, does Firefox for Android use enough of the Download manager that it would be able to use this new malware protection, or do we need mobile-specific work?

(In reply to Gian-Carlo Pascutto (:gcp) from comment #19) > mfinkle, does Firefox for Android use enough of the Download manager that it > would be able to use this new malware protection, or do we need > mobile-specific work? Firefox for Android uses the toolkit Download manager (the XPCOM component currently, but see bug 901360) to handle all primary file downloads. We use Java to download favicons and Firefox update APKs.

Comment on attachment 826062 [details] [diff] [review] Integrate application reputation with download manager, disable remote lookups for application reputation ( I am treating this like a "need-info" since I really can't comment on the patch itself.

Yeah sorry that was the idea. Monica, you should adapt your patch accordingly.

Actually, supporting application reputation check properly in nsDownloadManager.cpp would be more effort than switching Firefox for Android to use Downloads.jsm. I'll post a few pointers in the bug. When the patch in this bug is in the tree and the reputation check is needed in Android, it will be sufficient to fix bug 901360 (mostly code in mobile/android/chrome/content/downloads.js) and enable the related preferences.

Comment on attachment 826062 [details] [diff] [review] Integrate application reputation with download manager, disable remote lookups for application reputation ( Review of attachment 826062 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +427,5 @@ > + } catch (e2) { > + Cu.reportError(e2); > + } > + throw new DownloadError( > + { becauseBlockedByApplicationReputation: true }); nit: e2 => ex, align "{" under DownloadError (unless we shorten becauseBlockedByApplicationReputation and it can fit on one line, see below) @@ +1265,5 @@ > + > + /** > + * Indicates the download was blocked because it was malware. > + */ > + becauseBlockedByApplicationReputation: false, So, I wonder whether we should call this more generically "becauseBlockedByMalwareCheck" rather than "becauseBlockedByApplicationReputation", also seeing the comment here. I'll ask for Marco's consultancy on this interface addition as well. @@ +1396,5 @@ > throw new Error("Not implemented."); > }, > + > + /** > + * Returns the sha256 hash of the downloaded file, if it exists. mini-nit: you might consider using "SHA-256" (uppercase with dash) consistently in code comments. @@ +1455,5 @@ > */ > _canceled: false, > > /** > + * Save the SHA256 hash of the downloaded file. Please add information on the format and data type of the value, and in which cases this can stay null. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +415,5 @@ > + * > + * @return {Promise} > + * @resolves The boolean indicates to block downloads or not. > + */ > + shouldBlockForApplicationReputation: function DI_shouldBlockForApplicationReputation(aDownload) { No need for function names anymore (we temporarily keep names in functions that have them). @@ +425,5 @@ > + hash = aDownload.saver.getSha256Hash(); > + } catch (ex) { > + // Bail if DownloadSaver doesn't have a hash. > + return Promise.resolve(false); > + } In addition to getSha256Hash not implemented, we should also check the case where it returns null, right? @@ +431,5 @@ > + // Construct the ApplicationReputationQuery > + let query = { > + sourceURI: NetUtil.newURI(aDownload.source.url), > + fileSize: aDownload.currentBytes, > + sha256hash: hash This should be sha256Hash. I think we can now do an end-to-end manual test (rather than a unit test) to exercise this glue code path. @@ +433,5 @@ > + sourceURI: NetUtil.newURI(aDownload.source.url), > + fileSize: aDownload.currentBytes, > + sha256hash: hash > + }; > + gApplicationReputationService.queryReputation(query, nit: may inline the "query" object like we usually do for Places calls.

(In reply to :Paolo Amadini from comment #24) > @@ +1265,5 @@ > > + > > + /** > > + * Indicates the download was blocked because it was malware. > > + */ > > + becauseBlockedByApplicationReputation: false, > > So, I wonder whether we should call this more generically > "becauseBlockedByMalwareCheck" rather than > "becauseBlockedByApplicationReputation", also seeing the comment here. > > I'll ask for Marco's consultancy on this interface addition as well.

Comment on attachment 827623 [details] [diff] [review] Integrate application reputation with download manager, disable remote lookups for application reputation ( Address Paolo's comments. Re: manual testing, thanks for reminding me, I discovered that I had omitted setting DOWNLOAD_DIRTY state in updateFromJSDownload and was failing with a generic error. To test: 1) Register for a Google API key https://code.google.com/apis/console/ and enable Safebrowsing in the services tab of the API console 2) Replace instances of %GOOGLE_API_KEY% in browser/app/profile/firefox.js with your key 3) Wait and see if goog-badbinurl-shavar and goog-downloadwhite-digest256 get downloaded into your <profile>/safebrowsing directory 4) Download some files. Unfortunately there is no publicly available URL for testing like there is for the phishing URL checks -- so to trigger the malware UI manually change shouldBlockForMalwareCheck to true. If the lists have not been downloaded yet, the shouldBlock check is always false.

> > Address Paolo's comments. Re: manual testing, thanks for reminding me, I > discovered that I had omitted setting DOWNLOAD_DIRTY state in > updateFromJSDownload and was failing with a generic error. I forgot to mention, the malware-specific error is "Blocked: May contain a virus or spyware". The generic error is simply "Failed."

Try was mostly green: https://tbpl.mozilla.org/?tree=Try&rev=516bc84169b6

(In reply to :Paolo Amadini from comment #25) > (In reply to :Paolo Amadini from comment #24) > > > + /** > > > + * Indicates the download was blocked because it was malware. > > > + */ > > > + becauseBlockedByApplicationReputation: false, > > > > So, I wonder whether we should call this more generically > > "becauseBlockedByMalwareCheck" rather than > > "becauseBlockedByApplicationReputation", also seeing the comment here. > > > > I'll ask for Marco's consultancy on this interface addition as well. False positives exists, so I don't like call anything a malware by default. What about something in the middle: /** * The download was blocked cause it didn't pass the reputation * check. This may indicate a malware. */ becauseBlockedByReputationCheck: false, That said all of the suggestions here are acceptable, it's mostly internal detail how we decide to call the feature, we should not over-engineer a name here.

Comment on attachment 827623 [details] [diff] [review] Integrate application reputation with download manager, disable remote lookups for application reputation ( Looks good! Remove the unneeded "dump" call and, per Marco's previous comment, feel free to select the name you deem appropriate for the DownloadError property.

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #27) > Unfortunately there is no publicly available URL for > testing like there is for the phishing URL checks Is a public testing URL being set up? Or can we request one?

(In reply to :Paolo Amadini from comment #31) > Comment on attachment 827623 [details] [diff] [review] > Integrate application reputation with download manager, disable remote > lookups for application reputation ( > > Looks good! Remove the unneeded "dump" call and, per Marco's previous > comment, feel free to select the name you deem appropriate for the > DownloadError property. Renamed as Marco suggested, and now I realize I forgot to refresh after removing the dump call. I will create a new patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/a818c7fa1372 https://hg.mozilla.org/integration/mozilla-inbound/rev/13beeed7c609 > > Unfortunately there is no publicly available URL for > > testing like there is for the phishing URL checks > > Is a public testing URL being set up? Or can we request one? I will request one.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab109e8391a to remove the extra dump call.

https://hg.mozilla.org/mozilla-central/rev/13beeed7c609 https://hg.mozilla.org/mozilla-central/rev/a818c7fa1372 https://hg.mozilla.org/mozilla-central/rev/4ab109e8391a