Bug 521461 (CVE-2009-3984)

SSL spoofing by setting location.href to SSL page with 204 (No Content) response

RESOLVED FIXED

Status

Core Graveyard
Security: UI
P2
normal
RESOLVED FIXED
8 years ago
7 months ago

People

(Reporter: bsterne, Assigned: mayhemer)

Tracking

({verified1.9.0.16, verified1.9.1})

Trunk
verified1.9.0.16, verified1.9.1
Bug Flags:
blocking1.9.2 +
blocking1.9.0.16 +
wanted1.9.0.x +
wanted1.8.1.x +

Firefox Tracking Flags

(blocking2.0 alpha1+, status1.9.2 beta1-fixed, blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [sg:moderate])

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
This bug was reported to security@m.o by Jonathan Morgan:

I moved his PoC code into the attached testcase which requires being signed in to a Google account to get the requisite 204 response from https://www.google.com/calendar/perf.  Note the SSL indicators appear when viewing the testcase over http:  as well as file:.

From the email:

-----
If a secure (https) page returns HTTP status 204 (no content), then the current page will show a verified security certificate for the current page.  One such page is part of Google Calendar, which will give users a verified certificate for the site verified by Thawte. Quite a lot of people are perpetually signed in to their Google account, meaning that adding the following script to your site would make it appear verified and secure to any user who is currently signed into the Google account (even if it was an http site, not an https site).
-----
(Reporter)

Comment 1

8 years ago
Created attachment 405542 [details]
testcase

spoofs SSL badging (load via http: or file:)
(Reporter)

Comment 2

8 years ago
I noticed that when you load the testcase from file:, Larry starts out with blank, but valid, SSL info.  If you switch tabs to another SSL page and back, the SSL info from the last tab stays populated in the Larry dialog for the spoofed page.
blocking1.9.1: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
Flags: wanted1.9.0.x?
Flags: blocking1.9.2?
Flags: blocking1.9.0.16?
Yeah, the UI is getting the wrong info from onSecurityChange - cc'ng people who have looked at that recently. We should fix this.
Similar to bug 514232, do they have the same underlying cause in the UI code?
I doubt it; in this case the docshell URI never changes.  I'm not sure when the UI code gets onSecurityChange here or why....
(Assignee)

Comment 6

8 years ago
-> me
Assignee: kaie → honzab.moz
Status: NEW → ASSIGNED
(Assignee)

Comment 7

8 years ago
BTW: Could anyone cc me please to bug 514232? Thanks.
(Assignee)

Comment 8

8 years ago
Boris, looks like we broke this in bug 480713, 204 responses notifies STATE_TRANSFERRING that makes security UI take it into account and be driven by it as the request is also flagged as a document load (top level) which is correct. See http://hg.mozilla.org/mozilla-central/annotate/dbebe1771118/uriloader/base/nsDocLoader.cpp#l630. This patch https://bugzilla.mozilla.org/show_bug.cgi?id=329869#c11 is causing it.

204/205 responses should not change the document view (what correctly as spec'ed by RFC 2616 happens). One idea is that those should be taken by the security UI as subrequests. The other one is to not set the transferring flag in case of 204/205 responses as a quick fix, but then a new tab loading 204 will never have a ssl ui. However, the way security UI treats these request must change anyway.
We should probably just make security UI ignore 204/205 responses for document loads.
(Assignee)

Comment 10

8 years ago
Created attachment 405898 [details] [diff] [review]
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]

Doing location = "http://..." from a secure page doesn't drop the security state. Question is if it has to or not as we would like to tell the user prior to making the request, bug 62178. If not then this is a sufficient patch.
Attachment #405898 - Flags: review?(bzbarsky)
How does imagelib (say) handle 204/205 responses with a body?  Or CSSLoader, or scriptloader?  How does this patch affect such responses when they're not the document request, if at all?
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> How does imagelib (say) handle 204/205 responses with a body? 
> Or CSSLoader, or
> scriptloader?  

Body is ignored on http transaction level: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpTransaction.cpp#857

>How does this patch affect such responses when they're not the
> document request, if at all?

It doesn't affect them at all. isToplevelProgress is checked (used for a condition) always with nsIChannel::LOAD_DOCUMENT_URI that cannot be set for anything other then a document.
Comment on attachment 405898 [details] [diff] [review]
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]

> Body is ignored on http transaction level:

Ah, excellent.

r=bzbarsky
Attachment #405898 - Flags: review?(bzbarsky) → review+
blocking1.9.1: ? → .5+
status1.9.1: ? → wanted
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.16+
Whiteboard: [sg:moderate]
(Assignee)

Comment 14

8 years ago
Comment on attachment 405898 [details] [diff] [review]
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]

http://hg.mozilla.org/mozilla-central/rev/0eb9fe3674d5
Attachment #405898 - Attachment description: v1 → v1 [Checkin comment 14]
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Whiteboard: [sg:moderate] → [sg:moderate][baking since Oct-16][needs-192-landing]
(Assignee)

Comment 15

