Skip to content

Commit

Permalink
Ensure we not try to call select when the AbstractSniHandler was …
Browse files Browse the repository at this point in the history
…already removed from the pipeline.

Motivation:

We tried to call `select` after we closed the channel (and so removed all the handlers from the pipeline) when we detected a non SSL record. This would cause an exception like this:

```
Caused by: java.util.NoSuchElementException: io.netty.handler.ssl.SniHandler
	at io.netty.channel.DefaultChannelPipeline.getContextOrDie(DefaultChannelPipeline.java:1098)
	at io.netty.channel.DefaultChannelPipeline.replace(DefaultChannelPipeline.java:506)
	at io.netty.handler.ssl.SniHandler.replaceHandler(SniHandler.java:133)
	at io.netty.handler.ssl.SniHandler.onLookupComplete(SniHandler.java:113)
	at io.netty.handler.ssl.AbstractSniHandler.select(AbstractSniHandler.java:225)
	at io.netty.handler.ssl.AbstractSniHandler.decode(AbstractSniHandler.java:218)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:489)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:428)
	... 40 more
```

Modifications:

- Ensure we rethrow the NotSslRecordException when detecting it (and closing the channel). This will also ensure we not call `select(...)`
- Not catch `Throwable` but only `Exception`
- Add test case.

Result:

Correctly handle the case of an non SSL record.
  • Loading branch information
normanmaurer committed Dec 8, 2017
1 parent 805ac00 commit 1453f8d
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
// SSLv3 or TLS
if (majorVersion == 3) {
final int packetLength = in.getUnsignedShort(readerIndex + 3) +
SslUtils.SSL_RECORD_HEADER_LENGTH;
SslUtils.SSL_RECORD_HEADER_LENGTH;

if (readableBytes < packetLength) {
// client hello incomplete; try again to decode once more data is ready.
Expand Down Expand Up @@ -185,7 +185,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
}

final String hostname = in.toString(offset, serverNameLength,
CharsetUtil.US_ASCII);
CharsetUtil.US_ASCII);

try {
select(ctx, hostname.toLowerCase(Locale.US));
Expand All @@ -208,7 +208,10 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
break loop;
}
}
} catch (Throwable e) {
} catch (NotSslRecordException e) {
// Just rethrow as in this case we also closed the channel and this is consistent with SslHandler.
throw e;
} catch (Exception e) {
// unexpected encoding, ignore sni and use default
if (logger.isDebugEnabled()) {
logger.debug("Unexpected client hello packet: " + ByteBufUtil.hexDump(in), e);
Expand Down
36 changes: 36 additions & 0 deletions handler/src/test/java/io/netty/handler/ssl/SniHandlerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,42 @@ public SniHandlerTest(SslProvider provider) {
this.provider = provider;
}

@Test
public void testNonSslRecord() throws Exception {
SslContext nettyContext = makeSslContext(provider, false);
try {
final AtomicReference<SslHandshakeCompletionEvent> evtRef =
new AtomicReference<SslHandshakeCompletionEvent>();
SniHandler handler = new SniHandler(new DomainNameMappingBuilder<SslContext>(nettyContext).build());
EmbeddedChannel ch = new EmbeddedChannel(handler, new ChannelInboundHandlerAdapter() {
@Override
public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
if (evt instanceof SslHandshakeCompletionEvent) {
assertTrue(evtRef.compareAndSet(null, (SslHandshakeCompletionEvent) evt));
}
}
});

try {
byte[] bytes = new byte[1024];
bytes[0] = SslUtils.SSL_CONTENT_TYPE_ALERT;

try {
ch.writeInbound(Unpooled.wrappedBuffer(bytes));
fail();
} catch (DecoderException e) {
assertTrue(e.getCause() instanceof NotSslRecordException);
}
assertFalse(ch.finish());
} finally {
ch.finishAndReleaseAll();
}
assertTrue(evtRef.get().cause() instanceof NotSslRecordException);
} finally {
releaseAll(nettyContext);
}
}

@Test
public void testServerNameParsing() throws Exception {
SslContext nettyContext = makeSslContext(provider, false);
Expand Down

0 comments on commit 1453f8d

Please sign in to comment.