New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 133176 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Security: Chrome incorrectly displays the protocol

Project Member Reported by batz@google.com, Jun 16 2012

Issue description

This template is ONLY for reporting security bugs. Please use a different
template for other types of bug reports.

Please see the following link for instructions on filing security bugs:
http://www.chromium.org/Home/chromium-security/reporting-security-bugs


VULNERABILITY DETAILS
Please provide a brief explanation of the security issue.

VERSION
Chrome Version: AFAIK all
Operating System: all

REPRODUCTION CASE
1) open https://b.corp.google.com (authenticate if needed)
2) Clear the b.corp.google.com HSTS setting (see chrome://net-internals/#hsts)
3) open http://b.corp.google.com:443/home

3 loads using the SPDY socket.  The browser issues a valid HTTP request so this may cause non-secure cookies to be set.  The browser also shows HTTP in the URL.

It almost appears Chrome used the FE as a SPDY forward proxy...
 

Comment 1 by jsc...@chromium.org, Jun 18 2012

Labels: -Area-Undefined Area-Internals Internals-Network

Comment 2 by jsc...@chromium.org, Jun 18 2012

Cc: rch@chromium.org
Labels: Internals-Network-SPDY Internals-Network-SSL
Can this happen in a way which does not require manual changes to the HSTS settings?
Cc: willchan@chromium.org
Also - looks like SPDY session logic does not take into account scheme when deciding whether it should be part of a session. 

I believe the HSTS logic is treated like a redirect to https without requiring server round trip, so if it wasn't cleared this should work correctly.

Comment 5 by rch@chromium.org, Jun 18 2012

Yes, the SpdySessionPool uses host:port as the key, not scheme:host:port.  It could be changed to add scheme, I suppose, though it's worth noting that with Alternate-Protocol we can legitimately send HTTP requests over an existing SPDY session that was used for HTTPS requests.  (Though that is not the case here).

That being said, what is the security aspect of this bug?

Comment 6 by rch@chromium.org, Jun 18 2012

After talking offline with Julien from the security team, the security related problem is as follows.  Assume that an attacker has network level control of the client.  Assuming that the client has visited https://foo/ over SPDY, the attacker causes the user to visit http://foo:443/.  Because of the Chrome bug, the request is sent over the existing SPDY session.  The server might be confused in this case.  It might have been written to never expect HTTP requests over this port, and might issue a valid login cookie without the "secure" attribute.  The next time the user visited http://foo/ (note HTTP and no port), this login cookie would be sent in the clear.  This causes the credentials to be sent in the clear, and hence read by the attacker.

