645564 - "view-source:" on DV SSL does not update site identity button text -> spoofing

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Matt McCutchen]

Navigating to a "view-source:" page on a DV SSL site or switching to a tab showing such a page fails to update the text of the site identity button.  This can be used to spoof the name of another DV SSL site in the site identity button by opening a new window on that site and then navigating the window to a "view-source:" page on one's own site.  See the demo in the URL field.

The site identity button color is updated correctly in all cases, and the text is updated correctly when navigating/switching to "view-source:" on an EV SSL site.  Therefore, this bug does not make it possible to spoof text in a green site identity button.

Comment 1

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

The patch in bug 522319 would fix this, by ensuring that view-source pages have useful location objects (i.e. whose .hostname doesn't return an empty string). That's a different issue, though - at a minimum we should avoid throwing here.

Comment 2

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

I filed bug 646690 on the general issue of location.hostname not being useful for view-source pages.

Comment 3

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

Attachment: patch

Removes an unused variable (missed cleanup from http://hg.mozilla.org/mozilla-central/rev/9649eb8bda37 ) and avoids calling hasMatchingOverride if the hostname is empty (which throws).

Comment 4

[Matt McCutchen]

The patch seems to work.  I wonder if it might be prudent to completely clear the site identity button/popup at the beginning in order to fail secure if there are any other unforeseen exceptions.

Comment 5

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

That's a good idea - my only concern with that is that this code has been optimized to avoid making too many UI changes, because in the past updating these indicators has had a significant impact on page load time (either because we do too much work when the security state changes, or because we get too many redundant "security state changed" notifications, or both).

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 523174 [details] [diff] [review]
patch

Review of attachment 523174 [details] [diff] [review]:

::: browser/base/content/browser.js
@@ +7634,1 @@
                                                     (this._lastLocation.port || 443),

I was pondering if .port can ever be 0 here (bug 347934 not withstanding), and then noticed you're not actually changing this. :) I'd be a bit surprised if a 0 can make it to here, though.

Also I just wanted to test Splinter.

Oh, yay, I can't change review flags here. :/ Oh, wait, I can. Mode-based interfaces FTL. :(

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/e213e8161a89