Closed Bug 1011799 Opened 6 years ago Closed 6 years ago

integrate redirects into application reputation check

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(1 file, 1 obsolete file)

Coverage basically sucks without it.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8424241 - Attachment is obsolete: true
Comment on attachment 8429656 [details] [diff] [review]
Integrate nsIRedirectChannel redirects into application reputation check

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

This change depends on bug 974018, but it looks like Honza and Patrick are basically happy with the interface changes there so I don't expect those to change much.
Attachment #8429656 - Flags: review?(paolo.mozmail)
Attachment #8429656 - Flags: review?(gpascutto)
Comment on attachment 8429656 [details] [diff] [review]
Integrate nsIRedirectChannel redirects into application reputation check

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +752,1 @@
>    safe_browsing::ClientDownloadRequest req;

will delete
Comment on attachment 8429656 [details] [diff] [review]
Integrate nsIRedirectChannel redirects into application reputation check

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

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +547,5 @@
> +PendingLookup::AddRedirects(nsIArray* aRedirects)
> +{
> +  uint32_t length = 0;
> +  aRedirects->GetLength(&length);
> +  LOG(("ApplicationReputation: Got %d redirects", length));

Signed vs unsigned mismatch.

@@ +766,2 @@
>    // We have no way of knowing whether or not a user initiated the download.
> +  mRequest.set_user_initiated(false);

Do we have a bug for this? How useful is it for the detection ratio?

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1963,4 @@
>      // Free the reference that the saver keeps on us, even if we couldn't get
>      // the hash.
>      mSaver = nullptr;
>    

nit: Kill this spurious whitespace while you're at it :)

@@ +1969,5 @@
> +    if (history) {
> +      (void)history->GetRedirects(getter_AddRefs(mRedirects));
> +      uint32_t length = 0;
> +      mRedirects->GetLength(&length);
> +      LOG(("nsExternalAppHandler: Got %d redirects\n", length));

Signed vs unsigned mismatch.
Attachment #8429656 - Flags: review?(gpascutto) → review+
Comment on attachment 8429656 [details] [diff] [review]
Integrate nsIRedirectChannel redirects into application reputation check

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

Looks good. Have you already filed a bug about sanitizing any embedded username, password, hash or relevant data in the URIs, when checking them against whitelists or sending them remotely?

::: toolkit/components/downloads/nsIApplicationReputation.idl
@@ +82,5 @@
>     */
>    readonly attribute nsIArray signatureInfo;
> +
> +  /*
> +   * The nsIArray of nsIPrincipal of redirects that lead to this download.

Which ones are first, the most recent or the least recent in the chain?

I'd specify the same information in the other comments throughout the code, even if they are not on the IDL.
Attachment #8429656 - Flags: review?(paolo.mozmail) → review+
Thanks everybody, filed https://bugzilla.mozilla.org/show_bug.cgi?id=1017140 about url param stripping and https://bugzilla.mozilla.org/show_bug.cgi?id=1017145 for investigating user_initiated further. Unfortunately I don't know how much extra coverage that field provides.

I will update comments, fix %d to %u, and wait on try: https://tbpl.mozilla.org/?tree=Try&rev=2c2eedee5b07
And I forgot to qref to pickup the comment changes before pushing to inbound. Also I had forgotten to update nsIApplicationReputation.idl uuid after adding a field.

 https://hg.mozilla.org/integration/mozilla-inbound/rev/f92292ad5bea
https://hg.mozilla.org/mozilla-central/rev/566a41f1dbaa
https://hg.mozilla.org/mozilla-central/rev/f92292ad5bea
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.