This does seem problematic, though of course, this scenario also requires a confused server.  (I sure don't think this is P0, though)

Comment 7 by jar@chromium.org, Jun 18 2012

+1 comment 6.  I do not understand why this is rated as a P0 bug client side, when it relies on a server being written poorly, and sending secure credentials to an http request.

Comment 8 by jsc...@chromium.org, Jun 18 2012

The security template defaults to pri-0. Until severity and impact is assigned, it just means the bug is still untriaged. The template has always been this way, although if it's that confusing to people I suppose it can be changed.
Labels: -Pri-0 -Internals-Network-SSL Pri-1 Mstone-22
Dropping priority on it (or is this for security team to do?)

Seems like a good idea to incorporate scheme in the SpdySession logic.

For Alternate-Protocol, have there been any thoughts about treating like a fake redirect similar to HSTS?
I agree, this is not P0. And we do want to support http:// over spdy via Alternate-Protocol, unless that's totally unworkable due to security concerns. No, a fake redirect does not work, since it results in suboptimal performance. The problem is if you connect via HTTP first, and then get an Alternate-Protocol response header, you still have this HTTP connection (perhaps multiple) that you can probably reuse if they're persistent. Blocking the subsequent requests on establishing a TLS connection for npn-spdy would slow down page load. Therefore, we race.

I think this bug is fixable by not allowing http:// over SPDY except via Alternate-Protocol. That should not be hard to do.
I doubt it's a pri-1 either. The sheriff is busy triaging the bugs from the weekend, so he'll set the appropriate flags shortly.

Comment 12 by rch@chromium.org, Jun 18 2012

> I think this bug is fixable by not allowing http:// over SPDY except via Alternate-Protocol. That should not be hard to do.

We also need http over SPDY in the "HTTPS Proxy" case (SPDY Proxy).  I seem to recall that the flag --use-spdy=no-ssl also would result in http over SPDY.  Is that right?
I neglected to mention that we should also respect the various command line flags. The SPDY proxy is a good point too. This isn't hard, just a SMOP. The harder / more annoying thing is to add appropriate tests to make sure we don't break the various configurations that we don't think about as much.

Comment 14 by rch@chromium.org, Jun 18 2012

Ok, so when we attempt to fetch a SPDY session from the pool, we will first check that the key matches (host, port, proxy).  Then, we will further check that at least on of the following is true:

* the URL is HTTPS
* Alternate-Protocol is being used
* the proxy is not DIRECT (i.e. it's a SPDY Proxy)
* the force spdy flag is set

Am I missing anything?

(I'll cook up a CL for this)
Does that seem like the right logic?

Comment 15 by cdn@chromium.org, Jun 18 2012

Labels: -Pri-1 Pri-3 SecSeverity-Low SecImpacts-Stable SecImpacts-Beta
Status: Available

Comment 16 by cdn@chromium.org, Jun 18 2012

Labels: OS-All
Seems like low severity. Does someone want to own it?
Owner: rch@chromium.org
Status: Assigned
Seems like rch is already on it.

Comment 18 by rch@chromium.org, Jun 18 2012

Actually, in thinking about this a bit more, if the attacker has control of the network, then they can force an Alternate-Protocol header to be set on requests to http://foo/ which would cause subsequent HTTP requests for http://foo/ to be sent over the existing SPDY connection.  So even once we fix this bug, the attacker can still cause the problems outlined in this report.  As such, I think we should fix the bug (working on it) but also remove the sec* labels from this bug.  Thoughts?
I agree.

Comment 20 by rch@chromium.org, Jun 19 2012

Labels: -Restrict-View-SecurityTeam -Type-Security -SecSeverity-Low -SecImpacts-Stable -SecImpacts-Beta Type-Bug

Comment 21 by rch@chromium.org, Jun 19 2012

Status: Started
Project Member

Comment 22 by bugdroid1@chromium.org, Jun 20 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=143290

------------------------------------------------------------------------
r143290 | gab@chromium.org | Wed Jun 20 16:13:34 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.cc?r1=143290&r2=143289&pathrev=143290
 M http://src.chromium.org/viewvc/chrome/trunk/src/base/win/win_util.cc?r1=143290&r2=143289&pathrev=143290

Replace all spaces by '_' in the AppUserModelId

Adjust DCHECKs in base::win::SetAppIdForPropertyStore() to reflect the new appid standard which requires less than 64 characters, not 128 (as of Windows 8, see Metro guidelines for details).

BUG= 133176 
TEST=Chrome installs, launches, and multi-profile OS integration works for user-level installs (on a user with a space in username).

Review URL: https://chromiumcodereview.appspot.com/10559097
------------------------------------------------------------------------

Comment 23 by gab@chromium.org, Jun 21 2012

My bad, wrong bug...
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 21 2012

Labels: merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=143430

------------------------------------------------------------------------
r143430 | karen@chromium.org | Thu Jun 21 12:23:11 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/installer/util/shell_util.cc?r1=143430&r2=143429&pathrev=143430
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/base/win/win_util.cc?r1=143430&r2=143429&pathrev=143430

Merge 143290 - Replace all spaces by '_' in the AppUserModelId

Adjust DCHECKs in base::win::SetAppIdForPropertyStore() to reflect the new appid standard which requires less than 64 characters, not 128 (as of Windows 8, see Metro guidelines for details).

BUG= 133176 
TEST=Chrome installs, launches, and multi-profile OS integration works for user-level installs (on a user with a space in username).

Review URL: https://chromiumcodereview.appspot.com/10559097

TBR=gab@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10643004
------------------------------------------------------------------------
Project Member

Comment 25 by bugdroid1@chromium.org, Jul 2 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=145172

------------------------------------------------------------------------
r145172 | rch@chromium.org | Mon Jul 02 12:14:04 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_transaction_spdy2_unittest.cc?r1=145172&r2=145171&pathrev=145172
 M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_transaction_spdy3_unittest.cc?r1=145172&r2=145171&pathrev=145172
 M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory_impl_job.cc?r1=145172&r2=145171&pathrev=145172
 M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_stream_factory_impl_job.h?r1=145172&r2=145171&pathrev=145172

Prevent requests for http://host:port/ from being sent over an existing spdy session to https://host:port/.

BUG= 133176 
TEST=HttpNetworkTransactionSpdy\*Test.\*UseSpdySessionForHttp\*


Review URL: https://chromiumcodereview.appspot.com/10581020
------------------------------------------------------------------------

Comment 26 by rch@chromium.org, Jul 13 2012

Status: Fixed
Project Member

Comment 27 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 28 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -Internals-Network -Internals-Network-SPDY -Mstone-22 M-22 Cr-Internals Cr-Internals-Network Cr-Internals-Network-SPDY
Project Member

Comment 29 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Cr-Internals-Network-SPDY Cr-Internals-Network-HTTP2
Migrate from Cr-Internals-Network-SPDY to Cr-Internals-Network-HTTP2

Sign in to add a comment