Shard the SSL session cache by version fallback.

Shard the SSL session cache by version fallback.

This addresses two issues:

- NSS clamps client_version to the session version. This means that a
  successful fallback connection is effectively cached, despite our fallback
  being stateless. This causing our metrics to be under-reported and, more
  problematic, makes spurious fallbacks stick.

- BoringSSL does not clamp, but many versions of OpenSSL on the server will
  happily resume older sessions at newer protocol versions, rather than doing a
  full handshake at the newer protocol version. This means a successful
  spurious fallback causes us later resume with a weaker cipher than we should.
  Moreover, this mismatch is forbidden by every other client implementation.
  The metrics are reporting 0.06% of connections on beta channel hit this case.
  I expect it to go down after this change.

Note: this will also increase traffic to version-intolerant servers on NSS
ports. But that's only Linux/CrOS/iOS now and the BoringSSL switch did the same
thing by losing the version clamp.

BUG=459690, 441456
Committed: https://crrev.com/21ea1b4ef8d6c111605866108c7dac1b31440114
Cr-Commit-Position: refs/heads/master@{#317605}

[davidben, 2015-02-20 22:57:55]

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

[davidben, 2015-02-20 22:57:56]

https://codereview.chromium.org/947603002/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/947603002/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2837: // attempt to resume with two
more connections.
Ideally, this test should function correctly under any combination of:

- Older versions of NSS that don't do 1.2 vs working TLS 1.2 support

- Test server resuming a TLS 1.0 session at TLS 1.2 (tlslite) vs silently doing
a full handshake when offered a TLS 1.0 session but negotiated 1.2 (what you
should be doing)

- NSS clamping client_version when offering a resumed session vs BoringSSL not
doing that because that's dumb.

- BoringSSL allowing such a session/protocol version mismatch vs (I hope) later
rejecting thing.

[Ryan Sleevi, 2015-02-21 00:07:55]

I'm not sure how I feel about the BoringSSL change. That clamping is limited to
24h, right? That is, after 24h have progressed, we'll negotiate at the newer,
stronger version.

The tradeoff here is that we might be increasing memory usage.

I'm tentatively OK with this, but wanted to discuss more to see if I've missed
something. Fixing NSS is totally sane though.

[davidben, 2015-02-21 00:35:01]

On 2015/02/21 00:07:55, Ryan Sleevi wrote:
> I'm not sure how I feel about the BoringSSL change. That clamping is limited
to
> 24h, right? That is, after 24h have progressed, we'll negotiate at the newer,
> stronger version.

On NSS or BoringSSL? BoringSSL doesn't do any clamping, only NSS does.

For NSS, 24h is the lifetime of a session cache entry, right? Then I think
that's right? Well, it could persist longer if the server has a shorter time
limit and you connect to the server after the session is invalidated on the
server, but before it is on the client. Then you'll mint a new session at the
lower version and refresh the timeout. (Note: I haven't studied NSS's session
cache behavior carefully.)


> The tradeoff here is that we might be increasing memory usage.

That's true. A spurious fallback will drop an entry in the session cache and
likely never be used again until it expires[*]. (Something that does need the
fallback won't though. The non-fallback ClientHello will fail and not drop
anything in the cache.)

We can certainly be clever about it and, say, drop all other entries for a host
when a new one comes in. Or not incorporate version_max into the key and instead
staple the old version_max alongside each session. Then, when deciding whether
to offer a session, validate that version_max matches. If not, pretend the
session's not there. That'll avoid the memory usage risk.

Though that wouldn't work on NSS, nor would the current testing strategy. But
dealing with two of them is generally a pain, so I'm quite happy only doing the
BoringSSL side and instead trying to get Linux/CrOS on BoringSSL sooner. (I hear
I even sent you a CL. ;-) )

[*] Looks like we currently cap number of entries at 1024. And a timeout of...
one hour?? That seems low...


> I'm tentatively OK with this, but wanted to discuss more to see if I've missed
> something. Fixing NSS is totally sane though.

