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

Don't loop over TLS records for SNI (#7479)

Browse Source 
Motivation:

The AbstractSniHandler previously was willing to tolerate up to three
non-handshake records before a ClientHello that contained an SNI
extension field. This is, so far as I can tell, completely
unnecessary: no TLS implementation will be sending alerts or change
cipher spec messages before ClientHello.

Given that it was not possible to determine why this loop is in
the code to begin with, it's probably just best to remove it.

Modifications:

Remove the for loop.

Result:

The AbstractSniHandler will more rapidly determine whether it should
pass the records on to the default SSL handler.

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
This commit is contained in:
Cory Benfield 2019-07-01 05:22:55 -04:00 committed by Norman Maurer
parent 4596f9e139
commit 14154074f2
1 changed files with 103 additions and 121 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java
View File

@ -42,9 +42,6 @@ import java.util.Locale;
*/ */
public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder implements ChannelOutboundHandler { public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder implements ChannelOutboundHandler {
// Maximal number of ssl records to inspect before fallback to the default SslContext.
private static final int MAX_SSL_RECORDS = 4;
private static final InternalLogger logger = private static final InternalLogger logger =
InternalLoggerFactory.getInstance(AbstractSniHandler.class); InternalLoggerFactory.getInstance(AbstractSniHandler.class);
@ -55,22 +52,18 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
@Override @Override
protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception { protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
if (!suppressRead && !handshakeFailed) { if (!suppressRead && !handshakeFailed) {
final int writerIndex = in.writerIndex();
try { try {
loop:
for (int i = 0; i < MAX_SSL_RECORDS; i++) {
final int readerIndex = in.readerIndex(); final int readerIndex = in.readerIndex();
final int readableBytes = writerIndex - readerIndex; final int readableBytes = in.readableBytes();
if (readableBytes < SslUtils.SSL_RECORD_HEADER_LENGTH) { if (readableBytes < SslUtils.SSL_RECORD_HEADER_LENGTH) {
// Not enough data to determine the record type and length. // Not enough data to determine the record type and length.
return; return;
} }
final int command = in.getUnsignedByte(readerIndex); final int command = in.getUnsignedByte(readerIndex);
// tls, but not handshake command
switch (command) { switch (command) {
case SslUtils.SSL_CONTENT_TYPE_CHANGE_CIPHER_SPEC: case SslUtils.SSL_CONTENT_TYPE_CHANGE_CIPHER_SPEC:
// fall-through
case SslUtils.SSL_CONTENT_TYPE_ALERT: case SslUtils.SSL_CONTENT_TYPE_ALERT:
final int len = SslUtils.getEncryptedPacketLength(in, readerIndex); final int len = SslUtils.getEncryptedPacketLength(in, readerIndex);
@ -84,17 +77,16 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
SslUtils.handleHandshakeFailure(ctx, e, true); SslUtils.handleHandshakeFailure(ctx, e, true);
throw e; throw e;
} }
if (len == SslUtils.NOT_ENOUGH_DATA || if (len == SslUtils.NOT_ENOUGH_DATA) {
writerIndex - readerIndex - SslUtils.SSL_RECORD_HEADER_LENGTH < len) {
// Not enough data // Not enough data
return; return;
} }
// increase readerIndex and try again. // SNI can't be present in an ALERT or CHANGE_CIPHER_SPEC record, so we'll fall back and assume
in.skipBytes(len); // no SNI is present. Let's let the actual TLS implementation sort this out.
continue; break;
case SslUtils.SSL_CONTENT_TYPE_HANDSHAKE: case SslUtils.SSL_CONTENT_TYPE_HANDSHAKE:
final int majorVersion = in.getUnsignedByte(readerIndex + 1); final int majorVersion = in.getUnsignedByte(readerIndex + 1);
// SSLv3 or TLS // SSLv3 or TLS
if (majorVersion == 3) { if (majorVersion == 3) {
final int packetLength = in.getUnsignedShort(readerIndex + 3) + final int packetLength = in.getUnsignedShort(readerIndex + 3) +
@ -128,10 +120,7 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
final int endOffset = readerIndex + packetLength; final int endOffset = readerIndex + packetLength;
int offset = readerIndex + 43; int offset = readerIndex + 43;
if (endOffset - offset < 6) { if (endOffset - offset >= 6) {
break loop;
}
final int sessionIdLength = in.getUnsignedByte(offset); final int sessionIdLength = in.getUnsignedByte(offset);
offset += sessionIdLength + 1; offset += sessionIdLength + 1;
@ -145,16 +134,9 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
offset += 2; offset += 2;
final int extensionsLimit = offset + extensionsLength; final int extensionsLimit = offset + extensionsLength;
if (extensionsLimit > endOffset) {
// Extensions should never exceed the record boundary. // Extensions should never exceed the record boundary.
break loop; if (extensionsLimit <= endOffset) {
} while (extensionsLimit - offset >= 4) {
for (;;) {
if (extensionsLimit - offset < 4) {
break loop;
}
final int extensionType = in.getUnsignedShort(offset); final int extensionType = in.getUnsignedShort(offset);
offset += 2; offset += 2;
@ -162,7 +144,7 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
offset += 2; offset += 2;
if (extensionsLimit - offset < extensionLength) { if (extensionsLimit - offset < extensionLength) {
break loop; break;
} }
// SNI // SNI
@ -170,7 +152,7 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
if (extensionType == 0) { if (extensionType == 0) {
offset += 2; offset += 2;
if (extensionsLimit - offset < 3) { if (extensionsLimit - offset < 3) {
break loop; break;
} }
final int serverNameType = in.getUnsignedByte(offset); final int serverNameType = in.getUnsignedByte(offset);
@ -181,12 +163,11 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
offset += 2; offset += 2;
if (extensionsLimit - offset < serverNameLength) { if (extensionsLimit - offset < serverNameLength) {
break loop; break;
} }
final String hostname = in.toString(offset, serverNameLength, final String hostname =
CharsetUtil.US_ASCII); in.toString(offset, serverNameLength, CharsetUtil.US_ASCII);
try { try {
select(ctx, hostname.toLowerCase(Locale.US)); select(ctx, hostname.toLowerCase(Locale.US));
} catch (Throwable t) { } catch (Throwable t) {
@ -195,18 +176,19 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
return; return;
} else { } else {
// invalid enum value // invalid enum value
break loop; break;
} }
} }
offset += extensionLength; offset += extensionLength;
} }
} }
// Fall-through }
}
break;
default: default:
//not tls, ssl or application data, do not try sni //not tls, ssl or application data, do not try sni
break loop; break;
}
} }
} catch (NotSslRecordException e) { } catch (NotSslRecordException e) {
// Just rethrow as in this case we also closed the channel and this is consistent with SslHandler. // Just rethrow as in this case we also closed the channel and this is consistent with SslHandler.