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

RESOLVED FIXED in Firefox 23



Safe Browsing
12 years ago
4 years ago


(Reporter: Dolske, Assigned: maxli)



Firefox 23

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [mentor=gavin], URL)


(1 attachment, 1 obsolete attachment)



12 years ago
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.
Summary: Query parms sent when reporting a phishing site, could contain sensitive info → Query params sent when reporting a phishing site could contain sensitive info
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.
Keywords: privacy
Whiteboard: [mentor=gavin]
Version: 2.0 Branch → unspecified

Comment 2

5 years ago
Created attachment 745636 [details] [diff] [review]
Assignee: nobody → maxli
Attachment #745636 - Flags: review?(gavin.sharp)
Comment on attachment 745636 [details] [diff] [review]

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!
Attachment #745636 - Flags: review?(gavin.sharp) → review+

Comment 4

5 years ago
Created attachment 746182 [details] [diff] [review]
Patch v2

Patch with fixes


5 years ago
Keywords: checkin-needed
Attachment #745636 - Attachment is obsolete: true
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.