Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Secure Client-Initiated Renegotiation Testing Yields False Vulnerability #2590

Open
prabhakaran-motorq opened this issue Oct 18, 2024 · 13 comments
Labels
bug:to be reproduced ... from maintainers waiting for more input User needs to give more information

Comments

@prabhakaran-motorq
Copy link

prabhakaran-motorq commented Oct 18, 2024

Secure Client-Initiated Renegotiation Testing Yields False Vulnerability

While testing client-initiated renegotiation, piping the character 'R' into OpenSSL's s_client command can lead to a false vulnerability detection. The issue arises because the client can close the connection before the server has a chance to respond with "closed." As a result, the "closed" status may not be logged when the status code is 0, causing testssl.sh to incorrectly classify the connection as vulnerable.

Details:

We operate our own Kafka servers and ran tests using testssl.sh. During some of these tests, client-initiated renegotiation was incorrectly flagged as vulnerable. Upon further investigation, we discovered that sending the 'R' character through stdin even with a delay was inconsistent and unreliable.

To address this, we devised a more reliable testing method using the expect command. This approach ensures the connection is fully established before sending the renegotiation request ('R').

Commands to Test:

  1. Original Flaky Command
    The following command sometimes fails to give reliable results:

    echo 'R' | openssl s_client -tls1_2 -connect ashu-kafka-broker-0.motorq.com:9094

    To observe the flakiness, the command can be run multiple times:

    # Run original flaky command 50 times
    for i in {1..50}; do echo 'R' | openssl s_client -tls1_2 -connect ashu-kafka-broker-0.motorq.com:9094 |& tail -1 & done 2>/dev/null | sort | uniq -c

    Flaky command output

  2. Reliable Testing Using expect
    The expect command waits for a "CONNECTED" message before sending the 'R' character, ensuring the connection is stable:

    expect -c 'spawn openssl s_client -tls1_2 -connect ashu-kafka-broker-0.motorq.com:9094; expect "CONNECTED"; send "R\r"; interact'

    Running this command multiple times produces consistent results:

    # Run expect command 50 times
    for i in {1..50}; do expect -c 'spawn openssl s_client -tls1_2 -connect ashu-kafka-broker-0.motorq.com:9094; expect "CONNECTED"; send "R\r"; interact' | tail -1; done | sort | uniq -c

    Reliable command output


Issue in testssl.sh

Here is a screenshot of the issue within testssl.sh:

Issue in testssl.sh

I'm using version 3.2, commit ID: 6452ec9. Similar issues have been reported in these discussions:

Expected Behavior:

As testssl.sh is widely used in the industry, it should yield reliable results. In cases where results are inconclusive, the script should indicate this rather than erroneously reporting a vulnerability. The following expect command can be used to test multiple renegotiations within a single session:

expect -c 'spawn openssl s_client -tls1_2 -connect ashu-kafka-broker-0.motorq.com:9094; expect "CONNECTED"; send "R\rR\rR\rR\rQ\r"; interact' | grep 'RENEGOTIATING' -A2

To further validate, a dummy server can be spun up without disabling renegotiation:

openssl req -x509 -newkey rsa:2048 -keyout key.pem -out cert.pem -days 365 -nodes
openssl s_server -key key.pem -cert cert.pem -accept 44330 -www -tls1_2
expect -c 'spawn openssl s_client -connect localhost:44330; expect "CONNECTED"; send "R\rR\rR\rR\rQ\r"; interact' | grep 'RENEGOTIATING' -A2

Testing renegotiation


System Information:

  • OS: Ubuntu 20.04.6 LTS
  • Platform: Linux 5.15.0-1053-azure x86_64
  • OpenSSL & Bash: OpenSSL 1.0.2-bad | Bash 5.0.17

PS:

This is my first time raising an issue here. If there's any additional information that would be helpful, please let me know, and I'd be happy to provide it. Our partner company relies heavily on testssl.sh and is currently blocked due to this issue, so resolving it is critical. I am more than willing to assist in testing, debugging, or providing any further details to expedite a solution. Additionally, feel free to use ashu-kafka-broker-0.motorq.com:9094 for your testing and confirmation. Your support is greatly appreciated.

@drwetter
Copy link
Owner

drwetter commented Oct 28, 2024

Hi,

thanks for reporting!

Unfortunately I can't reproduce it. I tried 50x from North America and 50x from Europe to generate a response VULNERABLE but I wasn't able to.

Independent on that: While suggestions are always appreciated but with expect this is something which won't happen. We'd like to keep dependencies low, like really low.

Actually I don´t know where CONNECTED should come from. Is that a kafka thing?

