Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Requests for static resources trigger a security error during mime type detection #254

Closed
kothar opened this issue Apr 29, 2019 · 7 comments
Labels
Milestone

Comments

@kothar
Copy link

kothar commented Apr 29, 2019

  • Framework version: 1.3
  • Implementations: Spring Boot

Scenario

Hosting static resources (e.g. css or js files) from a /static folder on the classpath.

Expected behavior

The static file is served by the service

Actual behavior

Requests for paths with extensions causes getMimeType to be called on the AwsServletContext class. This in turn calls SecurityUtils.getValidFilePath(String).

The string input to getMimeType is treated as a relative path, which fails the security check.

My understanding is that Spring is passing in the request path rather than it's true location (possibly embedded inside a jar file, or non-existent in the case of API endpoints). Prefixing non-absolute paths with "/tmp/" allows the content type to be correctly probed, although the file does not exist at that path.

Issue #158 resolves a similar problem with the rest container mappings by disabling extension-based mime type detection and avoiding this call completely, but static resources are requested by a different route and not affected by the fix in that issue. Additionally, it is desirable for the mime type detection to work as expected for static files, rather than disabling it.

Steps to reproduce

Request a static resource with a file extension (or a rest path which appears to have a file extension, as per #158)

Full log output

29 Apr 2019 13:07:01,423 ERROR  ErrorPageFilter: Forwarding to error page from request [/favicon.ico] due to exception [File path not allowed: /home/mhouston/<redacted>/lambda-ui/favicon.ico]
java.lang.IllegalArgumentException: File path not allowed: /home/mhouston/<redacted>/lambda-ui/favicon.ico
	at com.amazonaws.serverless.proxy.internal.SecurityUtils.getValidFilePath(SecurityUtils.java:194) ~[aws-serverless-java-container-core-1.3.jar:?]
	at com.amazonaws.serverless.proxy.internal.SecurityUtils.getValidFilePath(SecurityUtils.java:158) ~[aws-serverless-java-container-core-1.3.jar:?]
	at com.amazonaws.serverless.proxy.internal.servlet.AwsServletContext.getMimeType(AwsServletContext.java:146) ~[aws-serverless-java-container-core-1.3.jar:?]
	at org.springframework.web.accept.ServletPathExtensionContentNegotiationStrategy.getMediaTypeForResource(ServletPathExtensionContentNegotiationStrategy.java:97) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.resource.ResourceHttpRequestHandler.getMediaType(ResourceHttpRequestHandler.java:527) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.resource.ResourceHttpRequestHandler.handleRequest(ResourceHttpRequestHandler.java:354) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.mvc.HttpRequestHandlerAdapter.handle(HttpRequestHandlerAdapter.java:51) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:967) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:901) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:970) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:861) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:635) ~[tomcat-embed-core-8.5.15.jar:8.5.15]
	at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:846) ~[spring-webmvc-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:742) ~[tomcat-embed-core-8.5.15.jar:8.5.15]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainManager$ServletExecutionFilter.doFilter(FilterChainManager.java:351) ~[aws-serverless-java-container-core-1.3.jar:?]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainHolder.doFilter(FilterChainHolder.java:84) ~[aws-serverless-java-container-core-1.3.jar:?]
	at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:99) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainHolder.doFilter(FilterChainHolder.java:84) ~[aws-serverless-java-container-core-1.3.jar:?]
	at org.springframework.web.filter.HttpPutFormContentFilter.doFilterInternal(HttpPutFormContentFilter.java:105) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainHolder.doFilter(FilterChainHolder.java:84) ~[aws-serverless-java-container-core-1.3.jar:?]
	at org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:81) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainHolder.doFilter(FilterChainHolder.java:84) ~[aws-serverless-java-container-core-1.3.jar:?]
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:197) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainHolder.doFilter(FilterChainHolder.java:84) ~[aws-serverless-java-container-core-1.3.jar:?]
	at org.springframework.boot.web.support.ErrorPageFilter.doFilter(ErrorPageFilter.java:117) [spring-boot-1.4.7.RELEASE.jar:1.4.7.RELEASE]
	at org.springframework.boot.web.support.ErrorPageFilter.access$000(ErrorPageFilter.java:61) [spring-boot-1.4.7.RELEASE.jar:1.4.7.RELEASE]
	at org.springframework.boot.web.support.ErrorPageFilter$1.doFilterInternal(ErrorPageFilter.java:92) [spring-boot-1.4.7.RELEASE.jar:1.4.7.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) [spring-web-4.3.9.RELEASE.jar:4.3.9.RELEASE]
	at org.springframework.boot.web.support.ErrorPageFilter.doFilter(ErrorPageFilter.java:110) [spring-boot-1.4.7.RELEASE.jar:1.4.7.RELEASE]
	at com.amazonaws.serverless.proxy.internal.servlet.FilterChainHolder.doFilter(FilterChainHolder.java:84) [aws-serverless-java-container-core-1.3.jar:?]
	at com.amazonaws.serverless.proxy.internal.servlet.AwsLambdaServletContainerHandler.doFilter(AwsLambdaServletContainerHandler.java:206) [aws-serverless-java-container-core-1.3.jar:?]
	at com.amazonaws.serverless.proxy.spring.SpringBootLambdaContainerHandler.handleRequest(SpringBootLambdaContainerHandler.java:154) [aws-serverless-java-container-spring-1.3.jar:?]
	at com.amazonaws.serverless.proxy.spring.SpringBootLambdaContainerHandler.handleRequest(SpringBootLambdaContainerHandler.java:52) [aws-serverless-java-container-spring-1.3.jar:?]
	at com.amazonaws.serverless.proxy.internal.LambdaContainerHandler.proxy(LambdaContainerHandler.java:177) [aws-serverless-java-container-core-1.3.jar:?]
	at com.amazonaws.serverless.proxy.internal.LambdaContainerHandler.proxyStream(LambdaContainerHandler.java:209) [aws-serverless-java-container-core-1.3.jar:?]
	at <redacted>.ui.lambda.StreamLambdaHandler.handleRequest(StreamLambdaHandler.java:38) [classes/:?]
	at <redacted>.ui.lambda.StreamLambdaHandlerTest.can_request_static_resource(StreamLambdaHandlerTest.java:44) [test-classes/:?]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_181]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_181]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_181]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_181]
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) [junit-4.12.jar:4.12]
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) [junit-4.12.jar:4.12]
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) [junit-4.12.jar:4.12]
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) [junit-4.12.jar:4.12]
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) [junit-4.12.jar:4.12]
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) [junit-4.12.jar:4.12]
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363) [junit-4.12.jar:4.12]
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137) [junit-4.12.jar:4.12]
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) [junit-rt.jar:?]
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47) [junit-rt.jar:?]
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) [junit-rt.jar:?]
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70) [junit-rt.jar:?]
@sapessi
Copy link
Collaborator

