Fix logged errors reported by fuzzing (micronaut-projects#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.<init>(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 netty/netty#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
altro3 · Dec 15, 2023 · bbf65a9 · bbf65a9
1 parent 68bd0a1
commit bbf65a9
Show file tree

Hide file tree

Showing 10 changed files with 102 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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<ILoggingEvent> {
+    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/...server-netty/src/test/groovy/io/micronaut/http/server/netty/fuzzing/FuzzyInputSpec.groovy b/...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
@@ -8,8 +8,17 @@
         </encoder>
     </appender>
 
+    <appender name="TRIGGER_FAILURE" class="io.micronaut.http.server.netty.fuzzing.FlagAppender">
+        <filter class="ch.qos.logback.classic.filter.ThresholdFilter">
+            <level>WARN</level>
+            <onMatch>ACCEPT</onMatch>
+            <onMismatch>DENY</onMismatch>
+        </filter>
+    </appender>
+
     <root level="info">
         <appender-ref ref="STDOUT" />
+        <appender-ref ref="TRIGGER_FAILURE" />
     </root>
 <!--    <logger name="io.micronaut.http" level="TRACE"/>-->
     <!--<logger name="io.micronaut.websocket" level="TRACE"/>-->

diff --git a/...main/java/io/micronaut/http/server/exceptions/response/HateoasErrorResponseProcessor.java b/...main/java/io/micronaut/http/server/exceptions/response/HateoasErrorResponseProcessor.java
@@ -66,7 +66,11 @@ public MutableHttpResponse<JsonError> 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
@@ -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
@@ -869,9 +869,13 @@ public static List<MediaType> orderedOf(List<? extends CharSequence> 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
@@ -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<String> strings = new ArrayList<>();
+        while (input.remainingBytes() > 0 && strings.size() < 128) {
+            strings.add(input.consumeString(32));
+        }
+        MediaType.orderedOf(strings);
+    }
+}
diff --git a/.../io/micronaut/http/MediaTypeFuzzTestInputs/crash-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe b/.../io/micronaut/http/MediaTypeFuzzTestInputs/crash-22df7f6e72bba86bdb1fdbd4bf92372fd4fa6bbe