<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="m_-6827951325917517961gmail_attr">On Wed, Jan 23, 2019 at 4:24 AM Matt Caswell <<a href="mailto:matt@openssl.org" target="_blank">matt@openssl.org</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 22/01/2019 20:41, David Benjamin wrote:<br>
> On Tue, Jan 22, 2019 at 1:48 PM Viktor Dukhovni <<a href="mailto:openssl-users@dukhovni.org" target="_blank">openssl-users@dukhovni.org</a><br>
> <mailto:<a href="mailto:openssl-users@dukhovni.org" target="_blank">openssl-users@dukhovni.org</a>>> wrote:<br>
>     As for applications mishandling "SSL_CB_HANDSHAKE_START", not quite sure<br>
>     what to do there, but perhaps we could define a new even for keyUpdates<br>
>     that does not mislead applications into assuming a new "handshake".<br>
> <br>
> I think this is clearly the right option. This is a compatibility break, IMO.<br>
> <br>
> Prior to SSL_OP_NO_RENEGOTIATION (new in the same release that added 1.3), this<br>
> was the only way to disable renegotiations. I've seen a lot of codebases do<br>
> this. I don't think one could even call it a mishandling. The natural<br>
> interpretation of "SSL_CB_HANDSHAKE_START" is that a handshake has started.<br>
> Thus, if you wish to block renegotiations, absent a more direct API, it's<br>
> natural to count those and fail if you see more than one.<br>
> <br>
> A KeyUpdate is not a handshake.<br>
<br>
That's depends on your perspective. One peer sends a message, and the other peer<br>
(may) respond with a message. Sounds like a handshake to me. KeyUpdate gets sent<br>
in handshake records, with a HandshakeType value of 24 and is defined in section<br>
4 of the RFC ("Handshake Protocol").<br></blockquote><div><br></div></div><div dir="ltr"><div class="gmail_quote"><div>This notion of "handshake" is not supported by RFC 8446 uses the terms "the handshake", "a handshake", and "post-handshake". "Post-handshake", in particular, implies KeyUpdate are after the handshake, not part of it.</div><div><br></div><div>KeyUpdate is also not quite a request/response pair. The caller is allowed to <a href="https://tools.ietf.org/html/rfc8446#section-4.6.3">coalesce consecutive key_update_requests</a>. This was done to <a href="https://mailarchive.ietf.org/arch/msg/tls/cfw4paCGxI7Fj8QNmj6k1I66VII">avoid DoS</a> and allow a lighter strategy: if you see key_update_request on read, set a flag and continue. When you make an outgoing record on write, if the flag is set, queue a KeyUpdate up first and clear the flag. This avoids the DoS: write overhead is bounded and read/write flows are independent.</div><div><br></div><div>Finally, compare with TCP socket APIs, which most applications have in mind. There is a connect() phase that happens <i>once</i> at the start, afterwards one calls read() and write(). During read() and write(), TCP still ends other non-data packets, but those are abstracted away.</div></div></div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
To me it makes perfect sense to signal it this way. In fact it was a deliberate<br>
decision to do so and is documented accordingly:<br>
<br>
 =item SSL_CB_HANDSHAKE_START<br>
<br>
 Callback has been called because a new handshake is started. In TLSv1.3 this is<br>
 also used for the start of post-handshake message exchanges such as for the<br>
 exchange of session tickets, or for key updates. It also occurs when resuming a<br>
 handshake following a pause to handle early data.<br>
<br>
 =item SSL_CB_HANDSHAKE_DONE           0x20<br>
<br>
 Callback has been called because a handshake is finished. In TLSv1.3 this is<br>
 also used at the end of an exchange of post-handshake messages such as for<br>
 session tickets or key updates. It also occurs if the handshake is paused to<br>
 allow the exchange of early data.<br></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>This documentation does not help code written prior to TLSv1.3, which is the problem here. OpenSSL claims that TLS 1.3 is a backwards-compatible addition, so documentation updates for 1.3 may clarify but cannot be necessary. More on this below.</div></div></div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Ironically it was done this way with a view to *avoiding* a compatibility break.<br>
The thinking was that applications written with TLSv1.2 in mind may find it<br>
surprising to start receiving events after the initial connection setup that are<br>
not enclosed in HANDSHAKE_START/HANDSHAKE_DONE. The info callback is intended as<br>
a tracing API IMO. It's only the fact that it has been used in ways other than<br>
we might have expected that we have this problem. In fact if your application is<br>
using the API to detect renegs because it wants to report when key updates are<br>
happening (rather than blocking them)....well that application will continue to<br>
work and will stop working if we change the events.<br>
<br>
Given this is documented that way, some applications may already be using it -<br>
so changing the events now should be done with caution. That itself could cause<br>
a break. We know that this is causing some problems now because of the way that<br>
some applications are (mis)using this API. What we don't know is what new<br>
problems would surface if we changed its semantics in a letter release.<br>
<br>
Really the root of this problem is not in the info callback at all. It is the<br>
fact that in earlier 1.1.0 releases we did not have an effective way of blocking<br>
renegs. We now have SSL_OP_NO_RENEGOTIATION which is the "right" way to do this.<br>
I'd also note that this was backported to the 1.1.0 branch in 1.1.0h (March<br>
2018) and so has been available to all 1.1.x users for some while.<br></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>The fact of the matter is the TLS 1.3 ecosystem is intolerant to KeyUpdates, as a result of behavior in OpenSSL and its consumers. As a result of this failure, <i>no one</i> can use KeyUpdate. This warrants consideration on both sides of the API boundary.</div><div><br></div><div>You're right that adapting this API to 1.3 required a judgement call. It observed a state machine which changes across versions. There isn't a right answer, a priori. Elsewhere in this thread, I pushed against adding new tracing events without a clear need. This is counter to OpenSSL's usual API style but comes from experience developing on both sides of the API. (My day job involves working both on BoringSSL itself and code calling into it, much of which was written to OpenSSL.) APIs like that are problematic. I'll note BoringSSL doesn't expose state constants at all.</div><div><br></div><div>But OpenSSL 1.1.0 and 1.0.x had that API without SSL_OP_NO_RENEGOTIATION, so we must deal with it. Note March 2018 is not "for some while" on the timescales of OpenSSL consumers, sadly. Ubuntu 18.04 LTS still ships <a href="https://packages.ubuntu.com/bionic-updates/openssl" target="_blank">1.1.0g</a> (plus security fix backports). OpenSSL 1.0.x is also relevant. Projects did not port to OpenSSL 1.1.0 by revising every API call against the new documentation. They tried to compile it, fixed any errors, maybe ran some basic tests, fixed any obvious runtime issues, and called it done. This is simply the natural way to do it. Change strategies must <a href="https://boringssl.googlesource.com/boringssl/+/HEAD/BREAKING-CHANGES.md#fail-early_fail-closed">take this reality into account</a>.</div><div><br></div><div>OpenSSL's API thus has a long history. Its consumers grew up organically with OpenSSL, sometimes even SSLeay. Judgement calls cannot be made in a vacuum. When we were designing TLS 1.3 support for BoringSSL, every question like this was backed by sampling existing callers. From that experience, I have seen the following uses of the info callback:</div><div><br></div><div>(a) Debugging hooks for tracing, often <a href="https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=apps/s_cb.c;h=af57c34558684d4bc027d1471b63d5e4f7a316ab;hb=HEAD#l454" target="_blank">copied from the openssl binary</a>.</div><div>(b) As a callback to know when the handshake (in the RFC8446 sense described above, not the OpenSSL sense) is done, sensitive to SSL_CB_HANDSHAKE_DONE.</div><div>(c) As a callback to block renegotiations.</div><div><br></div><div>The problem here is (c), and empirically has affected versions of NGINX, Node, HAProxy and the real TLS 1.3 ecosystem. There may be more yet undiscovered problems; we only had KeyUpdates on in Chrome for a week on Chrome Canary before we had to shut it off. At three affected callers, one cannot simply say this is the consumer's fault.</div><div><br></div><div>As for the others, (b) also doesn't want to trigger on KeyUpdate, though it may tolerate it. (I have seen versions of (b) which ignore duplicates and versions which break on renegos---no one tests against it, which is why it's off by default in BoringSSL.) (a) is closest to the scenario you are concerned about, but such debugging notes are just that: debugging. I have never seen code which cares about their particulars. Indeed, if it did, adding TLS 1.3 would not be compatible because 1.3 changes the state machine.</div><div><br></div><div>Thus, the fix is clear: don't signal HANDSHAKE_START and HANDSHAKE_DONE on KeyUpdate. Not signaling has some <a href="https://boringssl.googlesource.com/boringssl/+/HEAD/BREAKING-CHANGES.md#breakage-risk">risk</a>, but it is low, especially in comparison to the known breakage and ecosystem damage caused by signaling.</div><div><br></div><div>David</div></div></div></div>