Closed Bug 933432 Opened 11 years ago Closed 10 years ago

Enable remote lookups for application reputation on Windows

Categories

(Toolkit :: Downloads API, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 + fixed

People

(Reporter: mmc, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

We can roll this back when bug 928536 is resolved
Assignee: nobody → mmc
Blocks: 895476
No longer depends on: 895476
Comment on attachment 825515 [details] [diff] [review]
Disable remote lookups for application reputation (

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

Split off and addressed comments from https://bugzilla.mozilla.org/show_bug.cgi?id=895476#c11.
Attachment #825515 - Flags: review?(paolo.mozmail)
Sorry for the misunderstanding, I actually meant we should have filed a new bug for re-enabling the preference, to be referenced in the code comments. I've morphed this one to be the one to be referenced in the patch in bug 895476.
No longer blocks: 895476
Depends on: 928536, 895476
Summary: Disable remote lookups for application reputation → Enable remote lookups for application reputation
Attachment #825515 - Attachment is obsolete: true
Attachment #825515 - Flags: review?(paolo.mozmail)
Comment on attachment 826059 [details] [diff] [review]
Disable remote lookups for application reputation (

Sorry, wrong bug.
Attachment #826059 - Attachment is obsolete: true
Depends on: 964465
Summary: Enable remote lookups for application reputation → Enable remote lookups for application reputation on Windows
Depends on: 966557
Depends on: 967298
No longer blocks: 977236
Depends on: 977236
Depends on: 985623
Depends on: 985720
Depends on: 985722
Comment on attachment 8407931 [details] [diff] [review]
Enable remote lookups for application on Windows (

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

I wanted to wait for bug 974018 to enable these, but it looks like that may not happen anytime soon. This patch should set the appid on the channel that we use to send the request. However, I am having some gross Windows problems compiling nsDownloadScanner which relies on Windows CreateEvent which is undef'ed somewhere when including the appropriate LoadContext headers, and haven't quite figured it out yet.

The rest of this patch is ready for review though.
Attachment #8407931 - Flags: review?(gpascutto)
Comment on attachment 8407931 [details] [diff] [review]
Enable remote lookups for application on Windows (

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

r+ though of course you actually have to fix the build problems.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +341,5 @@
> +{
> +  nsString fileName;
> +  nsresult rv = mQuery->GetSuggestedFileName(fileName);
> +  if (NS_FAILED(rv)) {
> +    return false;

I don't know enough about the context, but I just want to remark this is not fail-safe from the security perspective. Do we lose anything if we try to lookup a file we don't know the type of?

@@ +359,5 @@
> +    StringEndsWith(fileName, NS_LITERAL_STRING(".reg")) ||
> +    StringEndsWith(fileName, NS_LITERAL_STRING(".scr")) ||
> +    StringEndsWith(fileName, NS_LITERAL_STRING(".vb")) ||
> +    StringEndsWith(fileName, NS_LITERAL_STRING(".vbs")) ||
> +    StringEndsWith(fileName, NS_LITERAL_STRING(".zip"));

Why did you leave out the Android apk's from upstream?

@@ +395,5 @@
> +    LOG(("Not eligible for remote lookups [this=%x]", this));
> +    return OnComplete(false, NS_OK);
> +  }
> +  // Send the remote query if we are on Windows.
> +#ifdef XP_WIN

No need to do the above check if we're not on Windows, I'd rework the ordering here.
Attachment #8407931 - Flags: review?(gpascutto) → review+
I can restore the apk check and optimize IsBinary. After looking at the Windows thing for a bit, I think the best thing to do is refactor LoadContext.h so it doesn't depend on SerializedLoadContext. That requires some docshell changes so I'll split it into another bug.
Reverted all changes related to the load context and filed bug 998007.

Try looks pretty good so I checked in:
https://tbpl.mozilla.org/?tree=Try&rev=6a9bff532111
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d25da069190

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)

> > +  nsString fileName;
> > +  nsresult rv = mQuery->GetSuggestedFileName(fileName);
> > +  if (NS_FAILED(rv)) {
> > +    return false;
> 
> I don't know enough about the context, but I just want to remark this is not
> fail-safe from the security perspective. Do we lose anything if we try to
> lookup a file we don't know the type of?

This is fail-open which we do throughout, because false positives are generally considered more damaging than false negatives for features like this.
You won't really get false positives with 256-bit hashes, and extra lookup traffic from 32-bit collisions doesn't seem a huge issue as downloads aren't very common at all. So I'm not convinced :)
https://hg.mozilla.org/mozilla-central/rev/8d25da069190
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8407931 [details] [diff] [review]
Enable remote lookups for application on Windows (

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

Hey Monica, I guess I am not the right person to review this part, but I can provide some feedback!

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +25,5 @@
>  #include "nsIX509Cert.h"
>  #include "nsIX509CertDB.h"
>  #include "nsIX509CertList.h"
>  
> +// #include "mozilla/LoadContext.h"

Delete this line, if you are not going to need the include.

@@ +341,5 @@
> +{
> +  nsString fileName;
> +  nsresult rv = mQuery->GetSuggestedFileName(fileName);
> +  if (NS_FAILED(rv)) {
> +    return false;

Nit: use NS_ENSURE_SUCCESS(rv, false);

@@ +773,5 @@
> +  nsCOMPtr<nsIInterfaceRequestor> loadContext =
> +    new mozilla::LoadContext(NECKO_SAFEBROWSING_APP_ID);
> +  rv = channel->SetNotificationCallbacks(loadContext);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  */

Does it have to be the NECKO_SAFEBROWSING_APP_ID? Why not define your own:http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1377
and use UINT32_MAX - 2, we are not going to hit the upper bound of available appiIds (at least not for a while :-))
Do you consider this a blocker?
Flags: needinfo?(mmc)
Hi Benjamin,

No. It is currently enabled (fixed) in 31, but this is new code that may have to be rolled back in case of regression. I nominated for tracking because of that.

Monica
Flags: needinfo?(mmc)
You need to log in before you can comment on or make changes to this bug.