@drwetter drwetter added waiting for more input User needs to give more information and removed bug:needs triage/confirmation labels Oct 28, 2024
@Tazmaniac
Copy link

The testing logic was completely rewritten to take into account what you called "flaky command". The test no longer use such construct.
see #2470 #2472 #2475 and #2487

@Tazmaniac
Copy link

Tazmaniac commented Oct 30, 2024

You are in the specific case $SERVICE != HTTP.
Which mean the single command "echo R | $OPENSSL s_client $(s_client_options "$proto $legacycmd $STARTTLS $BUGS -connect $NODEIP:$PORT $PROXY $SNI")" returned 0 two times, without server connection close (see L17184 to L17200).
The complex retry logic is even not attempted.
So the observed behavior could only be an other deviation from the known single renegotiation s_client invocation (like L17196).
If you could reproduce the unexpected behavior in debug mode, the content of the TMPFILE and ERRFILE should give us insights.

@Tazmaniac
Copy link

Other though:
Your command
for i in {1..50}; do echo 'R' | openssl s_client -tls1_2 -connect ashu-kafka-broker-0.motorq.com:9094 |& tail -1 & done 2>/dev/null | sort | uniq -c
is launching openssl s_client multiple times in a raw (and not issuing multiple 'R' in the std input of the running command). As the first command to openssl s_client is buffered, it should not be lost.
On the contrary, issuing multiple commands in a raw without waiting for each intermediate results is known to be flawed, either in pure bash or with expect (send "R\rR\rR\rR\rQ\r").
I hope this is not a openssl s_client regression.
I will prepare a paranoid patch to wait for the establishment of the TLS session before issuing the R command even in the single 'R' case for you to test. It could be the exhibit of a local openssl initialization corner case.

@drwetter
Copy link
Owner

@Tazmaniac : Thanks a lot for taking care! Love that 👍

I would suggest though that we need to reproduce the problem before patching. If we try to handle all not-really-possible or not-possible cases it's not good for the code base. There's enough legacy stuff already

@Tazmaniac
Copy link

@prabhakaran-motorq : you really have a problem on your server/LB side
I put in place a quick/dirty paranoid version of testssl.sh and while testing it targeting your server, I reproduce multiple times a successful renego from openssl point of view, without server side disconnect:

/tmp/testssl.pWO5WK/52.252.235.54.run_renego.errorlog
depth=2 C = US, ST = New Jersey, L = Jersey City, O = The USERTRUST Network, CN = USERTrust RSA Certification Authority
verify return:1
depth=1 C = GB, ST = Greater Manchester, L = Salford, O = Sectigo Limited, CN = Sectigo RSA Domain Validation Secure Server CA
verify return:1
depth=0 CN = *.motorq.com
verify return:1
RENEGOTIATING
DONE
tail -n 10 /tmp/testssl.pWO5WK/52.252.235.54.run_renego.txt
    12c0 - dd 2c bb c6 ac 3d 70 1b-71 5e f1 c2 b5 75 13 27   .,...=p.q^...u.'
    12d0 - 90 ed 2d 2a 8f c2 5b 42-57 d6 8b 6c 38 09 3f ed   ..-*..[BW..l8.?.
    12e0 - f0 1d 53 d1 d9 02 8c fb-cb e8 93 34 26 4a f3 da   ..S........4&J..
    12f0 - a3 72                                             .r

    Start Time: 1730299478
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
    Extended master secret: yes
---

Correct behavior:

cat /tmp/testssl.xk8bEJ/52.252.235.54.run_renego.errorlog
depth=2 C = US, ST = New Jersey, L = Jersey City, O = The USERTRUST Network, CN = USERTrust RSA Certification Authority
verify return:1
depth=1 C = GB, ST = Greater Manchester, L = Salford, O = Sectigo Limited, CN = Sectigo RSA Domain Validation Secure Server CA
verify return:1
depth=0 CN = *.motorq.com
verify return:1
RENEGOTIATING
tail -n 10 /tmp/testssl.xk8bEJ/52.252.235.54.run_renego.txt
    12d0 - 1e d5 15 d5 44 f2 e3 56-60 26 de b5 80 43 c5 bb   ....D..V`&...C..
    12e0 - d6 d7 58 55 82 f7 f0 4f-33 dd 2b 0c 79 66 66 69   ..XU...O3.+.yffi
    12f0 - e9                                                .

    Start Time: 1730300037
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
    Extended master secret: yes
---
closed

@prabhakaran-motorq
Copy link
Author

Hi @Tazmaniac, thanks for taking the time to go over this.

Could you share the paranoid version of testssl.sh to me, please?

I reproduce multiple times a successful renego

Also, during your testing were you able to do multiple renegotiations initiated by the client in a single connection?

Also, are you able to reproduce this renegotiation when you manually run the openssl (vulnerable version) s_client and typing out 'R' via stdin?

@Tazmaniac
Copy link

Also, during your testing were you able to do multiple renegotiations initiated by the client in a single connection?
No, because it is only an acceptable mitigation on https for old client certificate authentication scheme specific to http protocol context. For the others protocols, renegociation should be disabled.

Also, are you able to reproduce this renegotiation when you manually run the openssl (vulnerable version) s_client and typing out 'R' via stdin?

No, but it give me insight, we have another not handled openssl s_client corner case:
The equivalent testssl openssl command is : echo R |openssl s_client -no_tls1_3 -connect 52.252.235.54:9094 -servername ashu-kafka-broker-0.motorq.com -no_comp -cipher @SECLEVEL=0:ALL:COMPLEMENTOFALL
With THIS command I could easily reproduce the

    12f0 - 82 51                                             .Q

    Start Time: 1730711040
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
    Extended master secret: yes
---
RENEGOTIATING
DONE

pattern.

But with (echo R; sleep 1) |openssl s_client -no_tls1_3 -connect 52.252.235.54:9094 -servername ashu-kafka-broker-0.motorq.com -no_comp -cipher @SECLEVEL=0:ALL:COMPLEMENTOFALL never.

In the general http case, we stay in interactive mode, STDIN being closed at the end of the test or by the server. So we could not hit this race/bug. Adding SSL_RENEG_WAIT (0.25s by default) seems to be sufficient to close the race window in your case, but as a quick safe fix I will put a 1s delay if @drwetter is ok.

The proper fix is to do a big cleanup, merging the second test with the http one (with ssl_reneg_attempts=2) as the big http loop is properly taking care of all possible case, waiting for the server result without closing prematurely STDIN.
As a bonus, we will get complete tracking of the session state ("paranoid" behavior) issuing the renegotiation only when the SSL session is completely "stable", without additional code and faster http test (one less openssl renego connection).

But for this fix, I will need time to do it carefully with proper testing.

Tazmaniac pushed a commit to Tazmaniac/testssl.sh that referenced this issue Nov 4, 2024
Proper fix need another refactoring/cleanup of the renego test.
@Tazmaniac Tazmaniac mentioned this issue Nov 4, 2024
12 tasks
Tazmaniac pushed a commit to Tazmaniac/testssl.sh that referenced this issue Nov 4, 2024
Proper fix need another refactoring/cleanup of the renego test.
Tazmaniac pushed a commit to Tazmaniac/testssl.sh that referenced this issue Nov 4, 2024
Proper fix need another refactoring/cleanup of the renego test.
Tazmaniac pushed a commit to Tazmaniac/testssl.sh that referenced this issue Nov 4, 2024
Proper fix need another refactoring/cleanup of the renego test.
@Tazmaniac Tazmaniac mentioned this issue Nov 4, 2024
12 tasks
@Tazmaniac
Copy link

@prabhakaran-motorq you can test https://github.com/Tazmaniac/testssl.sh/tree/client-renego-refactoring which contain the proper fix / cleanup and fix a bad interaction between --openssl-timeout and the renego loop.

@prabhakaran-motorq
Copy link
Author

@Tazmaniac thanks for the working on this.

When I initially found this issue, I tried sleep 1, but even in that case, I still got "DONE" very rarely. Especially in my MacBook M1 Pro (15.0.1 (24A348)) but that worked fine in my Linux machine, that is why I resorted to expect.

image

I tried this fix (https://github.com/Tazmaniac/testssl.sh/tree/client-renego-refactoring) in my MacBook
image

in my Linux VM
image

Our customer partners generally test using testssl.sh on their local system before sending it to their security team. So even with this fix, sometimes it may classify it as vulnerable.

@Tazmaniac
Copy link

Tazmaniac commented Nov 4, 2024

You did not use the correct branch.
Switch to the client-renego-refactoring before use.

You shoud see

  testssl.sh version 3.2rc3 from https://testssl.sh/dev/
  (7625422 2024-11-04 21:02:03)

as the version (latest commit).

@prabhakaran-motorq
Copy link
Author

Switch to the client-renego-refactoring before use.

Thanks for pointing it out, client-renego-refactoring branch works. Thanks for the quick fix.

image

@Tazmaniac
Copy link

Please do a git pull to test the latest version (git tag 7625422 2024-11-04 21:02:03).

Today I will do more testing and submit a PR if all is OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:to be reproduced ... from maintainers waiting for more input User needs to give more information
Projects
None yet
Development

No branches or pull requests

3 participants