maybe strip URL params from remote application reputation check

RESOLVED FIXED in mozilla32

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mmc, Assigned: mmc)

Tracking

(Blocks: 1 bug)

unspecified
mozilla32
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

For local lookups, this doesn't matter, because we do canonicalization in the classifier.

For remote lookups, double-check what the Chrome code does, we may be able to get away with stripping all url params but I doubt we can strip paths.
This needs to go out in 32, which is what adds the redirect chain to the remote lookup.
Blocks: 662819
Created attachment 8433522 [details] [diff] [review]
Strip url params for application reputation lookups (
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Comment on attachment 8433522 [details] [diff] [review]
Strip url params for application reputation lookups (

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

Google confirmed that this was fine. I decided to go with scheme + hostport rather than nsIURI.prepath, because prepath can include user@pass for HTTP basic auth. Not that anyone uses that anymore.
Attachment #8433522 - Flags: review?(gpascutto)
Comment on attachment 8433522 [details] [diff] [review]
Strip url params for application reputation lookups (

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

Paolo, could you take a look? I think gcp might be out and I'd like to avoid requesting uplift if possible :)
Attachment #8433522 - Flags: review?(paolo.mozmail)

Comment 5

5 years ago
Comment on attachment 8433522 [details] [diff] [review]
Strip url params for application reputation lookups (

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

I took a look at the current patch, mainly it needs a test but there are also other small things to fix.

I'll probably be able to review the next iteration tomorrow if gcp is out today.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +148,5 @@
>                                   nsresult aResult,
>                                   bool* aShouldBlock);
>  
> +  // Strip url parameters, fragments, and user@pass fields from the URI spec.
> +  nsresult GetEscapedSpec(nsIURI* aUri, nsACString& spec);

GetStrippedSpec seems a better name to me, since this function does not do character escaping (like the two functions below it do).

@@ +591,5 @@
>      rv = principal->GetURI(getter_AddRefs(uri));
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    nsCOMPtr<nsIURL> url = do_QueryInterface(uri, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

Not all URIs implement nsIURL. If they don't, this call will fail.

We should probably work with nsIURI and if the URI does not support nsIURL, just use the full spec.

@@ +628,5 @@
> +PendingLookup::GetEscapedSpec(nsIURI* aUri, nsACString& escaped)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIURL> url = do_QueryInterface(aUri, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

So, we can return success with the full spec if this fails.

We need a test. If an end-to-end test is too difficult, we can just expose the function to JavaScript and check that it does the expected thing with various URIs and URLs.

@@ +659,5 @@
>    nsresult rv = mQuery->GetSourceURI(getter_AddRefs(uri));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCString spec;
> +  rv = GetEscapedSpec(uri, spec);

One NS_ENSURE_SUCCESS call was lost by accident here.
Attachment #8433522 - Flags: review?(paolo.mozmail)
Created attachment 8436170 [details] [diff] [review]
Strip url params for application reputation lookups (
Attachment #8433522 - Attachment is obsolete: true
Attachment #8433522 - Flags: review?(gpascutto)
Created attachment 8436172 [details] [diff] [review]
Strip url params for application reputation lookups (
Attachment #8436170 - Attachment is obsolete: true
Comment on attachment 8436172 [details] [diff] [review]
Strip url params for application reputation lookups (

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

Hi Paolo,

Thanks for the quick review! I renamed to GetStrippedSpec and added bunch of cruft to the test URLs to check that lookups still work (the safebrowsing lookup canonicalization does not deal with params), and also accept nsIURIs that are not nsIURLs. I also verified through logging that params are being stripped as expected.

A couple of things: I couldn't find a way to instantiate an nsIURI that is not an nsIURL (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/﷒0﷓), and I am not sure it makes sense to call this code with non-URLs anyway. Uris are filled in through the download manager and through nsIRedirectHistory, which is only implemented on HttpChannel (child and parent), so they will definitely have nsIURLs. I don't think it hurts to leave it as is, but it probably doesn't make much sense either.

Have a great weekend,
Monica
Attachment #8436172 - Flags: review?(paolo.mozmail)

Comment 9

5 years ago
Comment on attachment 8436172 [details] [diff] [review]
Strip url params for application reputation lookups (

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #8)
> A couple of things: I couldn't find a way to instantiate an nsIURI that is
> not an nsIURL

You can try this in the browser console:

NetUtil.newURI("data:application/octet-stream,ABC") instanceof Ci.nsIURL

=> false

> and I am not sure it makes sense to call this code with non-URLs anyway.

This is a good point. Note that typing the URI above in the location bar does start a download. Do we (should we) filter those URIs out from remote checks? "data:" URIs could be used to create malicious files or documents.

> nsIRedirectHistory, which is only implemented on HttpChannel (child and
> parent), so they will definitely have nsIURLs

Right, intermediate redirect chains will definitely be nsIURLs.
Attachment #8436172 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #9)
> Comment on attachment 8436172 [details] [diff] [review]
> Strip url params for application reputation lookups (
> 
> (In reply to [:mmc] Monica Chew (please use needinfo) from comment #8)
> > A couple of things: I couldn't find a way to instantiate an nsIURI that is
> > not an nsIURL
> 
> You can try this in the browser console:
> 
> NetUtil.newURI("data:application/octet-stream,ABC") instanceof Ci.nsIURL
> 
> => false
> 
> > and I am not sure it makes sense to call this code with non-URLs anyway.
> 
> This is a good point. Note that typing the URI above in the location bar
> does start a download. Do we (should we) filter those URIs out from remote
> checks? "data:" URIs could be used to create malicious files or documents.

On reviewing the API doc they gave me again, I think we should be stripping these. They refer to the local lists as URL patterns of malware downloads, so I will revert the code that returns NS_OK on getting an non-nsIURL before checking in. Thanks for thinking of the data urls :)
Updated test to construct non-nsIURLs and make sure we are failing correctly:

https://tbpl.mozilla.org/?tree=Try&rev=063751111443
https://hg.mozilla.org/mozilla-central/rev/5bd30aa6ace2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.