andreacavalli
/
netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Remove remote initiated renegotiation support 

Motivation:
We recently removed support for renegotiation, but there are still some hooks to attempt to allow remote initiated renegotiation to succeed. The remote initated renegotiation can be even more problematic from a security stand point and should also be removed.

Modifications:
- Remove state related to remote iniated renegotiation from OpenSslEngine

Result:
More renegotiation code removed from the OpenSslEngine code path.
This commit is contained in:
Scott Mitchell 2018-01-02 10:10:13 -08:00 committed by Norman Maurer
parent d34a930e4b
commit ccecc20124
5 changed files with 67 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java
View File

@ -75,21 +75,6 @@ import static io.netty.util.internal.ObjectUtil.checkPositiveOrZero;
public abstract class ReferenceCountedOpenSslContext extends SslContext implements ReferenceCounted {
private static final InternalLogger logger =
InternalLoggerFactory.getInstance(ReferenceCountedOpenSslContext.class);
/**
* To make it easier for users to replace JDK implementation with OpenSsl version we also use
* {@code jdk.tls.rejectClientInitiatedRenegotiation} to allow disabling client initiated renegotiation.
* Java8+ uses this system property as well.
* <p>
* See also <a href="http://blog.ivanristic.com/2014/03/ssl-tls-improvements-in-java-8.html">
* Significant SSL/TLS improvements in Java 8</a>
*/
private static final boolean JDK_REJECT_CLIENT_INITIATED_RENEGOTIATION =
AccessController.doPrivileged(new PrivilegedAction<Boolean>() {
@Override
public Boolean run() {
return SystemPropertyUtil.getBoolean("jdk.tls.rejectClientInitiatedRenegotiation", false);
}
});
private static final int DEFAULT_BIO_NON_APPLICATION_BUFFER_SIZE =
AccessController.doPrivileged(new PrivilegedAction<Integer>() {
@ -140,7 +125,6 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
final OpenSslEngineMap engineMap = new DefaultOpenSslEngineMap();
final ReadWriteLock ctxLock = new ReentrantReadWriteLock();
private volatile boolean rejectRemoteInitiatedRenegotiation;
private volatile int bioNonApplicationBufferSize = DEFAULT_BIO_NON_APPLICATION_BUFFER_SIZE;
@SuppressWarnings("deprecation")
@ -221,10 +205,6 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
this.protocols = protocols;
this.enableOcsp = enableOcsp;
if (mode == SSL.SSL_MODE_SERVER) {
rejectRemoteInitiatedRenegotiation =
JDK_REJECT_CLIENT_INITIATED_RENEGOTIATION;
}
this.keyCertChain = keyCertChain == null ? null : keyCertChain.clone();
unmodifiableCiphers = Arrays.asList(checkNotNull(cipherFilter, "cipherFilter").filterCipherSuites(
@ -422,18 +402,24 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
}
/**
* {@deprecated Renegotiation is not supported}
* Specify if remote initiated renegotiation is supported or not. If not supported and the remote side tries
* to initiate a renegotiation a {@link SSLHandshakeException} will be thrown during decoding.
*/
@Deprecated
public void setRejectRemoteInitiatedRenegotiation(boolean rejectRemoteInitiatedRenegotiation) {
this.rejectRemoteInitiatedRenegotiation = rejectRemoteInitiatedRenegotiation;
if (!rejectRemoteInitiatedRenegotiation) {
throw new UnsupportedOperationException("Renegotiation is not supported");
}
}
/**
* Returns if remote initiated renegotiation is supported or not.
* {@deprecated Renegotiation is not supported}
* @return {@code true} because renegotiation is not supported.
*/
@Deprecated
public boolean getRejectRemoteInitiatedRenegotiation() {
return rejectRemoteInitiatedRenegotiation;
return true;
}
/**

4
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
View File

@ -203,7 +203,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
private final ByteBufAllocator alloc;
private final OpenSslEngineMap engineMap;
private final OpenSslApplicationProtocolNegotiator apn;
private final boolean rejectRemoteInitiatedRenegotiation;
private final OpenSslSession session;
private final Certificate[] localCerts;
private final ByteBuffer[] singleSrcBuffer = new ByteBuffer[1];
@ -238,7 +237,6 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
session = new OpenSslSession(context.sessionContext());
clientMode = context.isClient();
engineMap = context.engineMap;
rejectRemoteInitiatedRenegotiation = context.getRejectRemoteInitiatedRenegotiation();
localCerts = context.keyCertChain;
keyMaterialManager = context.keyMaterialManager();
enableOcsp = context.enableOcsp;
@ -1101,7 +1099,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
// As rejectRemoteInitiatedRenegotiation() is called in a finally block we also need to check if we shutdown
// the engine before as otherwise SSL.getHandshakeCount(ssl) will throw an NPE if the passed in ssl is 0.
// See https://github.com/netty/netty/issues/7353
if (rejectRemoteInitiatedRenegotiation && !isDestroyed() && SSL.getHandshakeCount(ssl) > 1) {
if (!isDestroyed() && SSL.getHandshakeCount(ssl) > 1) {
// TODO: In future versions me may also want to send a fatal_alert to the client and so notify it
// that the renegotiation failed.
shutdown();

46
handler/src/test/java/io/netty/handler/ssl/OpenSslRenegotiateTest.java Normal file
View File

@ -0,0 +1,46 @@
/*
* Copyright 2017 The Netty Project
*
* The Netty Project licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package io.netty.handler.ssl;
import org.junit.BeforeClass;
import java.util.concurrent.atomic.AtomicReference;
import javax.net.ssl.SSLException;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.junit.Assume.assumeTrue;
public class OpenSslRenegotiateTest extends RenegotiateTest {
@BeforeClass
public static void checkOpenSsl() {
assumeTrue(OpenSsl.isAvailable());
}
@Override
protected SslProvider serverSslProvider() {
return SslProvider.OPENSSL;
}
protected void verifyResult(AtomicReference<Throwable> error) throws Throwable {
Throwable cause = error.get();
// Renegotation is not supported by the OpenSslEngine.
assertThat(cause, is(instanceOf(SSLException.class)));
}
}

12
handler/src/test/java/io/netty/handler/ssl/RenegotiateTest.java
View File

@ -47,7 +47,6 @@ public abstract class RenegotiateTest {
try {
final SslContext context = SslContextBuilder.forServer(cert.key(), cert.cert())
.sslProvider(serverSslProvider()).build();
initSslServerContext(context);
ServerBootstrap sb = new ServerBootstrap();
sb.group(group).channel(LocalServerChannel.class)
.childHandler(new ChannelInitializer<Channel>() {
@ -125,10 +124,7 @@ public abstract class RenegotiateTest {
latch.await();
clientChannel.close().syncUninterruptibly();
channel.close().syncUninterruptibly();
Throwable cause = error.get();
if (cause != null) {
throw cause;
}
verifyResult(error);
} finally {
group.shutdownGracefully();
}
@ -136,6 +132,10 @@ public abstract class RenegotiateTest {
protected abstract SslProvider serverSslProvider();
protected void initSslServerContext(SslContext context) {
protected void verifyResult(AtomicReference<Throwable> error) throws Throwable {
Throwable cause = error.get();
if (cause != null) {
throw cause;
}
}
}

8
testsuite/src/main/java/io/netty/testsuite/transport/socket/SocketSslClientRenegotiateTest.java
View File

@ -40,7 +40,6 @@ import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
import javax.net.ssl.SSLHandshakeException;
import java.io.File;
import java.nio.channels.ClosedChannelException;
import java.security.cert.CertificateException;
@ -49,7 +48,11 @@ import java.util.Collection;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;
import static org.junit.Assert.*;
import javax.net.ssl.SSLHandshakeException;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@RunWith(Parameterized.class)
public class SocketSslClientRenegotiateTest extends AbstractSocketTest {
@ -79,7 +82,6 @@ public class SocketSslClientRenegotiateTest extends AbstractSocketTest {
boolean hasOpenSsl = OpenSsl.isAvailable();
if (hasOpenSsl) {
OpenSslServerContext context = new OpenSslServerContext(CERT_FILE, KEY_FILE);
context.setRejectRemoteInitiatedRenegotiation(true);
serverContexts.add(context);
} else {
logger.warn("OpenSSL is unavailable and thus will not be tested.", OpenSsl.unavailabilityCause());