For NSS, we can additionally patch out the clamping because it's crazy.

It's actually not sufficient on it's own to unstick the fallback, interestingly.
We'll still have an effectively sticky fallback on many OpenSSL servers without
doing something to the session cache because NSS *does* enforce session/protocol
version match. So we'll offer our spurious-fallback-created TLS 1.1 session,
OpenSSL server will resume at 1.2, we'll ERR_SSL_PROTOCOL_ERROR and then
fallback. I'm not sure there's any way around making the session cache
fallback-aware if we don't want a sticky fallback and still enforce the match.

[agl, 2015-02-21 00:40:03]

(p.s. I'm happy with this change.)

[Ryan Sleevi, 2015-02-21 00:47:12]

On 2015/02/21 00:35:01, David Benjamin wrote:
> On NSS or BoringSSL? BoringSSL doesn't do any clamping, only NSS does.
> 
> For NSS, 24h is the lifetime of a session cache entry, right? Then I think
> that's right? Well, it could persist longer if the server has a shorter time
> limit and you connect to the server after the session is invalidated on the
> server, but before it is on the client. Then you'll mint a new session at the
> lower version and refresh the timeout. (Note: I haven't studied NSS's session
> cache behavior carefully.)

Well, your description presents the BoringSSL issue as a problem with peers, not
with the client. That is, if the peer upgrades their configuration, the peer
will continue to negotiate the weaker settings until the client stops
advertising the session ID, right?

Further, when last I played in this space, Apache and nginx flushed their
session caches when their SSL configs changed, so even if we advertised an older
session, wouldn't it fail and get a full handshake?

> > The tradeoff here is that we might be increasing memory usage.
> 
> That's true. A spurious fallback will drop an entry in the session cache and
> likely never be used again until it expires[*]. (Something that does need the
> fallback won't though. The non-fallback ClientHello will fail and not drop
> anything in the cache.)
> 
> We can certainly be clever about it and, say, drop all other entries for a
host
> when a new one comes in. Or not incorporate version_max into the key and
instead
> staple the old version_max alongside each session. Then, when deciding whether
> to offer a session, validate that version_max matches. If not, pretend the
> session's not there. That'll avoid the memory usage risk.

I'm concerned on the server side how this might be malicious exploited (if at
all), and on the client side if this might cause unreasonable growth. I'm
totally fine with finding a BoringSSL-only fix for this, and I hope it doesn't
require us getting to clever. If the versions don't match, not only do we
pretend the session isn't there, we just actively drop it. Does that sound
workable?

> [*] Looks like we currently cap number of entries at 1024. And a timeout of...
> one hour?? That seems low...

I thought you were copied on that CL. Yes, the Chromecast team capped it low for
their needs. APIs are hard and such.

[davidben, 2015-02-21 01:12:13]

On 2015/02/21 00:47:12, Ryan Sleevi wrote:
> On 2015/02/21 00:35:01, David Benjamin wrote:
> > On NSS or BoringSSL? BoringSSL doesn't do any clamping, only NSS does.
> > 
> > For NSS, 24h is the lifetime of a session cache entry, right? Then I think
> > that's right? Well, it could persist longer if the server has a shorter time
> > limit and you connect to the server after the session is invalidated on the
> > server, but before it is on the client. Then you'll mint a new session at
the
> > lower version and refresh the timeout. (Note: I haven't studied NSS's
session
> > cache behavior carefully.)
> 
> Well, your description presents the BoringSSL issue as a problem with peers,
not
> with the client. That is, if the peer upgrades their configuration, the peer
> will continue to negotiate the weaker settings until the client stops
> advertising the session ID, right?

The BoringSSL issue is that OpenSSL server has a bug that was only recently
fixed:

https://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=9e189b9dc10786c75591...
(Note the timestamp on that is quite recent)

Every client out there besides BoringSSL will error if the server tries to
resume a TLS 1.1 session at TLS 1.2. This is completely incompatible with the
world.

