tree 42c7db7d57ce60a440aa29fa085c2960b7362178
parent ee477d433e0297dcdd4e51139fcbd0700cf794df
author David Benjamin <davidben@google.com> 1661873931 -0400
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> 1661900207 +0000

Prefer established session properties mid renegotiation.

Among many many problems with renegotiation is it makes every API
ambiguous. Do we return the pending handshake's properties, or the most
recently completed handshake? Neither answer is unambiguously correct:

On the one hand, OpenSSL's API makes renegotiation transparent, so the
pending handshake breaks invariants. E.g., currently,
SSL_get_current_cipher and other functions can return NULL mid
renegotiation. See https://crbug.com/1010748.

On the other hand, OpenSSL's API is callback-heavy. During a handshake
callback, the application most likely wants to check the pending
parameters. Most notably, cert verify callbacks calling
SSL_get_peer_certificate.

Historically, only the pending state was available to return anyway.
We've since changed this
(https://boringssl-review.googlesource.com/8612), but we kept the public
APIs as-is. I was particularly worried about cert verify callbacks.

As of https://boringssl-review.googlesource.com/c/boringssl/+/14028/ and
https://boringssl-review.googlesource.com/c/boringssl/+/19665/, cert
verify is moot. We implement the 3-SHAKE mitigation in library, so the
peer cert cannot change, and we don't reverify the certificate at all.

With that, I think we should switch to returning the established
parameters. Chromium is the main consumer that enables renegotiation,
and it would be better off with this behavior. (Maybe we should try to
forbid other properties, like the cipher suite, from changing on
renegotiation. Unchangeable properties make this issue moot.)

This CL would break if the handshake internally used SSL_get_session,
but this is no longer true as of
https://boringssl-review.googlesource.com/c/boringssl/+/41865.

Update-Note: Some APIs will now behave differently mid-renegotation. I
think this is the safer option, but it is possible code was relying on
the other one.

Fixed: chromium:1010748
Change-Id: I42157ccd9704cde3eebf947136d47cda6754c36e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54165
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>