Give more request types a TransportSecurityState.

Give more request types a TransportSecurityState.

DCHECK on NULL TransportSecurityState, as a precursor to a real CHECK. It
should be an error to try to connect with an SSL client socket without
having a live TSS.

BUG=246724
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206013

[palmer, 2013-06-05 22:22:03]

Is this along the lines of how I should proceed?

[Ryan Sleevi, 2013-06-05 22:54:48]

Yup, looks like you're on the right path.

I've added raymes/ for the Pepper stuff.

Raymes, this has the effect of ensuring that any SSL socket requests from the
Pepper layers will use Chrome's hardcoded list of pins (but not necessarily any
pins the user has observed). This is Real Good Security(tm), but I'm curious if
you'd have any objections.

[Ryan Sleevi, 2013-06-05 22:56:00]

net/ LGTM, mod nits.

https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (left):

https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket....
net/socket/ssl_client_socket.h:22: class TransportSecurityState;
This change shouldn't be necessary. It's all forward declares here in the .h

https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_unittest.cc:24: #include
"net/ssl/ssl_config_service.h"
Add the transport_security_state.h dependency here.

[raymes, 2013-06-05 23:09:23]

What impact could this have on existing users of SSL in pepper?

On Wed, Jun 5, 2013 at 3:56 PM,  <rsleevi@chromium.org> wrote:
> net/ LGTM, mod nits.
>
>
>
https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket.h
> File net/socket/ssl_client_socket.h (left):
>
>
https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket....
> net/socket/ssl_client_socket.h:22: class TransportSecurityState;
> This change shouldn't be necessary. It's all forward declares here in
> the .h
>
>
https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket_...
> File net/socket/ssl_client_socket_unittest.cc (right):
>
>
https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket_...
> net/socket/ssl_client_socket_unittest.cc:24: #include
> "net/ssl/ssl_config_service.h"
> Add the transport_security_state.h dependency here.
>
> https://codereview.chromium.org/16501002/

[Ryan Sleevi, 2013-06-05 23:17:25]

On 2013/06/05 23:09:23, raymes wrote:
> What impact could this have on existing users of SSL in pepper?
> 

They match the rest of the Chrome networking stack, and get protection from
hostile CAs trying to MITM connections to preloaded pinned sites? :)

I'm not sure what the existing users of SSL in Pepper are, so it's hard to weigh
impact.

Today, they have no pinning protection.

[raymes, 2013-06-05 23:35:18]

Sorry I should have mentioned. The existing users that I know of are
Chromoting and Flash.

It sounds like the only people who might break are those who are
victims of an attack though. So lgtm.

Thanks!

On Wed, Jun 5, 2013 at 4:17 PM,  <rsleevi@chromium.org> wrote:
> On 2013/06/05 23:09:23, raymes wrote:
>>
>> What impact could this have on existing users of SSL in pepper?
>
>
>
> They match the rest of the Chrome networking stack, and get protection from
> hostile CAs trying to MITM connections to preloaded pinned sites? :)
>
> I'm not sure what the existing users of SSL in Pepper are, so it's hard to
> weigh
> impact.
>
> Today, they have no pinning protection.
>
>
> https://codereview.chromium.org/16501002/

[palmer, 2013-06-05 23:50:15]

https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (left):

https://codereview.chromium.org/16501002/diff/1/net/socket/ssl_client_socket....
net/socket/ssl_client_socket.h:22: class TransportSecurityState;
On 2013/06/05 22:56:00, Ryan Sleevi wrote:
> This change shouldn't be necessary. It's all forward declares here in the .h

Done.

[palmer, 2013-06-07 01:24:23]

Adding some more people for OWNERS review:

derat: chrome/browser/chromeos/web_socket_proxy.cc
cbentzel, agl: chrome/browser/net
jochen: content/shell
ajwong: jingle
sergeyu: remoting
tkent: webkit/support

Thank you!

[Daniel Erat, 2013-06-07 01:38:17]

LGTM for chrome/browser/chromeos/web_socket_proxy.cc

[tkent, 2013-06-07 02:33:45]

lgtm for webkit/support/

[jochen (gone - plz use gerrit), 2013-06-07 10:05:08]

lgtm

[agl, 2013-06-07 15:20:17]

chrome/browser/net: LGTM

[awong, 2013-06-07 15:50:40]

https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
File jingle/glue/proxy_resolving_client_socket.cc (left):

https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
jingle/glue/proxy_resolving_client_socket.cc:59: // transport_security_state is
NULL because it's not thread safe.
This comment concerns me.

I think Sergey wrote this code so he would be better suited to review it.

[palmer, 2013-06-07 16:35:06]

https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
File jingle/glue/proxy_resolving_client_socket.cc (left):

