SSL_clear do not reset the state of the QUIC members

[jpboivin]

Hello,

It looks like SSL_clear doesn't reset any QUIC-specific members (like the quic_read_level / quic_write_level). This means that it is currently impossible to re-use the same SSL object to re-establish another connection.

I guess we should reset the following members:

• quic_read_level to ssl_encryption_inititial
• quic_write_level to ssl_encryption_inititial
• quic_latest_level_received to ssl_encryption_inititial
• quic_buf to an empty buffer
• quic_input_data_head to NULL
• quic_input_data_tail to NULL
• quic_next_record_start to 0

Am I missing anything?

[tmshort]

TBH, I usually don't recommend re-using SSL objects; precisely because of these types of bugs.
PRs welcome.

[tmshort]

I'm working on this, but it's non-trivial, as SSL_clear() is called in unexpected places...

[tatsuhiro-t]

Since a467c89, ngtcp2 server no longer works anymore.

It looks like SSL_clear is called from

openssl/ssl/statem/statem.c

Line 337 in a467c89

if ((s->s3.flags & TLS1_FLAGS_STATELESS) == 0 && !SSL_clear(s))

.
It happens just after providing handshake data to SSL.

[kaduk]

Yes, this call to SSL_clear() from within libssl is quite annoying (and is responsible for the need for Todd to change the unit tests as part of this PR).
It actually makes me wonder if we should just revert this change, since it seems like in practice if we want this change we would be requiring all servers using quictls to make a dummy call to SSL_accept() (doomed to failure) before supplying quic data, and that's a pretty odd requirement to impose.

[nibanks]

we would be requiring all servers using quictls to make a dummy call to SSL_accept()

That sounds pretty annoying to me. I haven't updated MsQuic to the latest that includes this, because we didn't need it (as far as I could tell), but I would be a bit annoyed if I had to do this if/when a new security patch comes out and I updated to get the fixes and this breaks.

[tmshort]

So, my first comment was that "I usually don't recommend re-using SSL objects", and this is exactly why.

In the case of the unit tests, the SSL_accept() call is done first because normally, the data would have been transmitted over a socket. So the server is sitting there, blocked on SSL_accept(), and then the data is received.

Fundamentally, if the QUIC data is received before SSL_accept() is called (which was the case in the tests), then the SSL_clear() in SSL_accept() will erase that data. I had thought of putting the initial QUIC data to the server in a temporary buffer, but the failed SSL_accept() worked.

So, I see a number of options

1. ngtcp2 changes how it provides data to the server. A bad idea if ngtcp2 already works with BoringSSL.
2. We could fix the line that @tatsuhiro-t mentions to check SSL_IS_QUIC, but that might break other things.
3. Revert and say "don't do that".
4. Revert and add a new API to like SSL_clear_quic() to clear out QUIC data (which is not a BoringSSL API).

I'm leaning toward 2 or 3.

[jpboivin]

Like @tatsuhiro-t, we had to change our code to do a dummy SSL_do_handshake call on the server to "put it in the right state", before feeding any received data. Typically, our loop would be feeding data -> doing handshake, so it broke by calling SSL_clear internally and discarding pending data.

I'd be leaning toward 2, as it looks to me that SSL_clear is not wrong per say, but more that SSL_accept has wrong assumptions when used with QUIC. I wonder if there is any valid cases where the clear would be required in SSL_accept for QUIC, and that this change would break them? I can't think of any from what I know, but I can be easily missing something.

4. Revert and add a new API to like SSL_clear_quic() to clear out QUIC data (which is not a BoringSSL API).

Do you know if BoringSSL has a working SSL_clear implementation for QUIC?

[tmshort]

2. We could fix the line that @tatsuhiro-t mentions to check SSL_IS_QUIC, but that might break other things.

I did note that line in SSL_accept(), which is how I came up with the unit test solution.

[tmshort]

Do you know if BoringSSL has a working SSL_clear implementation for QUIC?

AFAICT, BoringSSL does not call SSL_clear() within the state machine.

This is what BoringSSL does for SSL_clear():

  ssl->method->ssl_free(ssl);
  if (!ssl->method->ssl_new(ssl)) {
    return 0;
  }

Which is basically freeing the SSL and then newing it.

[tmshort]

For those who are experiencing problems, is it on 1.1.1 or 3.0? (i.e. which one should should I try an initial patch for?)

[tatsuhiro-t]

I found this issue with 3.0.0, but I can pin dependency down to the working revision, so either is fine.

[jpboivin]

For those who are experiencing problems, is it on 1.1.1 or 3.0? (i.e. which one should should I try an initial patch for?)

We are using 1.1.1, but if your solution works fine for @tatsuhiro-t, it should work fine for us too. We are using ngtcp2, but with a custom crypto layer, also based on OpenSSL.

[tmshort]

I pushed a new fix for 1.1.1 (#61) and 3.0 (#62). Could someone (@tatsuhiro-t ?) see if it works better for them?

[nibanks]

@tmshort I can't right now, but if you ever want to run a 1.1.1 change through the (quite extensive) MsQuic automated CI, simply make a draft PR to MsQuic with an updated submodule and I'll approve the CI run. It should give you coverage on Linux, Windows, and macOS; functional, stress and performance. We can let it run to get the results and then close the PR afterwards.

[tmshort]

Thanks @nibanks: microsoft/msquic#2104

[jpboivin]

@tmshort Tested your changes with 1.1.1 and I could remove the SSL_do_handshake that we had to add. The clear also does work as expected.

[tatsuhiro-t]

The fix works for me. Thank you!