8 years ago
Comment on attachment 405898 [details] [diff] [review]
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ddb2cb8d1d51
Attachment #405898 - Attachment description: v1 [Checkin comment 14] → v1 [Checkin comment 14] [Checkin 1.9.2 comment 15]
(Assignee)

Updated

8 years ago
Whiteboard: [sg:moderate][baking since Oct-16][needs-192-landing] → [sg:moderate][baking since Oct-16]
(Assignee)

Comment 16

8 years ago
Comment on attachment 405898 [details] [diff] [review]
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1369259affe9
Attachment #405898 - Attachment description: v1 [Checkin comment 14] [Checkin 1.9.2 comment 15] → v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]
(Assignee)

Comment 17

8 years ago
Created attachment 407003 [details] [diff] [review]
v1 merge for 1.9.0 [Checkin 1.9.0 comment 17]

/cvsroot/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,v  <--  nsSecureBrowserUIImpl.cpp
new revision: 1.75; previous revision: 1.74
(Assignee)

Updated

8 years ago
status1.9.1: wanted → .5-fixed
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.0.16
Whiteboard: [sg:moderate][baking since Oct-16] → [sg:moderate]
Attachment #405898 - Flags: approval1.9.1.5?
Attachment #407003 - Flags: approval1.9.0.16?
Uh, why did the 1.9.1 and 1.9.0 patches land without approval? blocking+ does not give you permission to land on anything <1.9.2. Explicit approval is required, as per the rules listed on tinderbox.
(Assignee)

Comment 19

8 years ago
(In reply to comment #18)
> Uh, why did the 1.9.1 and 1.9.0 patches land without approval? blocking+ does
> not give you permission to land on anything <1.9.2. Explicit approval is
> required, as per the rules listed on tinderbox.

Ups, someone in the past told me that blocking flag gives that permission. Didn't know this has changed.
Nothing has changed. Maintenance branches always require explicit approval, as has been listed on the tinderbox pages for every maintenance branch for years.
Comment on attachment 405898 [details] [diff] [review]
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16]

Approved for 1.9.1.5, a=dveditz for release-drivers
Attachment #405898 - Flags: approval1.9.1.5? → approval1.9.1.5+
Comment on attachment 407003 [details] [diff] [review]
v1 merge for 1.9.0 [Checkin 1.9.0 comment 17]

Approved for 1.9.0.16, a=dveditz for release-drivers
Attachment #407003 - Flags: approval1.9.0.16? → approval1.9.0.16+
Running the attached testcase in 1.9.1.5 and in a 1.9.1.6 nightly build, I receive a page that states "This is not a secure page." 

I notice that if I explicitly load http://www.google.com/calendar/render?tab=yc first and then visit the testcase, in 1.9.1.6, the lock icon shows an alert and the detailed view shows that there is unencrypted content. This does not *show* in 1.9.1.5. This only happens if I explicitly load Google Calendar first. If I just load the testcase, I see no visible difference between 1.9.1.5 and 1.9.1.6. In *both* instances, I was already signed into Google before running any tests.

Looking at the bug history, it isn't clear to me whether we shipping this fixed in 1.9.1.5 because of the short cycle on the release. Is this message and behavior the correct ones for post-fix? It seems unlikely.
We did not fix this in 1.9.1.5.
Which then begs the question about the behavior I'm seeing.
(Reporter)

Comment 26

8 years ago
This is kind of a weird bug to verify.  The testcase first needs to be loaded as either a local file: resource or moved to a http: (not secure) server in order to be a valid test.  The testcase, in unfixed versions, spoofs SSL indicators even though it is loaded as an insecure resource.  The only reason the Google URL is being used is it responds with the 204 and empty response body.  To get the 204, you also need to be logged in to a Google account, otherwise they redirect you to the login page which breaks the testcase.  So to verify:

1. Log in to a Google account
2. Download testcase as local file: or put on http: server
3. Load the testcase
   a. if SSL indicators present -> broken
   b. if no SSL indicators -> correct
Duplicate of this bug: 528688
Running this locally, I get a lock icon in 1.9.0.15 but not 1.9.0.16 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729)).

In 1.9.1.5 and 1.9.1.6 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)), I see the same behavior.

So this is fixed for 1.9.0.16 and 1.9.1.6.
Keywords: fixed1.9.0.16 → verified1.9.0.16, verified1.9.1

Comment 29

8 years ago
Created attachment 415093 [details] [diff] [review]
1.8.0 version

window.get() -> mWindow.get() change
Alias: CVE-2009-3984
Marking the 14 bugs that are both:
 * nominated for blocking1.9.3:?
 * fixed on the 1.9.2 branch (according to status1.9.2)
as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually.  They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.
blocking2.0: ? → alpha1
Group: core-security
Flags: wanted1.8.1.x+
(Assignee)

Updated

7 years ago
Attachment #415093 - Flags: approval1.8.1.next?
(Assignee)

Updated

6 years ago
Attachment #415093 - Flags: approval1.8.1.next?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.