1011799 - integrate redirects into application reputation check

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Coverage basically sucks without it.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate nsIRedirectChannel redirects into application reputation check

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Integrate nsIRedirectChannel redirects into application reputation check

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

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 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[:Paolo Amadini]

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.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a94326b20f5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/566a41f1dbaa

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/566a41f1dbaa
https://hg.mozilla.org/mozilla-central/rev/f92292ad5bea