368106 - Query params sent when reporting a phishing site could contain sensitive info

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Justin Dolske [:Dolske]]

A security alert from Finjan was recently issued (see URL), which reported that Google's phishing blacklist contained usernames, passwords, etc. It appears that Google is now filtering their list to remove such entries.

However, Firefox is sending the full URL of the reported site -- including the query string (http://foo.com/foo?querystring). To avoid privacy problems, the browser probably shouldn't be sending the query part at all. Stripping out the query values might also be an option (so that ...?user=me&pw=secret is submitted as ...?user=&pw=).

It seems like just the hostname and URL path should be enough to identify a phishing site.

Also, check to make sure we strip http://user:pass@host/ type URLs.

Addendum: In an old urlclassifier.sqlite, I see a few URLs in the form "http://[google IP]search?q=cache:xxxxxxxxxx". This implies one can phish from legitimate sites using just the query string. Proxy-like sites, such as translate.google.com, have been used in the past for other tricks like this. So there may be a security-vs-privacy tug of war here.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Simple to fix, seems like we should do it.

getReportURL in browser/base/content/browser-safebrowsing.js just needs to clone the currentURI, see whether it's instanceof Ci.nsIURL, and if so strip out the "query" portion.

Comment 2

[Max Li [:maxli]]

Attachment: Patch

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 745636 [details] [diff] [review]
Patch

Adding a comment about why we're stripping the query seems worthwhile (e.g. "remove query to avoid including potentially sensitive data").

I'd also re-name the variable "pageURI" now that it's an nsIURI, since that is more conventional (that you then QI it to nsIURL is potentially confusing, but not a big deal).

Looks good otherwise!

Comment 4

[Max Li [:maxli]]

Attachment: Patch v2

Patch with fixes

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef4542be8fd

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ef4542be8fd