From bbf65a9f9180c8fa4c5d572e387b8ceb9a32c3ef Mon Sep 17 00:00:00 2001 From: Jonas Konrad Date: Fri, 15 Dec 2023 15:06:29 +0100 Subject: [PATCH] Fix logged errors reported by fuzzing (#10273) * Fix logged errors reported by fuzzing There are two bugs fixed here: (1) Empty URI handling in HateoasErrorResponseProcessor ``` 10:18:18.775 [main] ERROR i.m.h.s.netty.RoutingInBoundHandler - Micronaut Server Error - No request state present. Cause: URI cannot be empty java.lang.IllegalArgumentException: URI cannot be empty at io.micronaut.http.hateoas.DefaultLink.(DefaultLink.java:49) at io.micronaut.http.hateoas.Link.of(Link.java:115) at io.micronaut.http.server.exceptions.response.HateoasErrorResponseProcessor.processResponse(HateoasErrorResponseProcessor.java:69) at io.micronaut.http.server.RouteExecutor.createDefaultErrorResponse(RouteExecutor.java:214) at io.micronaut.http.server.netty.RoutingInBoundHandler.writeResponse(RoutingInBoundHandler.java:229) at io.micronaut.http.server.netty.NettyRequestLifecycle.lambda$handleException$2(NettyRequestLifecycle.java:147) at io.micronaut.core.execution.ImperativeExecutionFlowImpl.onComplete(ImperativeExecutionFlowImpl.java:132) at io.micronaut.http.server.netty.NettyRequestLifecycle.handleException(NettyRequestLifecycle.java:147) at io.micronaut.http.server.netty.NettyRequestLifecycle.handleNormal(NettyRequestLifecycle.java:89) at io.micronaut.http.server.netty.RoutingInBoundHandler.accept(RoutingInBoundHandler.java:220) [...] ``` Test input added to FuzzyInputSpec. FuzzyInputSpec has been adjusted to recognize logged errors. (2) Bad sorting in MediaType.orderedOf ``` 11:48:58.716 [41560@yawkat-oracle main] ERROR i.m.http.server.RouteExecutor - Unexpected error occurred: Comparison method violates its general contract! java.lang.IllegalArgumentException: Comparison method violates its general contract! at java.base/java.util.TimSort.mergeLo(TimSort.java:781) at java.base/java.util.TimSort.mergeAt(TimSort.java:518) at java.base/java.util.TimSort.mergeForceCollapse(TimSort.java:461) at java.base/java.util.TimSort.sort(TimSort.java:254) at java.base/java.util.Arrays.sort(Arrays.java:1307) at java.base/java.util.ArrayList.sort(ArrayList.java:1721) at io.micronaut.http.MediaType.orderedOf(MediaType.java:870) at io.micronaut.http.netty.NettyHttpHeaders.accept(NettyHttpHeaders.java:301) ``` Created a new MediaTypeFuzzTest that hits this issue. The sample input (crash-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe) reproduces the issue and will be run as part of the normal micronaut-http test suite now. Setting the env variable JAZZER_FUZZ=1 will enable exploratory fuzzing. (3) Added workaround for https://github.com/netty/netty/pull/13730 This does not appear to be an issue in the real world, only with EmbeddedChannel. I've added a workaround for the issue so that it doesn't trigger my fuzz tests anymore. --- None of these bugs appear security-relevant, (3) is not applicable outside a test env, (1) and (2) only produce additional error logs. * annotation --- gradle/libs.versions.toml | 3 ++ .../http/server/netty/NettyHttpServer.java | 31 +++++++++++++++++- .../server/netty/fuzzing/FlagAppender.java | 24 ++++++++++++++ .../netty/fuzzing/FuzzyInputSpec.groovy | 6 ++++ .../src/test/resources/logback.xml | 9 +++++ .../HateoasErrorResponseProcessor.java | 6 +++- http/build.gradle | 1 + .../java/io/micronaut/http/MediaType.java | 8 +++-- .../io/micronaut/http/MediaTypeFuzzTest.java | 18 ++++++++++ ...h-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe | Bin 0 -> 156 bytes 10 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FlagAppender.java create mode 100644 http/src/test/java/io/micronaut/http/MediaTypeFuzzTest.java create mode 100644 http/src/test/resources/io/micronaut/http/MediaTypeFuzzTestInputs/crash-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index eeee522017a..ea86e5c635d 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -27,6 +27,7 @@ jsr107 = "1.1.1" jsr305 = "3.0.2" jakarta-el = "5.0.1" jakarta-el-impl = "5.0.0-M1" +jazzer = "0.22.1" jcache = "1.1.1" junit5 = "5.9.3" junit-platform="1.9.3" @@ -206,6 +207,8 @@ junit-jupiter-params = { module = "org.junit.jupiter:junit-jupiter-params", vers junit-platform-engine = { module = "org.junit.platform:junit-platform-suite-engine", version.ref = "junit-platform" } junit-vintage = { module = "org.junit.vintage:junit-vintage-engine", version.ref = "junit5" } +jazzer-junit = { module = "com.code-intelligence:jazzer-junit", version.ref = "jazzer" } + jetbrains-annotations = { module = "org.jetbrains:annotations", version.ref = "jetbrains-annotations" } kotlin-kotest-junit5 = { module = "io.kotest:kotest-runner-junit5-jvm" } diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpServer.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpServer.java index 61008ff44e3..85ee45a870e 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpServer.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpServer.java @@ -53,10 +53,13 @@ import io.netty.bootstrap.Bootstrap; import io.netty.bootstrap.ServerBootstrap; import io.netty.channel.Channel; +import io.netty.channel.ChannelDuplexHandler; import io.netty.channel.ChannelFuture; +import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelInitializer; import io.netty.channel.ChannelOption; import io.netty.channel.ChannelPipeline; +import io.netty.channel.ChannelPromise; import io.netty.channel.EventLoopGroup; import io.netty.channel.embedded.EmbeddedChannel; import io.netty.channel.group.ChannelGroup; @@ -768,7 +771,33 @@ private HttpPipelineBuilder createPipelineBuilder(NettyServerCustomizer customiz */ @Internal public EmbeddedChannel buildEmbeddedChannel(boolean ssl) { - EmbeddedChannel channel = new EmbeddedChannel(); + EmbeddedChannel channel = new EmbeddedChannel(new ChannelDuplexHandler() { + // work around https://github.com/netty/netty/pull/13730 + + boolean reading = false; + ChannelPromise closePromise; + + @Override + public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg) throws Exception { + reading = true; + ctx.fireChannelRead(msg); + reading = false; + ChannelPromise closePromise = this.closePromise; + if (closePromise != null) { + this.closePromise = null; + ctx.close(closePromise); + } + } + + @Override + public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exception { + if (reading) { + closePromise = promise; + } else { + ctx.close(promise); + } + } + }); buildEmbeddedChannel(channel, ssl); return channel; } diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FlagAppender.java b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FlagAppender.java new file mode 100644 index 00000000000..76e78a47f88 --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FlagAppender.java @@ -0,0 +1,24 @@ +package io.micronaut.http.server.netty.fuzzing; + +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.AppenderBase; + +public class FlagAppender extends AppenderBase { + private static volatile boolean triggered = false; + + public static void clear() { + triggered = false; + } + + public static void checkTriggered() { + if (triggered) { + triggered = false; + throw new RuntimeException("Log message recorded, failing."); + } + } + + @Override + protected void append(ILoggingEvent eventObject) { + triggered = true; + } +} diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FuzzyInputSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FuzzyInputSpec.groovy index cb2896e38e7..7fd6f6f3ea0 100644 --- a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FuzzyInputSpec.groovy +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FuzzyInputSpec.groovy @@ -73,6 +73,7 @@ class FuzzyInputSpec extends Specification { def 'http1 cleartext embedded channel'() { given: + FlagAppender.clear() BufferLeakDetection.startTracking() ApplicationContext ctx = ApplicationContext.run([ @@ -97,10 +98,15 @@ class FuzzyInputSpec extends Specification { embeddedChannel.checkException() BufferLeakDetection.stopTrackingAndReportLeaks() + FlagAppender.checkTriggered() where: input << [ Base64.decoder.decode("RyAqIFAvMS4xCmNvbnRlbnQtbGVuZ3RoOjQKCg1JT05TIC8gUC8xLjEKClMgLyBQLzEuMQpjb250ZW50LWxlbmd0aDo0CgoNZ3BJUyAvIFQvMS43CgpQT1NUIC8gUC8xLjEKY29udGVudC1sZW5ndGg6NAoKDUlPTlMgLyBILzEuMQpjb250ZW50LWxlbmd0aDo0Cgo="), + Base64.decoder.decode("VCAvLy4gSFRUUC8xLjEKCg=="), + Base64.decoder.decode("T1BUSU9OUyAqIEhUVFAvMS4xCgogKiBIVFQxCgo="), + Base64.decoder.decode("SEVBRCAvIEhUVFAvMS4xCkhvc3Q6IGFHRVQgLyBIVFRQpC8xLjEKSG9zdDogYXBpLmJvbpKSkpKSkpKSkpL//////////////////////////3d3dy07biE9ZQoKUE9TVCAvIABUVFAvMS4zCmNvbnRlbnQtdHlwZTp0R09HR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHRy9HR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR7nBR0dHR0dHOxkZGRkZGRkZGW49bkdHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHR0dHRztePWY7kpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpJUVFAvMS4zCmNvbnRlbnQtdHlwZTp0R0dHR0dHR0dHR0dHR0dHR29uOiBCYXNpYyBYWFgvLyBUVFAvMS4xCgo="), + Base64.decoder.decode("UE9TVCAvIEhUVFAvMS40NApBY2NlcHQ6IGFwcNTU1NTU1NTU1NTU1NTU0NTU////////////////////////////////Z2dnZy4xCkhvc3Q6IDQKQWNjZXB0OiAqLyoKdC5ldToyNDQ0CkFjY2VwdDogYXBwbGljYXRQT1NUIC8g///////////U1NTU1NTU1NTU1NTU1NTU0NTU////////////////////////////////////////Z2dnZy4xCkhvc3Q6IDQKQWNjZXB0OiAqLyoKdC5ldToyNDQ0CkFjY2VwdDogYXBwbGljYXRQT1NUIEdFVCAvIC8g/////////////////////yT/YXBw1NTU1NTU1NTU1NTU1NTQ1NT///////////////////////////////9nZ2dnLjEKSG9zdDogNApBY2NlcHQ6ICovKgp0LmV1OjI0NDQKQUhvc3Q6IDQKQWNjZXB0OiAqL3B0OiBhcHDU1NTU1NTU1NTU1NTU1NDU1P///////////////////////////////2dnZ2cuMQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCAvIP//////////1NTU1NTU1NTU1NTU1NTU1NDU1P///////////////////////////////////////2dnZ2cuMQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCBHRVQgLyAvIP////////////////////8k/2FwcNTU1NTU1NTU1NTU1NTU0NTU////////////////////////////////Z2dnZy4xCkhvc3Q6IDQKQWNjZXB0OiAqLyoKdC5ldToyNDQ0CkFIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCAvIP////////////////////8k/////////////////////////9TU1E9QVP//////////////Z2dnZy4xCkhvc3Q6IDQKQWNjZXB0OiAqVCAvIP///////yoKdC5ldToyNDQ0CkFjY2VwdDogYXBwbGljYXRQT1NUIC8g/////////////////////yT/////////////////////////1NTUT1BU//////////////9nZ2dnLjEKSG9zdDogNApBY2NlcHQ6ICovKgp0LmV1OjI0NDQKQWNjZXB0OiBldToyNDQ0CkFjY2VwdDogYXAscGxpY2F0UE9TVCAvIP////////////////////8k/////////////////////////9TU1E9QVElPTsQz1NTU1C8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCAvIP//MQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwLHBsaWNhdFBPU1QgLyD/////////////////////JP/////////////////////////U1NRPUFRJT07EM9TU1NTU1NTU1GFwaS5iZS1wcm9qZWN0LmV1OjQ0MwpBY2NlcHQ6IGFwcGxpY2F0UE9TVCgvIP//Ki8qCnQuZXU6MjQ0NApBY2NlcHQ6IGH////////////////////////U1NRPUFT//////////////2dnZ2cuMQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCAvIP//MQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UCD/////////////////////JP/////////////////////////U1NRPUFT//////////////2dnZ2cuMQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCAvIP//MQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwLHBsaWNhdFBPU1QgLyD/////////////////////JP/////////////////////////U1NRPUFRJT07EM9TU1NTU1NTU1GFwaS5iZS1wcm9qZWN0LmV1OjQ0MwpBY2NlcHQ6IGFwcGxpY2F0UE9TVCgvIP//Ki8qCnQuZXU6MjQ0NApBY2NlcHQ6IGH////////////////////////U1NRPUFT//////////////2dnZ2cuMQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjQ0NApBY2NlcHQ6IGFwcGxpY2F0UE9TVCAvIP//MQpIb3N0OiA0CkFjY2VwdDogKi8qCnQuZXU6MjT///9nZ2dnLjEKSG9zdDogNApBY2NlcHQ6ICovKgp0LmV1OjI0NDQKQWNjZXB0OiBhcHBsaWNhdFBPU1QgLyD//////////9TU1NTU1NTU1NTU1NTU1NTQ1NT//////////w=="), ] } diff --git a/http-server-netty/src/test/resources/logback.xml b/http-server-netty/src/test/resources/logback.xml index f592cc4fb43..e315d7867e6 100644 --- a/http-server-netty/src/test/resources/logback.xml +++ b/http-server-netty/src/test/resources/logback.xml @@ -8,8 +8,17 @@ + + + WARN + ACCEPT + DENY + + + + diff --git a/http-server/src/main/java/io/micronaut/http/server/exceptions/response/HateoasErrorResponseProcessor.java b/http-server/src/main/java/io/micronaut/http/server/exceptions/response/HateoasErrorResponseProcessor.java index de7a8e2b893..007182c9160 100644 --- a/http-server/src/main/java/io/micronaut/http/server/exceptions/response/HateoasErrorResponseProcessor.java +++ b/http-server/src/main/java/io/micronaut/http/server/exceptions/response/HateoasErrorResponseProcessor.java @@ -66,7 +66,11 @@ public MutableHttpResponse processResponse(@NonNull ErrorContext erro } error.embedded("errors", errors); } - error.link(Link.SELF, Link.of(errorContext.getRequest().getUri())); + try { + error.link(Link.SELF, Link.of(errorContext.getRequest().getUri())); + } catch (IllegalArgumentException ignored) { + // invalid URI, don't include it + } return response.body(error).contentType(MediaType.APPLICATION_JSON_TYPE); } diff --git a/http/build.gradle b/http/build.gradle index 3e0525827bd..1f6dc0a12d1 100644 --- a/http/build.gradle +++ b/http/build.gradle @@ -21,6 +21,7 @@ dependencies { testImplementation project(":inject") testImplementation project(":runtime") testImplementation(libs.logback.classic) + testImplementation(libs.jazzer.junit) } tasks.named("compileKotlin") { diff --git a/http/src/main/java/io/micronaut/http/MediaType.java b/http/src/main/java/io/micronaut/http/MediaType.java index c50d1b72193..19cb7baabc9 100644 --- a/http/src/main/java/io/micronaut/http/MediaType.java +++ b/http/src/main/java/io/micronaut/http/MediaType.java @@ -869,9 +869,13 @@ public static List orderedOf(List values) { } mediaTypes.sort((o1, o2) -> { //The */* type is always last - if (o1.type.equals("*")) { + boolean fullWildcard1 = o1.type.equals("*"); + boolean fullWildcard2 = o2.type.equals("*"); + if (fullWildcard1 && fullWildcard2) { + return 0; + } else if (fullWildcard1) { return 1; - } else if (o2.type.equals("*")) { + } else if (fullWildcard2) { return -1; } if (o2.subtype.equals("*") && !o1.subtype.equals("*")) { diff --git a/http/src/test/java/io/micronaut/http/MediaTypeFuzzTest.java b/http/src/test/java/io/micronaut/http/MediaTypeFuzzTest.java new file mode 100644 index 00000000000..00b2c4ee317 --- /dev/null +++ b/http/src/test/java/io/micronaut/http/MediaTypeFuzzTest.java @@ -0,0 +1,18 @@ +package io.micronaut.http; + +import com.code_intelligence.jazzer.api.FuzzedDataProvider; +import com.code_intelligence.jazzer.junit.FuzzTest; + +import java.util.ArrayList; +import java.util.List; + +public class MediaTypeFuzzTest { + @FuzzTest + public void orderedOf(FuzzedDataProvider input) { + List strings = new ArrayList<>(); + while (input.remainingBytes() > 0 && strings.size() < 128) { + strings.add(input.consumeString(32)); + } + MediaType.orderedOf(strings); + } +} diff --git a/http/src/test/resources/io/micronaut/http/MediaTypeFuzzTestInputs/crash-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe b/http/src/test/resources/io/micronaut/http/MediaTypeFuzzTestInputs/crash-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe new file mode 100644 index 0000000000000000000000000000000000000000..db8d1593a5ef0f69d4362b320bbfc12b8dc9892e GIT binary patch literal 156 zcmdPW*V55XOSAUUSJ2Vd(b5lM08;wsd>ybXx+n@Cs5OB>M@L_aK|dC#T1)@G_G5@P hhzwX%mw|x+ECaO+qzs}qS%C#&KPy-(1B0@H4gl@IA;JIv literal 0 HcmV?d00001