Disable TLS renegotiation #9885
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test
CentOS 7.
error:1409E0E5 needs some time to appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entering R triggers:
And after a lot of business as usual via futex and clock_gettime it gets closed:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While awaiting R result:
👍 The renegotiation attempt happens completely in the background.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even
for i in {1..30}; do while true; do openssl s_client -connect 10.27.0.91:5665 -no_tls1_3 <<<R; done & donedoesn't impress Icinga 2 CPU usage nor the gdb picture shown above andcurl -kv https://10.27.0.91:5665/v1/go through immediately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "some time to appear" 15 seconds by any chance? That's probably just
ApiListener.connect_timeoutkicking in and forcefully terminating the connection:Looks more like a TLS connection remaining in a weird state instead of being terminated with an error like it should.
So, let's go down the debugging rabbit hole and write a minimal reproducer to see if there's might be a problem somewhere else in the Icinga 2 code and allow for quicker testing:
asio-tls.cpp
And it showed just the same behavior: trigger a renegotiation and the connection hangs.
Then I found an interesting commit in Boost.Asio, but applying it did not make a difference.
Next, trying Boost 1.83 on CentOS 7 (still with the antiquated OpenSSL 1.0.2 it provides): still the same result, connection hangs.
Next attempt: try OpenSSL 1.1.1, and now it works and you immediately get the "no renegotiation" error, even with Boost 1.69, so doesn't look like that Boost version is doing something completely wrong here as the commit I referenced previously might suggest.
Hints for compiling with different versions of Boost and OpenSSL
OpenSSL 1.0.2, Boost 1.69
OpenSSL 1.0.2, Boost 1.83 (run with LD_LIBRARY_PATH=/opt/boost183/lib)
OpenSSL 1.1.1, Boost 1.69
OpenSSL 1.1.1, Boost 1.83 (run with LD_LIBRARY_PATH=/opt/boost183/lib)
openssl11-{devel,libs}can be installed from EPEL, Boost 1.83 can be installed from source (./bootstrap.sh --prefix=/opt/boost183; ./b2 install).But still, nginx manages to immediately give you an "no renegotiation" error with OpenSSL 1.0.2, also note how something is read from the socket instead of it just closing:
So that's where I got so far, something is odd here, I just don't know what exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean as in the connection is cleaned up immediately. Can you explain the state the connection remains in within Asio otherwise? What would happen if the client would send more data?
It's not about legit clients, if we'd only consider those, we wouldn't need this PR at all.
How is this a security feature? All I can tell is that a connection in an unclear (to me) and somewhat dysfunctional state remains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually ASIO only r/w-s data from/to the other side as we instruct it to do, passing it through OpenSSL (to duplicate as less of its logic as possible).* The latter is indeed interesting:
First of all, as a bad guy "exploiting" this "weakness" I'd still have to follow the protocol. I.e. to initiate a handshake, which is what OpenSSL does on renegotiation**, and to wait for the response before (maybe) sending anything else. After all I'd expect the server to kick me for protocol violation. But, to actually answer your question, our imaginary bad guy will notice my little client-hangup trick and not wait for a handshake response. Instead he'll immediately send data as if the renegotiation handshake never happened. (At least this is how I understand your question.)
Before the additional data, let's see what happens on the server on renegotiation itself. SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS is used in three different locations (ex. headers and DTLS):
(2) is on the client side. (1), ssl3_renegotiate(), is only called by:
And (3) is the only interesting one as it's ssl3_read_bytes(), the thing which ASIO calls* to receive the renegotiation request at all. Due to the flag being set, this if isn't run (but the rest of the condition implies SSL_ST_OK): https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_pkt.c#L1559-L1572
On the next line ssl3_accept() is called: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_pkt.c#L1573
Nothing really interesting happens during its init: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_srvr.c#L214-L227
Ex. the increment here: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_srvr.c#L230
But it's undone on return: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_srvr.c#L884
Oh. Due to currently SSL_ST_OK, the SSL object is cleared:
But SSL_ST_OK stays, so the SSL is cleaned even more before returning 1: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_srvr.c#L828-L837
If I did the math correctly, we're in a kinda funny state now. But not in a somewhat dangerous one, this seems to be just the immediate post-handshake state, as set also by: https://github.com/openssl/openssl/blob/OpenSSL_1_0_2k/ssl/s3_srvr.c#L724-L749
Finally reading more data is scheduled (which explains that the connection isn't active and the ASIO event loop is waiting for something to happen, see gdb above):
For a moment (the "Oh." above) I really wasn't sure, but now I think it is fine. I mean, a fresh state as just after the regular handshake and the server waits for some data. Exactly the additional data you worry about.👍 I'd say business as usual. Really as if the renegotiation was never requested. Could be worse. In addition I can crash-test the whole thing if you wish. But I don't think this is necessary. I mean, depending on the state OpenSSL either reads more data or throws an SSL error, e.g. protocol violation. What can happen?
On a DoS client the connection is indeed broken as it expects a handshake response, but gets nothing at all. Now instead of wasting our CPU, our bad guy's OpenSSL hangs.👍
Notes
I can explain those in human readable language if you wish, but not at this unorthodox time of the day.😅
*
**
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Icinga says
[2023-12-08 13:56:48 +0100] information/ApiListener: No data received on new API connection from [::ffff:127.0.0.1]:64397. Ensure that the remote endpoints are properly configured in a cluster setup.👍 on additional data after a renegotiation request via the following program:TL;DR
IMAO we have the following options (sorted by my preference, ascending):
(If I understood you correctly, legitimate API users aren't affected as they don't renegotiate.)
We could also make this decision even easier by using OpenSSL 1.1+ wherever it's available (everywhere) and doesn't conflict with v1.0 (used by libmysqlclient and libpq) at runtime (almost everywhere).
*
Shall we count derivatives (indented) separately here? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does this do? Does it write plain text directly to the TCP connection as I think it does? If so, what happens if you try to send something inside the TLS connection (which should be
SSL_write())?But looks like it's actually able to unblock the coroutine here:
icinga2/lib/remote/apilistener.cpp
Lines 762 to 776 in 420db15
Would be interesting to know what
error_code/ecis here (should be something like protocol error) and if this would actually successfully continue if you sent a HTTP request inside the TLS connection for example.So is that the answer to what's happening here? We're inside a read, OpenSSL receives a renegotiation request, ignores it, and because we're in a read, just tells Boost.Asio that it wants to read more data (as we're in a read after all)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wireshark said it is TLS application data. Admittedly I've written this throwaway program not entirely by myself to save time. So yes, that may be partially crap, but as long as that crap does its thing... 👍
Of course. I mean, the coroutine is awaiting something to read and my C program puts something on the wire. Business as usual. 🤷♂️
This is actually a good idea by itself!
But, to answer the question:
[2023-12-11 17:14:27 +0100] information/ApiListener: No data received on new API connection from [::ffff:127.0.0.1]:50906. Ensure that the remote endpoints are properly configured in a cluster setup. Error: unexpected recordI doubt, given the error above.
Exactly!