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.

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.

Blocking for investigation.

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.

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

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.

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 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 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

(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 on attachment 308023 [details] [diff] [review] Drop privilege, move strings to dtd, allow about:blocked to have chrome favicons a=beltzner

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.