tlswg / tls13-spec

TLS 1.3 Specification
565 stars 157 forks source link

Forbid more than one outstanding key_update_requested at a time #1341

Closed davidben closed 2 months ago

davidben commented 5 months ago

This is a bit late, but I kept forgetting to file this. :-(

We ran into a fun bug with a TLS implementation that would send key_update_requested once the other side sent more than N records. However, they were broken and send it on every record that comes in. Now imagine one side is sending at a much faster rate than the other. In the time it takes for the other side's KeyUpdate to come, there may have been many records and so the sender sends a huge storm of them.

Probably we should have a sentence to the effect of: if you send key_updated_requested, you MUST NOT send another until you've gotten a KeyUpdate from the other side.

tomato42 commented 5 months ago

I don't think it's enforceable... and for links with high bandwidth and high latency it may be impractical

camshaft commented 5 months ago

We've been seeing a very similar issue. The client submits a request and the server sends a very large response (>125Gb). While it's sending the response, the server never reads from the client. Once the client decides to start enforcing the configured key limit, it sends a key update + request message for every record it receives. It sends so many of these that the TCP flow window is completely filled and the connection deadlocks.

Maybe instead of putting a limit on in-flight key update requests the RFC more strongly indicates that the key update request is merely a request, not a requirement, and receivers ultimately cannot enforce key limits below the absolute maximum, at which point they can close the connection if the upper limit is exceeded.

martinthomson commented 5 months ago

If you have grossly asymmetric send weight, then the sender can always unilaterally update.

The feature whereby an update is requested is different. While there are cases where we might like to have one endpoint poke the other to update more often than they would on their own, this does not extend to a license to DoS.

I support making this change.