From 543c0661916dddd7ca3ce5d91cee853d070bc324 Mon Sep 17 00:00:00 2001 From: lutovich Date: Thu, 29 Mar 2018 11:57:32 +0200 Subject: [PATCH] Attach only valid HTTP headers in CORS filter Filter receives `Access-Control-Request-Headers` request header (usually send with OPTIONS HTTP request) and attaches all specified headers to `Access-Control-Allow-Headers` response header. This commit makes filter validate all provided headers and only attach valid ones. --- .../org/neo4j/server/rest/web/CorsFilter.java | 21 ++++++++-- .../org/neo4j/server/web/HttpHeaderUtils.java | 29 ++++++++++++++ .../neo4j/server/rest/web/CorsFilterTest.java | 12 ++++-- .../neo4j/server/web/HttpHeaderUtilsTest.java | 40 ++++++++++++++++--- 4 files changed, 91 insertions(+), 11 deletions(-) diff --git a/community/server/src/main/java/org/neo4j/server/rest/web/CorsFilter.java b/community/server/src/main/java/org/neo4j/server/rest/web/CorsFilter.java index ed084c1b77b8b..8a9d9c74c88cd 100644 --- a/community/server/src/main/java/org/neo4j/server/rest/web/CorsFilter.java +++ b/community/server/src/main/java/org/neo4j/server/rest/web/CorsFilter.java @@ -34,6 +34,8 @@ import org.neo4j.logging.LogProvider; import org.neo4j.server.web.HttpMethod; +import static org.neo4j.server.web.HttpHeaderUtils.isValidHttpHeaderName; + /** * This filter adds the header "Access-Control-Allow-Origin : *" to all * responses that goes through it. This allows modern browsers to do cross-site @@ -84,7 +86,7 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp while ( requestHeaderEnumeration.hasMoreElements() ) { String requestHeader = requestHeaderEnumeration.nextElement(); - response.addHeader( ACCESS_CONTROL_ALLOW_HEADERS, requestHeader ); + addAllowedHeaderIfValid( requestHeader, response ); } } @@ -99,14 +101,27 @@ public void destroy() private void addAllowedMethodIfValid( String methodName, HttpServletResponse response ) { HttpMethod method = HttpMethod.valueOfOrNull( methodName ); - if ( method == null ) + if ( method != null ) + { + response.addHeader( ACCESS_CONTROL_ALLOW_METHODS, methodName ); + } + else { log.warn( "Unknown HTTP method specified in " + ACCESS_CONTROL_REQUEST_METHOD + " '" + methodName + "'. " + "It will be ignored and not attached to the " + ACCESS_CONTROL_ALLOW_METHODS + " response header" ); } + } + + private void addAllowedHeaderIfValid( String headerName, HttpServletResponse response ) + { + if ( isValidHttpHeaderName( headerName ) ) + { + response.addHeader( ACCESS_CONTROL_ALLOW_HEADERS, headerName ); + } else { - response.addHeader( ACCESS_CONTROL_ALLOW_METHODS, methodName ); + log.warn( "Invalid HTTP header specified in " + ACCESS_CONTROL_REQUEST_HEADERS + " '" + headerName + "'. " + + "It will be ignored and not attached to the " + ACCESS_CONTROL_ALLOW_HEADERS + " response header" ); } } } diff --git a/community/server/src/main/java/org/neo4j/server/web/HttpHeaderUtils.java b/community/server/src/main/java/org/neo4j/server/web/HttpHeaderUtils.java index 07faffaad012f..61446a9a68b8e 100644 --- a/community/server/src/main/java/org/neo4j/server/web/HttpHeaderUtils.java +++ b/community/server/src/main/java/org/neo4j/server/web/HttpHeaderUtils.java @@ -87,4 +87,33 @@ public static long getTransactionTimeout( HttpServletRequest request, Log errorL } return GraphDatabaseSettings.UNSPECIFIED_TIMEOUT; } + + /** + * Validates given HTTP header name. Does not allow blank names and names with control characters, like '\n' (LF) and '\r' (CR). + * Can be used to detect and neutralize CRLF in HTTP headers. + * + * @param name the HTTP header name, like 'Accept' or 'Content-Type'. + * @return {@code true} when given name represents a valid HTTP header, {@code false} otherwise. + */ + public static boolean isValidHttpHeaderName( String name ) + { + if ( name == null || name.length() == 0 ) + { + return false; + } + boolean isBlank = true; + for ( int i = 0; i < name.length(); i++ ) + { + char c = name.charAt( i ); + if ( Character.isISOControl( c ) ) + { + return false; + } + if ( !Character.isWhitespace( c ) ) + { + isBlank = false; + } + } + return !isBlank; + } } diff --git a/community/server/src/test/java/org/neo4j/server/rest/web/CorsFilterTest.java b/community/server/src/test/java/org/neo4j/server/rest/web/CorsFilterTest.java index a488709fd07be..04beae536a391 100644 --- a/community/server/src/test/java/org/neo4j/server/rest/web/CorsFilterTest.java +++ b/community/server/src/test/java/org/neo4j/server/rest/web/CorsFilterTest.java @@ -122,16 +122,22 @@ public void shouldAttachNoRequestHeadersToAccessControlAllowHeadersWhenHeaderIsN } @Test - public void shouldAttachRequestHeadersToAccessControlAllowHeaders() throws Exception + public void shouldAttachValidRequestHeadersToAccessControlAllowHeaders() throws Exception { - List accessControlRequestHeaders = asList( "Accept", "Content-Length", "Content-Type" ); + List accessControlRequestHeaders = asList( "Accept", "X-Wrong\nHeader", "Content-Type", "Accept\r", "Illegal\r\nHeader", "", null, " " ); HttpServletRequest request = requestMock( emptyList(), accessControlRequestHeaders ); filter.doFilter( request, response, chain ); verify( response ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Accept" ); - verify( response ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Content-Length" ); verify( response ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Content-Type" ); + + verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, null ); + verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "" ); + verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, " " ); + verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "X-Wrong\nHeader" ); + verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Accept\r" ); + verify( response, never() ).addHeader( ACCESS_CONTROL_ALLOW_HEADERS, "Illegal\r\nHeader" ); } private static HttpServletRequest requestMock() diff --git a/community/server/src/test/java/org/neo4j/server/web/HttpHeaderUtilsTest.java b/community/server/src/test/java/org/neo4j/server/web/HttpHeaderUtilsTest.java index 7f6f9c13d0f6f..40a08c25e1317 100644 --- a/community/server/src/test/java/org/neo4j/server/web/HttpHeaderUtilsTest.java +++ b/community/server/src/test/java/org/neo4j/server/web/HttpHeaderUtilsTest.java @@ -19,6 +19,7 @@ */ package org.neo4j.server.web; +import org.apache.http.HttpHeaders; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -30,7 +31,12 @@ import org.neo4j.logging.Log; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; +import static org.neo4j.server.web.HttpHeaderUtils.MAX_EXECUTION_TIME_HEADER; +import static org.neo4j.server.web.HttpHeaderUtils.getTransactionTimeout; +import static org.neo4j.server.web.HttpHeaderUtils.isValidHttpHeaderName; public class HttpHeaderUtilsTest { @@ -47,9 +53,9 @@ public void setUp() @Test public void retrieveCustomTransactionTimeout() { - when( request.getHeader( HttpHeaderUtils.MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "100" ); + when( request.getHeader( MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "100" ); Log log = logProvider.getLog( HttpServletRequest.class ); - long transactionTimeout = HttpHeaderUtils.getTransactionTimeout( request, log ); + long transactionTimeout = getTransactionTimeout( request, log ); assertEquals( "Transaction timeout should be retrieved.", 100, transactionTimeout ); logProvider.assertNoLoggingOccurred(); } @@ -58,7 +64,7 @@ public void retrieveCustomTransactionTimeout() public void defaultValueWhenCustomTransactionTimeoutNotSpecified() { Log log = logProvider.getLog( HttpServletRequest.class ); - long transactionTimeout = HttpHeaderUtils.getTransactionTimeout( request, log ); + long transactionTimeout = getTransactionTimeout( request, log ); assertEquals( "Transaction timeout not specified.", 0, transactionTimeout ); logProvider.assertNoLoggingOccurred(); } @@ -66,11 +72,35 @@ public void defaultValueWhenCustomTransactionTimeoutNotSpecified() @Test public void defaultValueWhenCustomTransactionTimeoutNotANumber() { - when( request.getHeader( HttpHeaderUtils.MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "aa" ); + when( request.getHeader( MAX_EXECUTION_TIME_HEADER ) ).thenReturn( "aa" ); Log log = logProvider.getLog( HttpServletRequest.class ); - long transactionTimeout = HttpHeaderUtils.getTransactionTimeout( request, log ); + long transactionTimeout = getTransactionTimeout( request, log ); assertEquals( "Transaction timeout not specified.", 0, transactionTimeout ); logProvider.assertContainsMessageContaining("Fail to parse `max-execution-time` " + "header with value: 'aa'. Should be a positive number."); } + + @Test + public void shouldCheckHttpHeaders() + { + assertFalse( isValidHttpHeaderName( null ) ); + assertFalse( isValidHttpHeaderName( "" ) ); + assertFalse( isValidHttpHeaderName( " " ) ); + assertFalse( isValidHttpHeaderName( " " ) ); + assertFalse( isValidHttpHeaderName( " \r " ) ); + assertFalse( isValidHttpHeaderName( " \r\n\t " ) ); + + assertTrue( isValidHttpHeaderName( HttpHeaders.ACCEPT ) ); + assertTrue( isValidHttpHeaderName( HttpHeaders.ACCEPT_ENCODING ) ); + assertTrue( isValidHttpHeaderName( HttpHeaders.AGE ) ); + assertTrue( isValidHttpHeaderName( HttpHeaders.CONTENT_ENCODING ) ); + assertTrue( isValidHttpHeaderName( HttpHeaders.EXPIRES ) ); + assertTrue( isValidHttpHeaderName( HttpHeaders.IF_MATCH ) ); + assertTrue( isValidHttpHeaderName( HttpHeaders.TRANSFER_ENCODING ) ); + assertTrue( isValidHttpHeaderName( "Weird Header With Spaces" ) ); + + assertFalse( isValidHttpHeaderName( "My\nHeader" ) ); + assertFalse( isValidHttpHeaderName( "Other\rStrange-Header" ) ); + assertFalse( isValidHttpHeaderName( "Header-With-Tab\t" ) ); + } }