Closed Bug 368106 Opened 13 years ago Closed 7 years ago

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

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: Dolske, Assigned: maxli)

References

()

Details

(Keywords: privacy, Whiteboard: [mentor=gavin])

Attachments

(1 file, 1 obsolete file)

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
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → maxli
Attachment #745636 - Flags: review?(gavin.sharp)
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!
Attachment #745636 - Flags: review?(gavin.sharp) → review+
Attached patch Patch v2Splinter Review
Patch with fixes
Keywords: checkin-needed
Attachment #745636 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8ef4542be8fd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.