Closed
Bug 1017140
Opened 10 years ago
Closed 10 years ago
maybe strip URL params from remote application reputation check
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: mmc, Assigned: mmc)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
5.92 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
This needs to go out in 32, which is what adds the redirect chain to the remote lookup.
Blocks: downloadprotection
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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•10 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)
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8433522 -
Attachment is obsolete: true
Attachment #8433522 -
Flags: review?(gpascutto)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8436170 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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/public/nsIURI.idl#61), 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•10 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+
Assignee | ||
Comment 10•10 years ago
|
||
(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 :)
Assignee | ||
Comment 11•10 years ago
|
||
Updated test to construct non-nsIURLs and make sure we are failing correctly: https://tbpl.mozilla.org/?tree=Try&rev=063751111443
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd30aa6ace2
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5bd30aa6ace2
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•