Closed Bug 1055670 Opened 6 years ago Closed 6 years ago

disable remote application reputation checks

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox31 --- unaffected
firefox32 + verified
firefox33 + verified
firefox34 + verified

People

(Reporter: mmc, Assigned: gcp)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Disable remote lookups.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Comment on attachment 8475287 [details] [diff] [review]
Disable remote lookups (

Review of attachment 8475287 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +426,5 @@
> +  nsCString serviceUrl;
> +  NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl),
> +                    NS_ERROR_NOT_AVAILABLE);
> +  if (serviceUrl.EqualsLiteral("")) {
> +    return NS_ERROR_NOT_AVAILABLE;

I was wondering why you'd not return OnComplete(false, NS_OK) as happens in the non-Windows codepath, but it looks like
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/ApplicationReputation.cpp#626
means it ends up doing that anyway?
Attachment #8475287 - Attachment is obsolete: true
Duplicate of this bug: 1055732
Comment on attachment 8475325 [details] [diff] [review]
Disable remote lookups (

Review of attachment 8475325 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +426,5 @@
> +  nsCString serviceUrl;
> +  NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl),
> +                    NS_ERROR_NOT_AVAILABLE);
> +  if (serviceUrl.EqualsLiteral("")) {
> +    return OnComplete(false, NS_ERROR_NOT_AVAILABLE);

This looks wrong.

http://dxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIApplicationReputation.idl#93
   *        NS_OK if and only if the query succeeded. If it did, then
   *        shouldBlock is meaningful (otherwise it defaults to false). This
   *        may be NS_ERROR_FAILURE if the response cannot be parsed, or
   *        NS_ERROR_NOT_AVAILABLE if the service has been disabled or is not
   *        reachable.

This means that you do end up ignoring the local lists again, because returning an error
means that shouldBlock is not meaningful, per the above. (or the documentation is wrong)

From reading that, you need OnComplete(false, NS_OK) when the remote server isn't set
but you still want local blocks, which matches the !(#ifdef XP_WIN) path.
Attachment #8475325 - Flags: review-
Comment on attachment 8475325 [details] [diff] [review]
Disable remote lookups (

Review of attachment 8475325 [details] [diff] [review]:
-----------------------------------------------------------------

After reading the code closer, this is OK, but I'm still curious about the test.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +426,5 @@
> +  nsCString serviceUrl;
> +  NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl),
> +                    NS_ERROR_NOT_AVAILABLE);
> +  if (serviceUrl.EqualsLiteral("")) {
> +    return OnComplete(false, NS_ERROR_NOT_AVAILABLE);

Reading more, we'll bail out as soon as we get a local hit, so that's not an issue, and the calling code never actually checks the rv value (which wouldn't matter if it defaults to shouldBlock=false anyway) 

So it'll work.

I guess this also leaves room for UX like "the download looks fine but the AppRep server couldn't be contacted".

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ -170,5 @@
> -  gAppRep.queryReputation({
> -    sourceURI: createURI("http://example.com"),
> -    fileSize: 12,
> -  }, function onComplete(aShouldBlock, aStatus) {
> -    // We should be getting NS_ERROR_NOT_AVAILABLE if the service is disabled

Why doesn't this test work any more?
Attachment #8475325 - Flags: review- → review+
Comment on attachment 8475325 [details] [diff] [review]
Disable remote lookups (

Review of attachment 8475325 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ -170,5 @@
> -  gAppRep.queryReputation({
> -    sourceURI: createURI("http://example.com"),
> -    fileSize: 12,
> -  }, function onComplete(aShouldBlock, aStatus) {
> -    // We should be getting NS_ERROR_NOT_AVAILABLE if the service is disabled

It works now, reverting. It didn't work in the first iteration of this patch.
Attachment #8475325 - Attachment is obsolete: true
Comment on attachment 8475393 [details] [diff] [review]
Disable remote lookups (

Approval Request Comment
[Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=662819

[User impact if declined]:
Users will have improved malware detection through remote lookups, but continue have no way to recover the downloaded file.

[Describe test coverage new/current, TBPL]:
This requires manual testing on a release build to make sure the pref browser.safebrowsing.appRepURL is blank on beta and release. Unittests are test_app_rep.js and test_app_rep_windows.js in toolkit/components/url-classifier.

[Risks and why]: Pretty low risk. This makes malware detection the same as in FF 31 instead of increasing coverage.

[String/UUID change made/needed]: None.
Attachment #8475393 - Flags: review+
Attachment #8475393 - Flags: approval-mozilla-beta?
Attachment #8475393 - Flags: approval-mozilla-aurora?
Erm, I was wrong about the test change not being needed. The old way threw immediately, the new way does not.
Btw, the patch attached for beta approval is still correct and combines both comment 13 and comment 9.
QA Contact: mwobensmith
https://hg.mozilla.org/mozilla-central/rev/cb5e6705205d
https://hg.mozilla.org/mozilla-central/rev/082bcda8015a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
This fixes & re-enables the disabled test.
Attachment #8475981 - Flags: review?(mmc)
Assignee: mmc → gpascutto
Comment on attachment 8475981 [details] [diff] [review]
Re-enable removed ApplicationReputation tests

Review of attachment 8475981 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/test/unit/test_app_rep.js
@@ +171,5 @@
> +  gAppRep.queryReputation({
> +    sourceURI: createURI("http://example.com"),
> +    fileSize: 12,
> +  }, function onComplete(aShouldBlock, aStatus) {
> +    var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);

Yikes, is this the only way to do this? It might be cleaner just to do this test in test_app_rep_windows.js.
It's what the documentation recommends: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

Moving the test to test_app_rep_windows.js also works, but loses the test coverage on non-Windows. (It's currently verifying that the local lists are still checked if the appRepURL is empty, i.e. it's exactly testing what you just patched)
Hmm, not true given that the patch is local to the XP_WIN codepath. I guess we can move it then.
Patch is on m-c already. (comment 18)
Attachment #8475981 - Attachment is obsolete: true
Attachment #8475981 - Flags: review?(mmc)
Attachment #8476091 - Flags: review?(mmc)
Comment on attachment 8476091 [details] [diff] [review]
v2. Re-enable removed ApplicationReputation tests

Review of attachment 8476091 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Probably better not to do this kind of follow on work in the same bug that's being evaluated for uplift in order to avoid confusing approvers.
Attachment #8476091 - Flags: review?(mmc) → review+
Comment on attachment 8475393 [details] [diff] [review]
Disable remote lookups (

Approved for Aurora and Beta. This functionality will be reenabled in a later release.
Attachment #8475393 - Flags: approval-mozilla-beta?
Attachment #8475393 - Flags: approval-mozilla-beta+
Attachment #8475393 - Flags: approval-mozilla-aurora?
Attachment #8475393 - Flags: approval-mozilla-aurora+
Flags: in-testsuite?
Matt, please verify that browser.safebrowsing.appRepURL is blank on beta, filled in on nightly and aurora, and that you can download files on Windows.
Flags: needinfo?(mwobensmith)
I've verified that pref does not appear in Fx32, but does in Fx33 and Fx34. 
I've downloaded files - including an EXE - on Windows 8 Fx32 and no issues.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mwobensmith)
Depends on: 1057764
i have problems downloading files after this landed in beta, see bug 1057764
You need to log in before you can comment on or make changes to this bug.