https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
jingle/glue/proxy_resolving_client_socket.cc:59: // transport_security_state is
NULL because it's not thread safe.
On 2013/06/07 15:50:40, awong wrote:
> This comment concerns me.
> 
> I think Sergey wrote this code so he would be better suited to review it.

git blame says agl wrote it, but I am not sure what he meant in this context.
agl?

[palmer, 2013-06-11 17:54:15]

On 2013/06/07 16:35:06, Chromium Palmer wrote:
>
https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
> File jingle/glue/proxy_resolving_client_socket.cc (left):
> 
>
https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
> jingle/glue/proxy_resolving_client_socket.cc:59: // transport_security_state
is
> NULL because it's not thread safe.
> On 2013/06/07 15:50:40, awong wrote:
> > This comment concerns me.
> > 
> > I think Sergey wrote this code so he would be better suited to review it.
> 
> git blame says agl wrote it, but I am not sure what he meant in this context.
> agl?

Friendly ping? What's a good way to proceed on this CL?

[Sergey Ulanov, 2013-06-11 19:32:29]

jingle/glue - lgtm, but I have a question about remoting

https://codereview.chromium.org/16501002/diff/41001/remoting/protocol/ssl_hma...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/16501002/diff/41001/remoting/protocol/ssl_hma...
remoting/protocol/ssl_hmac_channel_authenticator.cc:105:
context.transport_security_state = transport_security_state_.get();
Do we really need to set it here? Chromoting always uses self-signed certs for
P2P connection, and it's not an HTTP connection, so TransportSecurityState
doesn't seem to be useful here.

[agl, 2013-06-11 19:42:28]

https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
File jingle/glue/proxy_resolving_client_socket.cc (left):

https://codereview.chromium.org/16501002/diff/29001/jingle/glue/proxy_resolvi...
jingle/glue/proxy_resolving_client_socket.cc:59: // transport_security_state is
NULL because it's not thread safe.
On 2013/06/07 16:35:07, Chromium Palmer wrote:
> git blame says agl wrote it, but I am not sure what he meant in this context.
> agl?

You have to summon me in a more forceful manner than that :)

I think I was worried that this code might be running on a different thread and,
since it doesn't do real validations anyway, I just set it to NULL.

[palmer, 2013-06-11 21:22:00]

> You have to summon me in a more forceful manner than that :)

"BY THE POWER OF GRAYSKULL..."

> I think I was worried that this code might be running on a different thread
and,
> since it doesn't do real validations anyway, I just set it to NULL.

This answers sergeyu's question too: rsleevi and I hope to make passing a NULL
TSS an error. (A non-NULL TSS shouldn't interfere with sergeyu's case, and we
want to make sure that in the future it is not possible to create request
contexts that could be MITM'd or are un-pinned/un-STS'd.)

[palmer, 2013-06-11 23:17:32]

To ensure that people don't call into TSS from more than one thread, I added
DCHECK(CalledOnValidThread()) in all the non-static member functions. Many such
functions already had the DCHECK, but some did not! Locally, I don't seem to
notice any DCHECKs getting hit, but we'll see what the trybots say.

[Sergey Ulanov, 2013-06-11 23:18:03]

On 2013/06/11 21:22:00, Chromium Palmer wrote:
> > You have to summon me in a more forceful manner than that :)
> 
> "BY THE POWER OF GRAYSKULL..."
> 
> > I think I was worried that this code might be running on a different thread
> and,
> > since it doesn't do real validations anyway, I just set it to NULL.
> 
> This answers sergeyu's question too: rsleevi and I hope to make passing a NULL
> TSS an error. (A non-NULL TSS shouldn't interfere with sergeyu's case, and we
> want to make sure that in the future it is not possible to create request
> contexts that could be MITM'd or are un-pinned/un-STS'd.)

Makes sense. remoting - LGTM

[Sergey Ulanov, 2013-06-11 23:25:57]

On Tue, Jun 11, 2013 at 4:17 PM, <palmer@chromium.org> wrote:

> To ensure that people don't call into TSS from more than one thread, I
> added
> DCHECK(CalledOnValidThread()) in all the non-static member functions. Many
> such
> functions already had the DCHECK, but some did not! Locally, I don't seem
> to
> notice any DCHECKs getting hit, but we'll see what the trybots say.
>

The code in question (jingle/glue) is used only for chromoting host and
sync, so you would not detect possible issues unless you enable sync or try
running chromoting host. There should be no issues with chromoting (the
socket is always used on the same thread on which TSS is created). I'm not
completely sure about sync (and I'm not sure if that code is still used by
sync at all) - akalin@ should know more about it, but you can also verify
if there are any issues by enabling sync in Debug builds (but I think you
would need to setup service keys for that)




>
>
https://codereview.chromium.**org/16501002/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-06-12 19:00:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/16501002/56001

[commit-bot: I haz the power, 2013-06-13 02:50:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/16501002/56001

[commit-bot: I haz the power, 2013-06-13 06:48:13]

Change committed as 206013