Downloads blocked by Application Reputation currently don't provide information about the verdict. This information will be required to differentiate the user interface when unblocking downloads whose verdict is UNCOMMON or POTENTIALLY_UNWANTED.

Review commit: https://reviewboard.mozilla.org/r/40429/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40429/

Review commit: https://reviewboard.mozilla.org/r/40449/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40449/

Review commit: https://reviewboard.mozilla.org/r/40451/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40451/

Comment on attachment 8731228 [details] MozReview Request: Bug 1254100 - Part 1 - The Application Reputation interface should provide information about the verdict. r=gcp https://reviewboard.mozilla.org/r/40451/#review36933 ::: toolkit/components/downloads/ApplicationReputation.cpp:169 (Diff revision 1) > ClientDownloadRequest::DownloadType GetDownloadType(const nsAString& aFilename); > > // Clean up and call the callback. PendingLookup must not be used after this > // function is called. > - nsresult OnComplete(bool shouldBlock, nsresult rv); > + nsresult OnComplete(bool shouldBlock, nsresult rv, > + uint32_t verdict = nsIApplicationReputationService::VERDICT_NONE); nit: indentation looks off ::: toolkit/components/downloads/ApplicationReputation.cpp:350 (Diff revision 1) > if (!mAllowlistOnly && FindInReadable(blockList, tables)) { > mPendingLookup->mBlocklistCount++; > Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, BLOCK_LIST); > LOG(("Found principal %s on blocklist [this = %p]", mSpec.get(), this)); > - return mPendingLookup->OnComplete(true, NS_OK); > + return mPendingLookup->OnComplete(true, NS_OK, > + nsIApplicationReputationService::VERDICT_MALWARE); nit: indentation ::: toolkit/components/downloads/ApplicationReputation.cpp:1143 (Diff revision 1) > // Treat everything else as safe > break; > } > > + if (!*aShouldBlock) { > + *aVerdict = nsIApplicationReputationService::VERDICT_NONE; This allows the user pref to override what verdict gets reported. I think that any downstream code that cares about whether it gets blocked or not should check shouldBlock directly instead, so remove this code to let the actual verdict propagate. Downstream users can then still decided if they want to clear the verdict if shouldBlock = false. This may be quite relevant for UNCOMMON verdicts, which we don't block IIRC. ::: toolkit/components/downloads/nsIApplicationReputation.idl:24 (Diff revision 1) > interface nsIApplicationReputationService : nsISupports { > /** > + * Indicates the reason for the application reputation block. > + */ > + const uint32_t VERDICT_NONE = 0; > + const uint32_t VERDICT_MALWARE = 1; Make this match https://dxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/csd.proto#277 so things don't get confusing if an actual MALWARE verdict gets added. (The mismatch between MALWARE and DANGEROUS is already confusing)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4) > > - nsresult OnComplete(bool shouldBlock, nsresult rv); > > + nsresult OnComplete(bool shouldBlock, nsresult rv, > > + uint32_t verdict = nsIApplicationReputationService::VERDICT_NONE); > > nit: indentation looks off > > > - return mPendingLookup->OnComplete(true, NS_OK); > > + return mPendingLookup->OnComplete(true, NS_OK, > > + nsIApplicationReputationService::VERDICT_MALWARE); > > nit: indentation What should it look like? There are different styles...

(In reply to :Paolo Amadini from comment #5) > (In reply to Gian-Carlo Pascutto [:gcp] from comment #4) > > > - nsresult OnComplete(bool shouldBlock, nsresult rv); > > > + nsresult OnComplete(bool shouldBlock, nsresult rv, > > > + uint32_t verdict = nsIApplicationReputationService::VERDICT_NONE); > > > > nit: indentation looks off > > > > > - return mPendingLookup->OnComplete(true, NS_OK); > > > + return mPendingLookup->OnComplete(true, NS_OK, > > > + nsIApplicationReputationService::VERDICT_MALWARE); > > > > nit: indentation > > What should it look like? There are different styles... Most of this code vertically aligns the vertically arguments, so nsIApplicationReputationService::VERDICT_MALWARE would line up with "true". If it's too long to fit, just do a single normal indent.

Comment on attachment 8731228 [details] MozReview Request: Bug 1254100 - Part 1 - The Application Reputation interface should provide information about the verdict. r=gcp Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40451/diff/1-2/

Comment on attachment 8731227 [details] MozReview Request: Bug 1254100 - Part 2 - Downloads blocked by Application Reputation should provide information about the verdict. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40449/diff/1-2/

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6) > Most of this code vertically aligns the vertically arguments, so > nsIApplicationReputationService::VERDICT_MALWARE would line up with "true". > If it's too long to fit, just do a single normal indent. Thanks, looks like the normal indent will do to avoid going over 80 characters.