Mostly due to not keying session cache by IP, it's possible BoringSSL would hit
this harder than other clients, so we opted not to enforce the match in
BoringSSL. I'd like to change this, so I started gathering UMA. It occurred to
me that a spurious fallback would be much more likely to hit this than some
weird multi-homed server config where two IPs could cross-resume but had
different maximum TLS versions. Today, if we hit a network error and fall back
to a 1.1 ClientHello against a 1.2 OpenSSL server before that fix (and before
FALLBACK_SCSV), we'll negotiate a 1.1 session. We then hit it again without a
fallback and offer that 1.1 session with a 1.2 ClientHello.

After my fix, that server would do a full handshake and everything will be fine.
Before, it would negotiate 1.2 and resume the 1.1 session. This is that mismatch
I want to forbid because it's crazy. (What if TLS 1.3 changes the master secret
length?)

Sharding the session caches eliminates that. If we have a TLS 1.1 session
because we spuriously fell back to 1.1, we will never offer it in combination
with a 1.2 client_version.


> Further, when last I played in this space, Apache and nginx flushed their
> session caches when their SSL configs changed, so even if we advertised an
older
> session, wouldn't it fail and get a full handshake?

I'm less interested in peers upgrading their configuration and more in what
happens on spurious fallback. Though even that's not quite enough because of
session tickets. It's certainly still possible to hit the mismatch in older
OpenSSLs even after sharding the session cache. My hope is that after resolving
the spurious callback case will drive the numbers even closer to zero. (Though,
honestly, I'd be fairly comfortable enforcing it as it is. The fallback saves us
anyway.)


> I'm concerned on the server side how this might be malicious exploited (if at
> all), and on the client side if this might cause unreasonable growth.

I'm confused. What's the potential for exploit here? This is just a client-local
decision about whether or not to offer a session to resume and which.


> I'm
> totally fine with finding a BoringSSL-only fix for this, and I hope it doesn't
> require us getting to clever. If the versions don't match, not only do we
> pretend the session isn't there, we just actively drop it. Does that sound
> workable?

Actually, I think we specifically don't want to actively drop it. That would
mean we more-or-less never resume sessions against servers that need the
fallback. I actually wouldn't object to actively punishing such servers that
way, but if we're doing that, we may as well just disable the session cache
altogether on all fallback legs and be done with it. Otherwise we only want to
drop it when a new successful session comes in.

But sure, there's any number of variations that would be easy to do on the
BoringSSL port because we have more control over the session cache and can check
interesting things. (Per crbug/454044 we currently completely botched this
additional control, but nevermind that. I'll rewrite it next week.) NSS is less
flexible, so we just get to manipulate the cache key.


> > [*] Looks like we currently cap number of entries at 1024. And a timeout
of...
> > one hour?? That seems low...
> 
> I thought you were copied on that CL. Yes, the Chromecast team capped it low
for
> their needs. APIs are hard and such.

I think that was the server bit. This dates to
https://codereview.chromium.org/89623002

[davidben, 2015-02-21 01:16:12]

> on the client side if this might cause unreasonable growth

The potential for growth is at most a factor of 3x since that's the number of
legs in our fallback chain. But it's really much much lower than that. It'd just
be the fraction of connections we make that are spurious fallbacks. Otherwise
we'll never have more than one entry per host.

As far some network attacker trying to drive up that growth... I guess? They
could just as easily get you to visit a page that contained
  <img src="https://blah${X}.evil.com">
for X in 1..1000. That'll blow through your session cache WAY more efficiently.

[Ryan Sleevi, 2015-02-21 01:30:48]

lgtm

[davidben, 2015-02-23 17:48:26]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-02-23 17:49:29]

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

[commit-bot: I haz the power, 2015-02-23 18:00:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-23 18:01:23]

Patchset 2 (id:??) landed as
https://crrev.com/21ea1b4ef8d6c111605866108c7dac1b31440114
Cr-Commit-Position: refs/heads/master@{#317605}