521461 - (CVE-2009-3984) SSL spoofing by setting location.href to SSL page with 204 (No Content) response

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Brandon Sterne (:bsterne)]

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

Attachment: testcase

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

Comment 2

[Brandon Sterne (:bsterne)]

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

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

Similar to bug 514232, do they have the same underlying cause in the UI code?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

-> me

Comment 7

[Honza Bambas (:mayhemer)]

BTW: Could anyone cc me please to bug 514232? Thanks.

Comment 8

[Honza Bambas (:mayhemer)]

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 [:bzbarsky]]

We should probably just make security UI ignore 204/205 responses for document loads.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: 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 [: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?

Comment 12

[Honza Bambas (:mayhemer)]

(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 [:bzbarsky]]

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

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

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

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

Attachment: 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]]

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

(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)]

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

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

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 - ex-MoCo]]

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

We did not fix this in 1.9.1.5.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Which then begs the question about the behavior I'm seeing.

Comment 26

[Brandon Sterne (:bsterne)]

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 28

[Al Billings [:abillings - ex-MoCo]]

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ý [:stransky] (ni? me)]

Attachment: 1.8.0 version

window.get() -> mWindow.get() change

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.