Closed Bug 645564 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Security, defect)

4.0 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 6
Tracking Status
blocking2.0 --- -
status2.0 --- wanted
status1.9.2 --- wanted
status1.9.1 --- wanted

People

(Reporter: matt, Assigned: Gavin)

References

()

Details

(Whiteboard: [sg:moderate] spoof)

Attachments

(1 file)

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.
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.
I filed bug 646690 on the general issue of location.hostname not being useful for view-source pages.
Attached patch patchSplinter Review
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).
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #523174 - Flags: review?(dolske)
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Whiteboard: [sg:moderate] spoof
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.
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).
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
blocking2.0: ? → -
status2.0: --- → wanted
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. :(
Attachment #523174 - Flags: review?(dolske) → review+
http://hg.mozilla.org/mozilla-central/rev/e213e8161a89
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
Flags: in-testsuite+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.