sapessi commented Jun 26, 2019

@kothar any chance you have some sample code I can test with?

@kothar
Copy link
Author

kothar commented Jun 26, 2019

Sure - I have put together a working example: https://github.com/kothar/spring-lambda-test

If you run the spring app directly, you can successfully request both /static.html and /dynamic
The unit test simulates making those requests to the lambda wrapper. Only the dynamic one succeeds.

Running the tests against the patch in #255 succeeds.

@sapessi sapessi added the bug label Jun 26, 2019
@sapessi sapessi added this to the Release 1.3.2 milestone Jun 26, 2019
@sapessi
Copy link
Collaborator

sapessi commented Jun 26, 2019

Thanks. I can make this work in my environment by explicitly enabling the path during the cold start:

LambdaContainerHandler.getContainerConfig()
    .addValidFilePath("/Users/xxx/workspace/aws-serverless-java-container/aws-serverless-java-container-spring");

It's a bit more work but I feel it's the safest way to enable access to static files. Would this not work for you @kothar?

@kothar
Copy link
Author

kothar commented Jun 27, 2019

@sapessi I can confirm that the workaround above works for me. You might want to use

LambdaContainerHandler.getContainerConfig().addValidFilePath(System.getProperty("user.dir"));

in the unit test.

While I'm perfectly happy to use this approach for my application, it seems to me that the fundamental problem is not the valid paths, but the implementation of AWSServletContext.getMimeType(String). The specification for that method says:

    /**
     * Returns the MIME type of the specified file, or <code>null</code> if the
     * MIME type is not known. The MIME type is determined by the configuration
     * of the servlet container, and may be specified in a web application
     * deployment descriptor. Common MIME types are <code>"text/html"</code> and
     * <code>"image/gif"</code>.
     *
     * @param file
     *            a <code>String</code> specifying the name of a file
     * @return a <code>String</code> specifying the file's MIME type
     */

In practice, the file name passed in to the method is not the path of an actual file, but the path of an HTTP request. For static files this is unlikely to be the correct path of the file on disk, and for dynamic requests the file may not even exist, and the type detected may be used to route to the correct handler.

This method is essentially saying: "given a hypothetical file xyz.html, what would the mime type be based on the file extension?".

Requiring that we mark all paths in the application working directory as valid seems unnecessary and potentially a risk if the same security check is used in other places.

My preference would be to keep the security check in place, but to isolate the file system probeContentType to a known (non-existent) path to allow the content type detection to work without custom security exceptions being needed.

That's just my view of the problem, I am happy to leave it as-is if you disagree :)

@sapessi
Copy link
Collaborator

sapessi commented Jun 27, 2019

You make a fair point @kothar and I'd be happy to change the implementation to work how you propose. Looks like that's exactly how Tomcat implements it. Before I go ahead with the change, I'd like to get confirmation that this is the only way the method is used. I cannot find anything explicit in the Servlet specifications. I'll keep googling for now.

sapessi added a commit that referenced this issue Jun 27, 2019
…the mime type from the file name, without probing the file system. Updated unit tests to match (#254)
@sapessi
Copy link
Collaborator

sapessi commented Jun 27, 2019

Release 1.3.2 - which includes this fix - is on its way to maven central! Resolving this issue.

@sapessi sapessi closed this as completed Jun 27, 2019
@kothar
Copy link
Author

kothar commented Jun 29, 2019

Thanks for taking the time research a proper patch @sapessi! That seems a much better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants