Skip to content

Commit 1415407

Browse files
Lukasanormanmaurer
Lukasa
and
normanmaurer
committed
Don't loop over TLS records for SNI (netty#7479)
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 <[email protected]>
1 parent 4596f9e commit 1415407

File tree

1 file changed

+107
-125
lines changed

1 file changed

+107
-125
lines changed

‎handler/src/main/java/io/netty/handler/ssl/AbstractSniHandler.java‎

Lines changed: 107 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,6 @@
4242
*/
4343
public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder implements ChannelOutboundHandler {
4444

45-
// Maximal number of ssl records to inspect before fallback to the default SslContext.
46-
private static final int MAX_SSL_RECORDS = 4;
47-
4845
private static final InternalLogger logger =
4946
InternalLoggerFactory.getInstance(AbstractSniHandler.class);
5047

@@ -55,83 +52,75 @@ public abstract class AbstractSniHandler<T> extends ByteToMessageDecoder impleme
5552
@Override
5653
protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
5754
if (!suppressRead && !handshakeFailed) {
58-
final int writerIndex = in.writerIndex();
5955
try {
60-
loop:
61-
for (int i = 0; i < MAX_SSL_RECORDS; i++) {
62-
final int readerIndex = in.readerIndex();
63-
final int readableBytes = writerIndex - readerIndex;
64-
if (readableBytes < SslUtils.SSL_RECORD_HEADER_LENGTH) {
65-
// Not enough data to determine the record type and length.
66-
return;
67-
}
56+
final int readerIndex = in.readerIndex();
57+
final int readableBytes = in.readableBytes();
58+
if (readableBytes < SslUtils.SSL_RECORD_HEADER_LENGTH) {
59+
// Not enough data to determine the record type and length.
60+
return;
61+
}
6862

69-
final int command = in.getUnsignedByte(readerIndex);
70-
71-
// tls, but not handshake command
72-
switch (command) {
73-
case SslUtils.SSL_CONTENT_TYPE_CHANGE_CIPHER_SPEC:
74-
case SslUtils.SSL_CONTENT_TYPE_ALERT:
75-
final int len = SslUtils.getEncryptedPacketLength(in, readerIndex);
76-
77-
// Not an SSL/TLS packet
78-
if (len == SslUtils.NOT_ENCRYPTED) {
79-
handshakeFailed = true;
80-
NotSslRecordException e = new NotSslRecordException(
81-
"not an SSL/TLS record: " + ByteBufUtil.hexDump(in));
82-
in.skipBytes(in.readableBytes());
83-
ctx.fireUserEventTriggered(new SniCompletionEvent(e));
84-
SslUtils.handleHandshakeFailure(ctx, e, true);
85-
throw e;
86-
}
87-
if (len == SslUtils.NOT_ENOUGH_DATA ||
88-
writerIndex - readerIndex - SslUtils.SSL_RECORD_HEADER_LENGTH < len) {
89-
// Not enough data
63+
final int command = in.getUnsignedByte(readerIndex);
64+
switch (command) {
65+
case SslUtils.SSL_CONTENT_TYPE_CHANGE_CIPHER_SPEC:
66+
// fall-through
67+
case SslUtils.SSL_CONTENT_TYPE_ALERT:
68+
final int len = SslUtils.getEncryptedPacketLength(in, readerIndex);
69+
70+
// Not an SSL/TLS packet
71+
if (len == SslUtils.NOT_ENCRYPTED) {
72+
handshakeFailed = true;
73+
NotSslRecordException e = new NotSslRecordException(
74+
"not an SSL/TLS record: " + ByteBufUtil.hexDump(in));
75+
in.skipBytes(in.readableBytes());
76+
ctx.fireUserEventTriggered(new SniCompletionEvent(e));
77+
SslUtils.handleHandshakeFailure(ctx, e, true);
78+
throw e;
79+
}
80+
if (len == SslUtils.NOT_ENOUGH_DATA) {
81+
// Not enough data
82+
return;
83+
}
84+
// SNI can't be present in an ALERT or CHANGE_CIPHER_SPEC record, so we'll fall back and assume
85+
// no SNI is present. Let's let the actual TLS implementation sort this out.
86+
break;
87+
88+
case SslUtils.SSL_CONTENT_TYPE_HANDSHAKE:
89+
final int majorVersion = in.getUnsignedByte(readerIndex + 1);
90+
// SSLv3 or TLS
91+
if (majorVersion == 3) {
92+
final int packetLength = in.getUnsignedShort(readerIndex + 3) +
93+
SslUtils.SSL_RECORD_HEADER_LENGTH;
94+
95+
if (readableBytes < packetLength) {
96+
// client hello incomplete; try again to decode once more data is ready.
9097
return;
9198
}
92-
// increase readerIndex and try again.
93-
in.skipBytes(len);
94-
continue;
95-
case SslUtils.SSL_CONTENT_TYPE_HANDSHAKE:
96-
final int majorVersion = in.getUnsignedByte(readerIndex + 1);
97-
98-
// SSLv3 or TLS
99-
if (majorVersion == 3) {
100-
final int packetLength = in.getUnsignedShort(readerIndex + 3) +
101-
SslUtils.SSL_RECORD_HEADER_LENGTH;
102-
103-
if (readableBytes < packetLength) {
104-
// client hello incomplete; try again to decode once more data is ready.
105-
return;
106-
}
107-
108-
// See https://tools.ietf.org/html/rfc5246#section-7.4.1.2
109-
//
110-
// Decode the ssl client hello packet.
111-
// We have to skip bytes until SessionID (which sum to 43 bytes).
112-
//
113-
// struct {
114-
// ProtocolVersion client_version;
115-
// Random random;
116-
// SessionID session_id;
117-
// CipherSuite cipher_suites<2..2^16-2>;
118-
// CompressionMethod compression_methods<1..2^8-1>;
119-
// select (extensions_present) {
120-
// case false:
121-
// struct {};
122-
// case true:
123-
// Extension extensions<0..2^16-1>;
124-
// };
125-
// } ClientHello;
126-
//
127-
128-
final int endOffset = readerIndex + packetLength;
129-
int offset = readerIndex + 43;
130-
131-
if (endOffset - offset < 6) {
132-
break loop;
133-
}
13499

100+
// See https://tools.ietf.org/html/rfc5246#section-7.4.1.2
101+
//
102+
// Decode the ssl client hello packet.
103+
// We have to skip bytes until SessionID (which sum to 43 bytes).
104+
//
105+
// struct {
106+
// ProtocolVersion client_version;
107+
// Random random;
108+
// SessionID session_id;
109+
// CipherSuite cipher_suites<2..2^16-2>;
110+
// CompressionMethod compression_methods<1..2^8-1>;
111+
// select (extensions_present) {
112+
// case false:
113+
// struct {};
114+
// case true:
115+
// Extension extensions<0..2^16-1>;
116+
// };
117+
// } ClientHello;
118+
//
119+
120+
final int endOffset = readerIndex + packetLength;
121+
int offset = readerIndex + 43;
122+
123+
if (endOffset - offset >= 6) {
135124
final int sessionIdLength = in.getUnsignedByte(offset);
136125
offset += sessionIdLength + 1;
137126

@@ -145,68 +134,61 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
145134
offset += 2;
146135
final int extensionsLimit = offset + extensionsLength;
147136

148-
if (extensionsLimit > endOffset) {
149-
// Extensions should never exceed the record boundary.
150-
break loop;
151-
}
152-
153-
for (;;) {
154-
if (extensionsLimit - offset < 4) {
155-
break loop;
156-
}
157-
158-
final int extensionType = in.getUnsignedShort(offset);
159-
offset += 2;
160-
161-
final int extensionLength = in.getUnsignedShort(offset);
162-
offset += 2;
163-
164-
if (extensionsLimit - offset < extensionLength) {
165-
break loop;
166-
}
137+
// Extensions should never exceed the record boundary.
138+
if (extensionsLimit <= endOffset) {
139+
while (extensionsLimit - offset >= 4) {
140+
final int extensionType = in.getUnsignedShort(offset);
141+
offset += 2;
167142

168-
// SNI
169-
// See https://tools.ietf.org/html/rfc6066#page-6
170-
if (extensionType == 0) {
143+
final int extensionLength = in.getUnsignedShort(offset);
171144
offset += 2;
172-
if (extensionsLimit - offset < 3) {
173-
break loop;
174-
}
175145

176-
final int serverNameType = in.getUnsignedByte(offset);
177-
offset++;
146+
if (extensionsLimit - offset < extensionLength) {
147+
break;
148+
}
178149

179-
if (serverNameType == 0) {
180-
final int serverNameLength = in.getUnsignedShort(offset);
150+
// SNI
151+
// See https://tools.ietf.org/html/rfc6066#page-6
152+
if (extensionType == 0) {
181153
offset += 2;
182-
183-
if (extensionsLimit - offset < serverNameLength) {
184-
break loop;
154+
if (extensionsLimit - offset < 3) {
155+
break;
185156
}
186157

187-
final String hostname = in.toString(offset, serverNameLength,
188-
CharsetUtil.US_ASCII);
189-
190-
try {
191-
select(ctx, hostname.toLowerCase(Locale.US));
192-
} catch (Throwable t) {
193-
PlatformDependent.throwException(t);
158+
final int serverNameType = in.getUnsignedByte(offset);
159+
offset++;
160+
161+
if (serverNameType == 0) {
162+
final int serverNameLength = in.getUnsignedShort(offset);
163+
offset += 2;
164+
165+
if (extensionsLimit - offset < serverNameLength) {
166+
break;
167+
}
168+
169+
final String hostname =
170+
in.toString(offset, serverNameLength, CharsetUtil.US_ASCII);
171+
try {
172+
select(ctx, hostname.toLowerCase(Locale.US));
173+
} catch (Throwable t) {
174+
PlatformDependent.throwException(t);
175+
}
176+
return;
177+
} else {
178+
// invalid enum value
179+
break;
194180
}
195-
return;
196-
} else {
197-
// invalid enum value
198-
break loop;
199181
}
200-
}
201182

202-
offset += extensionLength;
183+
offset += extensionLength;
184+
}
203185
}
204186
}
205-
// Fall-through
206-
default:
207-
//not tls, ssl or application data, do not try sni
208-
break loop;
209-
}
187+
}
188+
break;
189+
default:
190+
//not tls, ssl or application data, do not try sni
191+
break;
210192
}
211193
} catch (NotSslRecordException e) {
212194
// Just rethrow as in this case we also closed the channel and this is consistent with SslHandler.

0 commit comments

Comments
 (0)