Skip to content

Commit

Permalink
OpenSslEngine: Remove renegotiation support
Browse files Browse the repository at this point in the history
Motivation:

SSL.setState() has gone from openssl 1.1. Calling it is, and probably
always has been, incorrect. Doing renogitation in this manner is
potentially insecure. There have been at least two insecure
renegotiation vulnerabilities in users of the OpenSSL library.

Renegotiation is not necessary for correct operation of the TLS protocol.

BoringSSL has already eliminated this functionality, and the tests
(now deleted) were not running on BoringSSL.

Modifications:

If the connection setup has completed, always return that
negotiation is not supported. Previously this was done only if we were
the client.

Remove the tests for this functionality.

Fixes netty#6320.
  • Loading branch information
FauxFaux authored and Scottmitch committed Jan 2, 2018
1 parent 2b4f667 commit e24e06b
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 108 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ private enum HandshakeState {
}

private HandshakeState handshakeState = HandshakeState.NOT_STARTED;
private boolean renegotiationPending;
private boolean receivedShutdown;
private volatile int destroyed;
private volatile String applicationProtocol;
Expand Down Expand Up @@ -654,17 +653,6 @@ public final SSLEngineResult wrap(

status = handshake();

if (renegotiationPending && status == FINISHED) {
// If renegotiationPending is true that means when we attempted to start renegotiation
// the BIO buffer didn't have enough space to hold the HelloRequest which prompts the
// client to initiate a renegotiation. At this point the HelloRequest has been written
// so we can actually start the handshake process.
renegotiationPending = false;
SSL.setState(ssl, SSL.SSL_ST_ACCEPT);
handshakeState = HandshakeState.STARTED_EXPLICITLY;
status = handshake();
}

// Handshake may have generated more data, for example if the internal SSL buffer is small
// we may have freed up space by flushing above.
bytesProduced = bioLengthBefore - SSL.bioLengthByteBuffer(networkBIO);
Expand Down Expand Up @@ -1510,43 +1498,7 @@ public final synchronized void beginHandshake() throws SSLException {
// Nothing to do as the handshake is not done yet.
break;
case FINISHED:
if (clientMode) {
// Only supported for server mode at the moment.
throw RENEGOTIATION_UNSUPPORTED;
}
// For renegotiate on the server side we need to issue the following command sequence with openssl:
//
// SSL_renegotiate(ssl)
// SSL_do_handshake(ssl)
// ssl->state = SSL_ST_ACCEPT
// SSL_do_handshake(ssl)
//
// Because of this we fall-through to call handshake() after setting the state, as this will also take
// care of updating the internal OpenSslSession object.
//
// See also:
// https://github.com/apache/httpd/blob/2.4.16/modules/ssl/ssl_engine_kernel.c#L812
// http://h71000.www7.hp.com/doc/83final/ba554_90007/ch04s03.html
int status;
if ((status = SSL.renegotiate(ssl)) != 1 || (status = SSL.doHandshake(ssl)) != 1) {
int err = SSL.getError(ssl, status);
if (err == SSL.SSL_ERROR_WANT_READ || err == SSL.SSL_ERROR_WANT_WRITE) {
// If the internal SSL buffer is small it is possible that doHandshake may "fail" because
// there is not enough room to write, so we should wait until the renegotiation has been.
renegotiationPending = true;
handshakeState = HandshakeState.STARTED_EXPLICITLY;
lastAccessed = System.currentTimeMillis();
return;
} else {
throw shutdownWithError("renegotiation failed");
}
}

SSL.setState(ssl, SSL.SSL_ST_ACCEPT);

lastAccessed = System.currentTimeMillis();

// fall-through
throw RENEGOTIATION_UNSUPPORTED;
case NOT_STARTED:
handshakeState = HandshakeState.STARTED_EXPLICITLY;
handshake();
Expand Down

This file was deleted.

This file was deleted.

0 comments on commit e24e06b

Please sign in to comment.