[TLS] KeyUpdate storms from buggy update_requested logic

[David Benjamin <[e-mail]>]

Hi all,

MT and I were discussing KeyUpdate at the meeting, and I realized I never
got around to writing up an issue we'd observed in real world TLS 1.3
deployments. rfc8446bis is pretty far along now, but MT suggested I write
this up anyway:

We ran into a fun bug with a major TLS implementation that would send an
update_requested KeyUpdate once the other side sent more than N records.
However, they were broken and *sent it on every record that came in after
the threshold was crossed*. Now, imagine the channel is asymmetric and you
are sending at a much faster rate than the buggy peer. Or, even worse,
imagine if this is a simple, non-multiplexed request/response application
protocol, and you're in the middle of sending a very, very large response.
You won't read from the peer until you're done with that response.

In the time it takes for you to finish sending your response, resume
reading, pick up the KeyUpdate, and reply, there will have been many, many
record's past the peer's threshold. Once you go back to reading, you wake
up to a huge storm of KeyUpdates. That's assuming the peer hasn't managed
to deadlock itself by filling the TCP send buffer with KeyUpdates and
blocking, or DoS itself by buffering those writes up instead and burning
memory on queued writes.

This is clearly undesirable. There is no point in sending an
update_requested past the first one because repeat requests won't change
anything. The spec already has text that says:

> Note that implementations may receive an arbitrary
> number of messages between sending a KeyUpdate with request_update set
> to "update_requested" and receiving the
> peer's KeyUpdate, because those messages may already be in flight.

However, one needs to read between the lines to then realize "and therefore
I should not keep sending request_update because it's pointless". I've
filed https://github.com/tlswg/tls13-spec/issues/1341 describing the issue
and uploaded a PR at https://github.com/tlswg/tls13-spec/pull/1343 to
clarify this point.

Thoughts?

David