Correctly calculate and respect if we can correctly fullfil wrap for alerts. (#7945)
Motivation: We previously did not correctly take into account when we could not wrap (and so produce) the full SSL record with an alert when the SSLEngine was closed. There are two problems here: - If we call wrap(...) with an empty dst buffer after closeOutbound() was called we will not notify the user if we could not store the whole SSLRecord into the dst buffer and so we may produce incomplete SSLRecords Modifications: Add unit test which failed before. Result: Correctly handle the case when the dst buffer is not big enough and and alert needs to be produced.
This commit is contained in:
parent
47985c11c1
commit
546ddd2c28
@ -607,8 +607,17 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
|
||||
|
||||
int bioLengthBefore = SSL.bioLengthByteBuffer(networkBIO);
|
||||
|
||||
// Explicit use outboundClosed as we want to drain any bytes that are still present.
|
||||
// Explicitly use outboundClosed as we want to drain any bytes that are still present.
|
||||
if (outboundClosed) {
|
||||
// If the outbound was closed we want to ensure we can produce the alert to the destination buffer.
|
||||
// This is true even if we not using jdkCompatibilityMode.
|
||||
//
|
||||
// We use a plaintextLength of 2 as we at least want to have an alert fit into it.
|
||||
// https://tools.ietf.org/html/rfc5246#section-7.2
|
||||
if (!isBytesAvailableEnoughForWrap(dst.remaining(), 2, 1)) {
|
||||
return new SSLEngineResult(BUFFER_OVERFLOW, getHandshakeStatus(), 0, 0);
|
||||
}
|
||||
|
||||
// There is something left to drain.
|
||||
// See https://github.com/netty/netty/issues/6260
|
||||
bytesProduced = SSL.bioFlushByteBuffer(networkBIO);
|
||||
|
@ -23,7 +23,6 @@ import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
|
||||
import io.netty.handler.ssl.util.SelfSignedCertificate;
|
||||
import io.netty.internal.tcnative.SSL;
|
||||
import io.netty.util.internal.PlatformDependent;
|
||||
import org.junit.Assert;
|
||||
import org.junit.Assume;
|
||||
import org.junit.BeforeClass;
|
||||
import org.junit.Test;
|
||||
@ -401,6 +400,68 @@ public class OpenSslEngineTest extends SSLEngineTest {
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testCorrectlyCalculateSpaceForAlert() throws Exception {
|
||||
testCorrectlyCalculateSpaceForAlert(true);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testCorrectlyCalculateSpaceForAlertJDKCompatabilityModeOff() throws Exception {
|
||||
testCorrectlyCalculateSpaceForAlert(false);
|
||||
}
|
||||
|
||||
private void testCorrectlyCalculateSpaceForAlert(boolean jdkCompatabilityMode) throws Exception {
|
||||
SelfSignedCertificate ssc = new SelfSignedCertificate();
|
||||
serverSslCtx = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
|
||||
.sslProvider(sslServerProvider())
|
||||
.build();
|
||||
|
||||
clientSslCtx = SslContextBuilder.forClient()
|
||||
.trustManager(InsecureTrustManagerFactory.INSTANCE)
|
||||
.sslProvider(sslClientProvider())
|
||||
.build();
|
||||
SSLEngine clientEngine = null;
|
||||
SSLEngine serverEngine = null;
|
||||
try {
|
||||
if (jdkCompatabilityMode) {
|
||||
clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
|
||||
serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
|
||||
} else {
|
||||
clientEngine = wrapEngine(clientSslCtx.newHandler(UnpooledByteBufAllocator.DEFAULT).engine());
|
||||
serverEngine = wrapEngine(serverSslCtx.newHandler(UnpooledByteBufAllocator.DEFAULT).engine());
|
||||
}
|
||||
handshake(clientEngine, serverEngine);
|
||||
|
||||
// This should produce an alert
|
||||
clientEngine.closeOutbound();
|
||||
|
||||
ByteBuffer empty = allocateBuffer(0);
|
||||
ByteBuffer dst = allocateBuffer(clientEngine.getSession().getPacketBufferSize());
|
||||
// Limit to something that is guaranteed to be too small to hold a SSL Record.
|
||||
dst.limit(1);
|
||||
|
||||
// As we called closeOutbound() before this should produce a BUFFER_OVERFLOW.
|
||||
SSLEngineResult result = clientEngine.wrap(empty, dst);
|
||||
assertEquals(SSLEngineResult.Status.BUFFER_OVERFLOW, result.getStatus());
|
||||
|
||||
// This must calculate a length that can hold an alert at least (or more).
|
||||
dst.limit(dst.capacity());
|
||||
|
||||
result = clientEngine.wrap(empty, dst);
|
||||
assertEquals(SSLEngineResult.Status.CLOSED, result.getStatus());
|
||||
|
||||
// flip the buffer so we can verify we produced a full length buffer.
|
||||
dst.flip();
|
||||
|
||||
int length = SslUtils.getEncryptedPacketLength(new ByteBuffer[] { dst }, 0);
|
||||
assertEquals(length, dst.remaining());
|
||||
} finally {
|
||||
cleanupClientSslEngine(clientEngine);
|
||||
cleanupServerSslEngine(serverEngine);
|
||||
ssc.delete();
|
||||
}
|
||||
}
|
||||
|
||||
@Override
|
||||
protected void mySetupMutualAuthServerInitSslHandler(SslHandler handler) {
|
||||
ReferenceCountedOpenSslEngine engine = (ReferenceCountedOpenSslEngine) handler.engine();
|
||||
|
Loading…
Reference in New Issue
Block a user