Reject renegotiations in SSLClientSocket by default.

Reject renegotiations in SSLClientSocket by default.

Only HTTP/1.1 (and below) sockets may renegotiate. This fix a
crash because SpdyHttpStream didn't account for this properly.
(And can't as the renego + client auth hack is inherently
incompatible with multiplexing.)

Tested manually against hacked up Go servers:

- HTTP/1.1 server which renegotiates with client auth before
  sending a response on a fresh socket.

- Same as above but with a reused socket (the server only
  requests renego when fetching /auth).

- HTTP/2 which incorrectly renegotiates with client auth upon
  requesting /auth. Verified that we get ERR_SSL_PROTOCOL_ERROR
  and not crash.

- HTTP/1.1 server which does two handshakes in a row with Finished
  and HelloRequest in the same record. NSS and BoringSSL differ in
  their behavior here, but in neither port should we miss the
  renego.

BUG=484543, 462283
Committed: https://crrev.com/421116c22292293f78c6ab15c7a8d6ca2fc1b68b
Cr-Commit-Position: refs/heads/master@{#329466}

[davidben, 2015-05-06 23:30:18]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2015-05-06 23:30:18]

Unfortunately, my answer to the inevitable "tests?" is a very big sad face.
tlslite doesn't do renego and it'll probably be a nightmare and a half to add
support for it. To say nothing of convincing testserver.py to use it with the
client auth hack... or HTTP/2.

What I do have is a pile of Go code locally which does this for both HTTP/1.1
and HTTP/2 servers. (Fork of BoringSSL's fork of crypto/tls because we added
renego to it a while back, plus a fork of bradfitz/http2.)

https://codereview.chromium.org/1131763002/diff/1/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/ssl3con.c (left):

https://codereview.chromium.org/1131763002/diff/1/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl3con.c:5535: ssl_GetXmitBufLock(ss);
Without removing these, SSL3_SendAlert triggers an alert.

[Ryan Sleevi, 2015-05-06 23:36:59]

Can you describe what alternatives you considered?

I don't think this is right - or, more specifically, I have trouble reasoning
that this is correct - since Renego+Connect() is historically problematic.

I understand the motivation, and while I haven't looked closely enough for the
alternatives, it might help to know what you considered and discarded as
solutions for this.

For example, why not accept them by default (the status quo), but then reject
them during connect? What are the tradeoffs there?

[davidben, 2015-05-06 23:52:28]

On 2015/05/06 23:36:59, Ryan Sleevi wrote:
> Can you describe what alternatives you considered?

TBH, I don't really see that many other options. The other is that we default to
on and consumers need to turn it off. I think they're about equally potentially
problematic (below) and I much much prefer this default (below).

> I don't think this is right - or, more specifically, I have trouble reasoning
> that this is correct - since Renego+Connect() is historically problematic.

Do you mean that we might race with setting the toggle and the peer sending us
HelloRequest, or something else? This does assume we don't further pump the SSL
state machine until you start calling Read() or Write(). But defaulting to on
makes the same assumption. Instead of accidentally rejecting a renego, we'd
accidentally accept one.

We could maybe add DCHECKs or some more interesting delegate-ish API to ensure
that we only make a decision just after Connect() and before Read(). I don't
think we'll need to make a decision at other points. The latter sounds kind of
hairy to plumb through though.

> I understand the motivation, and while I haven't looked closely enough for the
> alternatives, it might help to know what you considered and discarded as
> solutions for this.
> 
> For example, why not accept them by default (the status quo), but then reject
> them during connect? What are the tradeoffs there?

We can't make a decision until after Connect() is done because we don't know
HTTP/1.1 vs HTTP/2. So either way it has to be some kind of post-creation
toggle. So the main question is which is default.

Renego is completely bonkers and I think it's rather important that we make it
opt-in rather than opt-out for the archetypal random unknown SSLClientSocket
consumer outside of //net. (After this lands and Chromium isn't sensitive to
which is default, I'd like to BoringSSL itself default to rejecting renego.) For
instance, ExportKeyingMaterial has extremely unclear semantics if
SSLClientSocket transparently renegos behind your back. At any point you call
it, you may be in a weird state.)


One thing that is awkward about this API is that you can't call it before
Connect has started either. I'm not super-happy about that one. I'd need to add
a boolean somewhere so that it can be set before ssl_ and nss_fd_ are
initialized. It didn't seem terribly worth the effort for that, but I could do
that.

[Ryan Sleevi, 2015-05-07 01:41:26]

OK, sounds reasonable, but I'm concerned about places where it's supposed to
work that this will now break. For example, in this CL, it looks like this
breaks WebSockets ;(

https://codereview.chromium.org/1131763002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1131763002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1123:
connection_->socket()->SetRenegotiationsAllowed(true);
This is also needed for WebSockets, which can and do use client certs

https://codereview.chromium.org/1131763002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1131763002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1126: // position to prevent renegotiation
attacks.
Word it w/o pronouns :)

Also, this comment doesn't make sense with lines 1128 - 1130. You set
SSL_RENEGOTIATE_NEVER but then you say in the comment prohibiting it is a waste
of time?

https://codereview.chromium.org/1131763002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1127: // http://extendedsubset.com/?p=8
Bad link? Doesn't load here

https://codereview.chromium.org/1131763002/diff/1/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1131763002/diff/1/net/socket/stream_socket.h#...
net/socket/stream_socket.h:102: virtual void SetRenegotiationsAllowed(bool
allowed) {}
= 0

https://codereview.chromium.org/1131763002/diff/1/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/ssl3con.c (left):

https://codereview.chromium.org/1131763002/diff/1/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl3con.c:5535: ssl_GetXmitBufLock(ss);
On 2015/05/06 23:30:18, David Benjamin wrote:
> Without removing these, SSL3_SendAlert triggers an alert.

Let's do this in a separate CL. At the least, I want to see an upstream bug
before we land this patch. It is correct though (because SSL3_SendAlert grabs
these locks)

[davidben, 2015-05-07 19:12:35]

https://codereview.chromium.org/1131763002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1131763002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1123:
connection_->socket()->SetRenegotiationsAllowed(true);
On 2015/05/07 01:41:26, Ryan Sleevi wrote:
> This is also needed for WebSockets, which can and do use client certs

Well, this doesn't break all client certs. Sane deployments (relatively
speaking) of client certs don't do the awful renego hack. :-) But you're right.
It should be up at the top of this block, in case someone does the renego hack
on the WebSockets entry point. Fixed.

https://codereview.chromium.org/1131763002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1131763002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1126: // position to prevent renegotiation
attacks.
On 2015/05/07 01:41:26, Ryan Sleevi wrote:
> Word it w/o pronouns :)
> 
> Also, this comment doesn't make sense with lines 1128 - 1130. You set
> SSL_RENEGOTIATE_NEVER but then you say in the comment prohibiting it is a
waste
> of time?

I just copied it from the other one. I think it's explaining why we do
transition vs requiring renegotiation info? I've just removed it because the
link is dead and I'm not sure exactly what it's saying.

https://codereview.chromium.org/1131763002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1127: // http://extendedsubset.com/?p=8
On 2015/05/07 01:41:26, Ryan Sleevi wrote:
> Bad link? Doesn't load here

Removed.

https://codereview.chromium.org/1131763002/diff/1/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/1131763002/diff/1/net/socket/stream_socket.h#...
net/socket/stream_socket.h:102: virtual void SetRenegotiationsAllowed(bool
allowed) {}
On 2015/05/07 01:41:26, Ryan Sleevi wrote:
> = 0

Fiiiine. Well, you get to review it. FWIW, EnableTCPFastOpenIfSupported is also
a {}, probably for similar reasons.

https://codereview.chromium.org/1131763002/diff/1/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/ssl3con.c (left):

https://codereview.chromium.org/1131763002/diff/1/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl3con.c:5535: ssl_GetXmitBufLock(ss);
On 2015/05/07 01:41:26, Ryan Sleevi wrote:
> On 2015/05/06 23:30:18, David Benjamin wrote:
> > Without removing these, SSL3_SendAlert triggers an alert.
> 
> Let's do this in a separate CL. At the least, I want to see an upstream bug
> before we land this patch. It is correct though (because SSL3_SendAlert grabs
> these locks)

Done. https://codereview.chromium.org/1134493002/

[davidben, 2015-05-08 21:33:00]

Updated with new API per discussion out-of-band.

[davidben, 2015-05-08 21:34:29]

https://codereview.chromium.org/1131763002/diff/60001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1131763002/diff/60001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:917: if (rv == 1) {
This function's getting slightly unwieldy. I'll refactor it into a
DoHandshakeComplete and such in a follow-up.

[Ryan Sleevi, 2015-05-08 22:40:59]

LGTM, but please don't CQ right away.

Can you make sure the commit note describes each of the scenarios you manually
tested using your go test harness? I want to make sure we've documented the
cases we're testing and care about, so that it's also easier to come back to if
we forgot a test.

[davidben, 2015-05-09 00:19:25]

Updated description and modified the NSS side to account for the fourth case.
Assuming it's still fine, I'll double-check the cartesian product on Monday
before landing because it's late and this deserves a second go when I'm less
tired.

[davidben, 2015-05-12 00:57:20]

(No rush, but PTAL at the new NSS code, in case that was unclear.)

[Ryan Sleevi, 2015-05-12 01:04:41]

Thanks - still LGTM, go ahead and CQ

[davidben, 2015-05-12 18:41:39]

Repeated the tests manually. Still behaves as expected. CQ'ing.

[davidben, 2015-05-12 18:41:45]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-05-12 18:43:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1131763002/80001

[commit-bot: I haz the power, 2015-05-12 19:57:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-12 19:58:00]

Patchset 5 (id:??) landed as
https://crrev.com/421116c22292293f78c6ab15c7a8d6ca2fc1b68b
Cr-Commit-Position: refs/heads/master@{#329466}