402370 - Phishing/Malware error page is chrome privileged

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Johnathan Nightingale [:johnath]]

A side effect of NOT marking the malware/phishing error page as "URI_SAFE_FOR_UNTRUSTED_CONTENT" here:

http://mxr.mozilla.org/mozilla/source/browser/components/safebrowsing/content/application.js#118

...is that it ends up with chrome privileges.  We should decide if this is something we want or not.  I obviously lean towards not having privilege anywhere we don't absolutely need it, and the current functionality on that page can be written in a way that doesn't require privilege.

On the other hand, bugs like bug 396696 will be more complicated to implement if the page doesn't have the ability to query XPCOM services.  Workarounds can probably be devised there, though.

Not returning URI_SAFE_FOR_UNTRUSTED_CONTENT does mean that random sites on the web can't explicitly link to it, or nest it in a frame, but since the list of blacklisted sites is available, an attacker could include a frame linking to a known malware site, and cause the page to be displayed.  At that point, any cross-frame vulnerability we had (e.g. cross frame script injection) could become a privilege escalation attack.

Comment 1

[Daniel Veditz [:dveditz]]

The inability to load about:config or similar privileged chrome is all that's stood between XSS vulnerabilities and remote abitrary code execution on numerous occasions.

We could add a new flag used by the about redirector to determine privilege, but short of that is there any real problem with using the URI_SAFE_FOR_UNTRUSTED_CONTENT here? I guess some joker could then use an XSS hole to deface a site by redirecting to about:blocked, but really, doing so just proves the site might in fact be unsafe so I'm kinda OK with that.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for investigation.

Comment 3

[Johnathan Nightingale [:johnath]]

Dan - you've been around the block more than almost anyone on chrome privilege and the like - what do you think the best approach is here?

Obviously, for the explicit actions on the page like button clicks, we can use the same approach I used in bug 401575, where the click event bubbles up into chrome, and we act on it there.  The "how Minefield protects you" link is a localized string pref though, and reaching that from content will be tough, I suspect.  And as I mention above, if we start putting things like "last updated on" dates in there, that sounds like more XPCOM traffic.  

Any thoughts you have here would be really helpful. 

Comment 4

[Johnathan Nightingale [:johnath]]

This is marked P3, but we really should take it - upping to P2.  Patch incoming.

Comment 5

[Johnathan Nightingale [:johnath]]

Attachment: Drop privilege, move strings to dtd, allow about:blocked to have chrome favicons

This doesn't meaningfully *change* any strings, but it moves them from a properties file to a DTD since non-chrome can't play with string bundles.  That means late-l10n, which means I'm going to try to find someone who can review RSN.

Comment 6

[Johnathan Nightingale [:johnath]]

The properties file these strings come from:

http://mxr.mozilla.org/mozilla/source/browser/locales/en-US/chrome/browser/safebrowsing/blockedSite.properties

is no longer needed after this patch.  All the strings are referenced only within blockedSite.xhtml, so I intend to remove that on checkin as well.

Comment 7

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

Comment on attachment 308023 [details] [diff] [review]
Drop privilege, move strings to dtd, allow about:blocked to have chrome favicons

Would it be best to have all elements hidden by default, and unhide the relevant ones, rather than showing all and removing the unneeded ones? Would handle catastrophic failure a bit better (missing text rather than extra contradictory text? maybe that's not better...), but more of an edge case for sure.

Comment 8

[Johnathan Nightingale [:johnath]]

Comment on attachment 308023 [details] [diff] [review]
Drop privilege, move strings to dtd, allow about:blocked to have chrome favicons

Requesting explicit approval for late-l10n

Comment 9

[Johnathan Nightingale [:johnath]]

(In reply to comment #7)
> (From update of attachment 308023 [details] [diff] [review])
> Would it be best to have all elements hidden by default, and unhide the
> relevant ones, rather than showing all and removing the unneeded ones? Would
> handle catastrophic failure a bit better (missing text rather than extra
> contradictory text? maybe that's not better...), but more of an edge case for
> sure.

Yeah, it's kinda weird, but netError does it this way too, because of bug 39098.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 308023 [details] [diff] [review]
Drop privilege, move strings to dtd, allow about:blocked to have chrome favicons

a=beltzner

Comment 11

[Johnathan Nightingale [:johnath]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.995; previous revision: 1.994
done
Checking in browser/components/safebrowsing/content/application.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/application.js,v  <--  application.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/components/safebrowsing/content/blockedSite.xhtml;
/cvsroot/mozilla/browser/components/safebrowsing/content/blockedSite.xhtml,v  <--  blockedSite.xhtml
new revision: 1.3; previous revision: 1.2
done
Checking in browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd,v  <--  phishing-afterload-warning-message.dtd
new revision: 1.14; previous revision: 1.13
done

Used a vague check-in comment just in case, leaving hidden until a beta ships with the fix.