Jelajahi Sumber

Do not allow safelisted media types on Content-Type (#83448)

The commit to allow custom media types validation accidentally allowed for CSRF.
The rules for media types which should be rejected on content-type were mentioned here

PR that introduced a bug: #80906
the comment that describes safelisted media types to be rejected #80482 (comment)
Przemyslaw Gomulka 3 tahun lalu
induk
melakukan
a2949f5305

+ 5 - 0
docs/changelog/83448.yaml

@@ -0,0 +1,5 @@
+pr: 83448
+summary: Do not allow safelisted media types on Content-Type
+area: Infra/REST API
+type: bug
+issues: []

+ 18 - 1
server/src/main/java/org/elasticsearch/rest/RestController.java

@@ -59,6 +59,12 @@ public class RestController implements HttpServerTransport.Dispatcher {
 
     private static final Logger logger = LogManager.getLogger(RestController.class);
     private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestController.class);
+    /**
+     * list of browser safelisted media types - not allowed on Content-Type header
+     * https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+     */
+    static final Set<String> SAFELISTED_MEDIA_TYPES = Set.of("application/x-www-form-urlencoded", "multipart/form-data", "text/plain");
+
     static final String ELASTIC_PRODUCT_HTTP_HEADER = "X-elastic-product";
     static final String ELASTIC_PRODUCT_HTTP_HEADER_VALUE = "Elasticsearch";
 
@@ -334,7 +340,7 @@ public class RestController implements HttpServerTransport.Dispatcher {
         throws Exception {
         final int contentLength = request.contentLength();
         if (contentLength > 0) {
-            if (handler.mediaTypesValid(request) == false) {
+            if (isContentTypeDisallowed(request) || handler.mediaTypesValid(request) == false) {
                 sendContentTypeErrorMessage(request.getAllHeaderValues("Content-Type"), channel);
                 return;
             }
@@ -388,6 +394,17 @@ public class RestController implements HttpServerTransport.Dispatcher {
         }
     }
 
+    /**
+     * in order to prevent CSRF we have to reject all media types that are from a browser safelist
+     * see https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+     * see https://www.elastic.co/blog/strict-content-type-checking-for-elasticsearch-rest-requests
+     * @param request
+     */
+    private boolean isContentTypeDisallowed(RestRequest request) {
+        return request.getParsedContentType() != null
+            && SAFELISTED_MEDIA_TYPES.contains(request.getParsedContentType().mediaTypeWithoutParameters());
+    }
+
     private boolean handleNoHandlerFound(String rawPath, RestRequest.Method method, String uri, RestChannel channel) {
         // Get the map of matching handlers for a request, for the full set of HTTP methods.
         final Set<RestRequest.Method> validMethodSet = getValidHandlerMethodSet(rawPath);

+ 21 - 0
server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

@@ -800,6 +800,27 @@ public class RestControllerTests extends ESTestCase {
         assertTrue(channel.getSendResponseCalled());
     }
 
+    public void testBrowserSafelistedContentTypesAreRejected() {
+        RestController restController = new RestController(Collections.emptySet(), null, client, circuitBreakerService, usageService);
+
+        final String mediaType = randomFrom(RestController.SAFELISTED_MEDIA_TYPES);
+        FakeRestRequest fakeRestRequest = requestWithContent(mediaType);
+
+        final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.NOT_ACCEPTABLE);
+
+        restController.registerHandler(new Route(GET, "/foo"), new RestHandler() {
+            @Override
+            public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {}
+        });
+
+        restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
+        assertTrue(channel.getSendResponseCalled());
+        assertThat(
+            channel.getRestResponse().content().utf8ToString(),
+            containsString("Content-Type header [" + mediaType + "] is not supported")
+        );
+    }
+
     private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport {
 
         TestHttpServerTransport() {}