Optimize Connection Memory Usage once Handshake is complete

[wbudd]

s2n -- finally a well-designed TLS library! As far as I'm concerned there is still some noise among the signal though: TLS renegotiation support is a feature that doesn't solve any real world problem of significance, while having a history of being repeatedly exploited directly and indirectly in various TLS attacks (e.g., the "renegotiation attack" and "the triple handshake attack").

Besides the numerous security benefits of eliminating this attack vector, scrapping this non-feature would also have the benefit of being able to do away with much of the data bloat that currently exists in the s2n_connection struct: the presence of two full sized 4088 byte s2n_crypto_parameters structs ("active" and "pending"). Without renegotiation support, only one s2n_crypto_parameters struct is needed (given that no negotiation would ever occur while the parameters are in use), reducing the size of the s2n_connection struct by 36% from 11488 bytes to 7400 bytes (on X86_64).

BTW: Even if TLS renegotiation support is deemed indispensable, a large volume of memory savings should still be possible in the connection struct. One idea relatively easily implemented would be to put all mutually/temporally exclusive state in unions (e.g, union { struct s2n_handshake, struct s2n_data_that_is_only_relevant_after_the_handshake}), but let's call that a separate issue.

I know performance considerations are at the bottom of the list of priorities for s2n, but for servers routinely handling 1000s of simultaneous TLS connections, shaving of multiple KB per connection is not insignificant, I think.

[colmmacc]

Thanks for the feedback! renegotiations are disabled in s2n; it doesn't advertise the ability to renegotiate and will ignore any attempt by the peer. So I think we're well covered on the protocol vulnerability side.

That's a good point about the memory, it's not insignificant - and maybe we should move that memory to being dynamically allocated. One of the reasons that the code is in there though is that we may support mutual auth, and doing so practically would require support for renegotiation. But it's not the kind of thing that we would enable by default, ever.

How does that seem?

[wbudd]

Thanks for clearing that up. Glad to hear renegotiation is disabled by default -- sorry, I should have checked that myself, but my assumption was based on this comment I came across in s2n_handshake_io.c:

Reading is a little more complicated than writing as the TLS RFCs allow content
types to be interleaved at the record layer. We may get an alert message
during the handshake phase, or messages of types that we don't support (e.g.
HEARTBEAT messages), or during renegotiations we may even get application
data messages that need to be handled by the application. The latter is punted
for now (s2n does support renegotiations).

The "during renegotiations we may even get" and "(s2n does support renegotiations)" parts seeming to be in contradiction with your "s2n [...] will ignore any attempt by the peer" statement. (Also, isn't "punted" a term better avoided when aiming for clarity?! =)

Is mutual auth not just a different term for a client-authenticated TLS handshake? Because in that case, the only additions to the handshake are the client certificate and its certificate verify message, or am I missing something? I'm not sure how that would practically require renegotiation or in any way necessitate having two sets of crypto parameters per connection. Can you elaborate on that?

[colmmacc]

I'll clear up the comments in the code. Thanks! at one point in the early development renegotiations were enabled.

Yes, mutual auth is the same as client-authentication (from the point of view of the server). In practical terms the way that client auth interacts with HTTPS is that the client maintains a persistent connection. It first attempts to get a resource, then gets an authorization required error message, and then renegotiates using the client certificate. It is possible to make this work without having renegotiations enabled, but means a new connection and adds delay and overhead. I'm not sure that many environments would be able to tolerate this.

[wbudd]

Fair enough. The overhead of requiring a new connection in that scenario does not seem like a big deal to me, but clearly I'm not particular to all the considerations involved there, so I won't pester you further with arguments against renegotiation in the case of mutual auth.

Either way, I'm a proponent of the idea that short lived state variables and/or variables only used in special cases should preferably not take up full space in the s2n_connection struct (which in practice pretty much amounts to giving them permanent lifetime). Besides the obvious performance benefits of reducing processor cache misses and such, I think the case can be made that it would improve security as well (e.g., reducing the amount of state information that can be inadvertently leaked, as well reducing the risk of code accidentally referencing wrong/irrelevant variables).

It might be a good idea to do a full review of the connection struct and the entire tree of descendants it contains, and identify candidates for tighter "manual scoping" so to speak, either by moving them to the heap or through (explicitly wiped) unions or otherwise (for example: "Do we really need to hold on to the premaster secret, twice even? Or should we wipe/free it when done with it?").

I'm considering doing that myself, and putting in a pull request somewhere down the road -- provided I can find a relatively non-disruptive way to do so. But I figure I'd first need to find out how (un)popular an approach like that would likely be...

[colmmacc]

I'm certainly a fan of that approach, it sounds very useful and beneficial.

[hyandell]

@colmmacc "I'll clear up the comments in the code." - did that happen?

[colmmacc]

We're about halfway through this. I've fixed the memory alignment, but we haven't consolidated everything yet to be less wasteful.

[goatgoose]

We are cleaning up stale feature requests. Please reopen this issue if you believe it should be reprioritized.