Comment on attachment 8731228 [details] MozReview Request: Bug 1254100 - Part 1 - The Application Reputation interface should provide information about the verdict. r=gcp https://reviewboard.mozilla.org/r/40451/#review36959 Please f? francois as well. He's more familiar with this part of the code.

Comment on attachment 8731228 [details] MozReview Request: Bug 1254100 - Part 1 - The Application Reputation interface should provide information about the verdict. r=gcp (In reply to Gian-Carlo Pascutto [:gcp] from comment #10) > Please f? francois as well. He's more familiar with this part of the code. Sure.

Comment on attachment 8731228 [details] MozReview Request: Bug 1254100 - Part 1 - The Application Reputation interface should provide information about the verdict. r=gcp https://reviewboard.mozilla.org/r/40451/#review37139 That looks great to me. I only have two small suggestions to consider before you land. ::: toolkit/components/downloads/ApplicationReputation.cpp:769 (Diff revision 2) > } > > nsresult > -PendingLookup::OnComplete(bool shouldBlock, nsresult rv) > +PendingLookup::OnComplete(bool shouldBlock, nsresult rv, uint32_t verdict) > { > if (NS_FAILED(rv)) { I would suggest an MOZ_ASSERT() here: (shouldBlock && verdict != SAFE) || (!shouldBlock && verdict == SAFE) ::: toolkit/components/downloads/ApplicationReputation.cpp:785 (Diff revision 2) > > Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_SHOULD_BLOCK, > shouldBlock); > double t = (TimeStamp::Now() - mStartTime).ToMilliseconds(); > if (shouldBlock) { > LOG(("Application Reputation check failed, blocking bad binary in %f ms " I rely on this logging quite a bit while testing this feature, so I would suggest exposing the verdict (as a uint is fine) in that LOG.

https://reviewboard.mozilla.org/r/40449/#review37141 ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:572 (Diff revision 2) > try { > hash = aDownload.saver.getSha256Hash(); > sigInfo = aDownload.saver.getSignatureInfo(); > channelRedirects = aDownload.saver.getRedirects(); > } catch (ex) { > // Bail if DownloadSaver doesn't have a hash or signature info. Question: will downloads on Linux have signature info? We started looking them up against the remote server last year despite the lack of signatures, we should make sure we keep doing that.

https://reviewboard.mozilla.org/r/40451/#review37273 ::: toolkit/components/downloads/nsIApplicationReputation.idl:27 (Diff revision 2) > + */ > + const uint32_t VERDICT_SAFE = 0; > + const uint32_t VERDICT_DANGEROUS = 1; > + const uint32_t VERDICT_UNCOMMON = 2; > + const uint32_t VERDICT_POTENTIALLY_UNWANTED = 3; > + const uint32_t VERDICT_DANGEROUS_HOST = 4; is there a specific reason to use uint32_t here instead of the usual (per convention) unsigned long?

Comment on attachment 8731227 [details] MozReview Request: Bug 1254100 - Part 2 - Downloads blocked by Application Reputation should provide information about the verdict. r=mak https://reviewboard.mozilla.org/r/40449/#review37275 The patch looks good to me. The test doesn't seem to be that great, do we have any tests that are actually covering kVerdictMap and the "real" onComplete code path?

(In reply to François Marier [:francois] from comment #12) > > +PendingLookup::OnComplete(bool shouldBlock, nsresult rv, uint32_t verdict) > > I would suggest an MOZ_ASSERT() here: > > (shouldBlock && verdict != SAFE) || (!shouldBlock && verdict == SAFE) That's what I originally thought but not what Gian-Carlo asked for in comment 4...

(In reply to Marco Bonardo [::mak] from comment #14) > > + const uint32_t VERDICT_DANGEROUS_HOST = 4; > > is there a specific reason to use uint32_t here instead of the usual (per > convention) unsigned long? No, I copied this from some randomly picked IDL file that apparently used "uint32_t" instead of "unsigned long". I can update the IDL to use the latter. (In reply to Marco Bonardo [::mak] from comment #15) > The patch looks good to me. The test doesn't seem to be that great, do we > have any tests that are actually covering kVerdictMap and the "real" > onComplete code path? No, that relies on the manual testing that will be done in the dependent bug. There's bug 899013 on file to make it easier to switch portions of DownloadIntegration to enable end-to-end tests.

(In reply to :Paolo Amadini from comment #16) > (In reply to François Marier [:francois] from comment #12) > > > +PendingLookup::OnComplete(bool shouldBlock, nsresult rv, uint32_t verdict) > > > > I would suggest an MOZ_ASSERT() here: > > > > (shouldBlock && verdict != SAFE) || (!shouldBlock && verdict == SAFE) > > That's what I originally thought but not what Gian-Carlo asked for in > comment 4... Some subtle differences though. You were changing/overriding the verdict passed down, which is different from asserting (in a debug build) that the default settings are as expected. Asserting !shouldBlock if verdict == SAFE seems OK to me in any case, I don't see prefs or changes to what we block overruling that :-)

(In reply to Gian-Carlo Pascutto [:gcp] from comment #18) > Some subtle differences though. You were changing/overriding the verdict > passed down, which is different from asserting (in a debug build) that the > default settings are as expected. I've some difficulty parsing this comment, but... > Asserting !shouldBlock if verdict == SAFE seems OK to me in any case, I > don't see prefs or changes to what we block overruling that :-) ...yeah, basically what I'm saying is that you asked only for what's represented by the second part of this assert, not the first one. Francois asked for something different.

(In reply to :Paolo Amadini from comment #19) > ...yeah, basically what I'm saying is that you asked only for what's > represented by the second part of this assert, not the first one. Francois > asked for something different. Right, I forgot about the prefs I added to control which verdicts to honour :) You guys are right, there could be more than one reason for `shouldBlock == false`. So let's amend my suggestion to: MOZ_ASSERT(!shouldBlock || verdict != VERDICT_SAFE); Also, I would suggest including the verdict ID in the MOZ_LOG() we print out when `shouldBlock == false`.

Comment on attachment 8731228 [details] MozReview Request: Bug 1254100 - Part 1 - The Application Reputation interface should provide information about the verdict. r=gcp Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40451/diff/2-3/

Comment on attachment 8731227 [details] MozReview Request: Bug 1254100 - Part 2 - Downloads blocked by Application Reputation should provide information about the verdict. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40449/diff/2-3/

I've split the log in two lines for simplicity. (In reply to François Marier [:francois] from comment #13) > Question: will downloads on Linux have signature info? No: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/BackgroundFileSaver.cpp#813 > We started looking them up against the remote server last year despite the > lack of signatures, we should make sure we keep doing that. This hasn't changed, as far as I can tell the signature information will not be specified on Linux when we send the lookup request.

https://hg.mozilla.org/integration/fx-team/rev/28fd36d6608b https://hg.mozilla.org/integration/fx-team/rev/6d268e72a031

(In reply to Pulsebot from comment #25) > https://hg.mozilla.org/integration/fx-team/rev/a6d740ef3a54 I missed the namespace on VERDICT_SAFE, I wonder why this compiled at all in my release build. I've pushed this follow-up as an attempt to patch the error. Let's see if it's the only issue in debug builds, otherwise it's better to re-land after a full try build.

https://hg.mozilla.org/mozilla-central/rev/28fd36d6608b https://hg.mozilla.org/mozilla-central/rev/6d268e72a031 https://hg.mozilla.org/mozilla-central/rev/a6d740ef3a54