Skip to content

Close sockets on receiving exceptions during SSL runHandshake phase.#53

Open
suvidya wants to merge 1 commit intowildfly-security:mainfrom
suvidya:wildfly-openssl-vw
Open

Close sockets on receiving exceptions during SSL runHandshake phase.#53
suvidya wants to merge 1 commit intowildfly-security:mainfrom
suvidya:wildfly-openssl-vw

Conversation

@suvidya
Copy link

@suvidya suvidya commented Oct 23, 2018

Without this, it may leak sockets that hold the connection until the server decides to kill it based on its idle channel configuration.

Possible reasons this may arise are data mangling over the wire, bad certificate fingerprint,etc - these issues result in SSL handshake failure at the 'unwrap' phase of OpenSSLEngine, which gets thrown all the way up without the OpenSSLSocket getting a chance to close it gracefully.

If enough sockets are leaked, it may result in serious issues like denial of service.

Without this, it may leak sockets that hold the connection until the server decides to kill it based on its idle channel configuration.
Possible reasons this may arise are data mangling over the wire, bad certificate fingerprint,etc - these issues result in SSL handshake failure at the 'unwrap' phase of OpenSSLEngine, which gets thrown all the way up without the OpenSSLSocket getting a chance to close it gracefully.

If enough sockets are leaked, it may result in serious issues like denial of service.
@suvidya
Copy link
Author

suvidya commented Nov 1, 2018

Is there a way to close this socket in RFC 5246 compliant way? It appears that with this fix, the socket gets closed, but the client doesn't send a handshake failure alert back to the server.

If Server FIN handshake message is mangled, the client should send a fatal alert upon receiving the message back to the server. That's not happening with this fix.

@stuartwdouglas / @jamezp / @jaikiran : any help will be appreciated.

Details about the test:

e) Modify a byte in the Server Finished handshake message, and verify that the client sends a fatal alert upon receipt and does not send any application data.-

Observations with the fix in this PR:

  1. Channel is closed with error error:1408F119:SSL routines:SSL3_GET_RECORD:decryption failed or bad record mac
  2. However, according to the Wireshark, the system appears to complete the handshake successfully.
    wireshark
  3. It looks like the test server failed to send an RFC-compliant TLS Alert message, which is required

@jaikiran
Copy link
Contributor

jaikiran commented Nov 2, 2018

@suvidya, is there a simple test case/example which reproduces the original issue which prompted this fix? That will help a bit in reproducing this quickly and thinking of a fix.

@suvidya
Copy link
Author

suvidya commented Nov 2, 2018

Thanks for responding @jaikiran. I have unit tests but they are catered towards my application code than wildfly-openssl's. I will try to articulate the test in a later comment. But I can see that the fix in this PR is closing the socket where it is supposed to (logging correct errors from the native library) but not sending the 'TLS alert' back to the peer (as described below the wireshark snippet above).

I have found a post showing strikingly similar behavior/bug (in terms of sending the TLS alert issue) that was fixed in netty. Please see: netty/netty#3900.. However the leaking socket issue still remains, which I believe is fixed in part by this PR.

This looks like a legit bug. Do you mind mapping the behavior in netty to wildfly? I'm a user of wildfly-openssl, don't quite fully understand the source or the native code.

So basically I'm chasing 2 potential bugs here:

  1. Underlying socket remains open in client mode when invalid cert is received from the server (This can be tested by mangling/corrupting the signature on the server certificate that is sent back). This is "fixed" in part by this PR.

  2. This forcible closing the socket, doesn't send 'TLS alert' message back a TLS alert to the peer to cleanly fail the handshake (there's some glue that is missing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants