Last Comment Bug 521461 - (CVE-2009-3984) SSL spoofing by setting location.href to SSL page with 204 (No Content) response
(CVE-2009-3984)
: SSL spoofing by setting location.href to SSL page with 204 (No Content) response
Status: RESOLVED FIXED
[sg:moderate]
: verified1.9.0.16, verified1.9.1
Product: Core Graveyard
Classification: Graveyard
Component: Security: UI (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
:
Mentors:
: 528688 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-09 12:09 PDT by Brandon Sterne (:bsterne)
Modified: 2016-09-27 13:03 PDT (History)
12 users (show)
bugzilla: blocking1.9.2+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
alpha1+
beta1-fixed
.6+
.6-fixed


Attachments
testcase (170 bytes, text/html)
2009-10-09 12:11 PDT, Brandon Sterne (:bsterne)
no flags Details
v1 [Checkin comment 14] [Checkin 1.9.2 comment 15][Checkin 1.9.1 comment 16] (4.20 KB, patch)
2009-10-12 12:23 PDT, Honza Bambas (:mayhemer)
bzbarsky: review+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review
v1 merge for 1.9.0 [Checkin 1.9.0 comment 17] (1.44 KB, patch)
2009-10-19 04:28 PDT, Honza Bambas (:mayhemer)
dveditz: approval1.9.0.16+
Details | Diff | Splinter Review
1.8.0 version (1.04 KB, patch)
2009-11-30 01:27 PST, Martin Stránský
no flags Details | Diff | Splinter Review

Description Brandon Sterne (:bsterne) 2009-10-09 12:09:51 PDT
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).
-----
Comment 1 Brandon Sterne (:bsterne) 2009-10-09 12:11:07 PDT
Created attachment 405542 [details]
testcase

spoofs SSL badging (load via http: or file:)
Comment 2 Brandon Sterne (:bsterne) 2009-10-09 12:16:59 PDT
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.
Comment 3 Johnathan Nightingale [:johnath] 2009-10-09 13:05:09 PDT
Yeah, the UI is getting the wrong info from onSecurityChange - cc'ng people who have looked at that recently. We should fix this.
Comment 4 Daniel Veditz [:dveditz] 2009-10-09 13:29:27 PDT
Similar to bug 514232, do they have the same underlying cause in the UI code?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2009-10-09 21:54:05 PDT
I doubt it; in this case the docshell URI never changes.  I'm not sure when the UI code gets onSecurityChange here or why....
Comment 6 Honza Bambas (:mayhemer) 2009-10-11 06:23:54 PDT
-> me
Comment 7 Honza Bambas (:mayhemer) 2009-10-11 06:24:44 PDT
BTW: Could anyone cc me please to bug 514232? Thanks.
Comment 8 Honza Bambas (:mayhemer) 2009-10-12 11:14:12 PDT
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.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2009-10-12 11:34:11 PDT
We should probably just make security UI ignore 204/205 responses for document loads.
Comment 10 Honza Bambas (:mayhemer) 2009-10-12 12:23:43 PDT
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.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2009-10-12 12:29:10 PDT
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?
Comment 12 Honza Bambas (:mayhemer) 2009-10-12 14:45:30 PDT
(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 13 Boris Zbarsky [:bz] (still a bit busy) 2009-10-12 15:30:33 PDT
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
Comment 14 Honza Bambas (:mayhemer) 2009-10-16 07:49:58 PDT
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
Comment 15 Honza Bambas (:mayhemer) 2009-10-19 03:51:07 PDT
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
Comment 16 Honza Bambas (:mayhemer) 2009-10-19 04:15:23 PDT
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
Comment 17 Honza Bambas (:mayhemer) 2009-10-19 04:28:45 PDT
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
Comment 18 Reed Loden [:reed] (use needinfo?) 2009-10-23 10:51:47 PDT
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.
Comment 19 Honza Bambas (:mayhemer) 2009-10-24 04:01:35 PDT
(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.
Comment 20 Samuel Sidler (old account; do not CC) 2009-10-24 04:58:14 PDT
Nothing has changed. Maintenance branches always require explicit approval, as has been listed on the tinderbox pages for every maintenance branch for years.
Comment 21 Daniel Veditz [:dveditz] 2009-10-26 14:18:27 PDT
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
Comment 22 Daniel Veditz [:dveditz] 2009-10-26 14:19:07 PDT
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
Comment 23 Al Billings [:abillings] 2009-11-10 16:32:35 PST
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.
Comment 24 Samuel Sidler (old account; do not CC) 2009-11-10 16:34:49 PST
We did not fix this in 1.9.1.5.
Comment 25 Al Billings [:abillings] 2009-11-10 16:36:09 PST
Which then begs the question about the behavior I'm seeing.
Comment 26 Brandon Sterne (:bsterne) 2009-11-11 11:16:15 PST
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
Comment 27 Daniel Veditz [:dveditz] 2009-11-15 00:21:24 PST
*** Bug 528688 has been marked as a duplicate of this bug. ***
Comment 28 Al Billings [:abillings] 2009-11-20 13:12:23 PST
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.
Comment 29 Martin Stránský 2009-11-30 01:27:24 PST
Created attachment 415093 [details] [diff] [review]
1.8.0 version

window.get() -> mWindow.get() change
Comment 30 David Baron :dbaron: ⌚️UTC-7 2010-01-22 14:29:14 PST
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.

Note You need to log in before you can comment on or make changes to this bug.