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

controllers.AssetsBuilder.resourceNameAt incorrectly looks at local file system #2337

Closed
dylex opened this issue Feb 5, 2014 · 6 comments
Closed

Comments

@dylex
Copy link
Contributor

dylex commented Feb 5, 2014

We ran into a strange situation where controllers.Assets was working on some systems and not others. This is because one function is treating the paths as local filesystem paths rather than resource paths:

Assets.scala:177: if (new File(resourceName).isDirectory || !new File(resourceName).getCanonicalPath.startsWith(new File(path).getCanonicalPath)) {

Elsewhere in Assets, path is correctly treated as a resource path, but here it actually constructs Files out of the raw path (e.g., "/public/foo"). If "/public/foo" happens to exist on the local machine and be a directory, Assets returns 404, even though there is a perfectly good resource called "/public/foo".

@jroper
Copy link
Member

jroper commented Feb 10, 2014

We should probably remove the new File(resourceName).isDirectory. The latter call is there to prevent dot dot attacks (it stops attackers from reading your application.conf file for example).

@dylex
Copy link
Contributor Author

dylex commented Feb 10, 2014

That seems like a good idea, but unfortunately still isn't quite right because getCanonicalPath attempts to resolve symlinks. It also seems like it would still let you read files under "public2". Would it be safer just to refuse any path containing "/../"?

@jroper
Copy link
Member

jroper commented Feb 10, 2014

The difficulty is that if you start targeting specific cases, then all sorts of edge cases start to slip through, like using Windows file separators, or combinations of UNIX and windows file separators, or URL encoding things to hide the separators, malformed paths, etc etc. I think it would be safer for us to prefix the whole thing with a path we know doesn't exist, and keep the current logic.

@dylex
Copy link
Contributor Author

dylex commented Feb 10, 2014

Just a random suggestion, but the path itself is passed through the resource loader immediately after this check. In the case of jar resources this isn't a problem, and URLs are supposed to be canonical anyway. Perhaps these checks should be done in the resource loader, where it actually knows about the underlying file system, or on the resulting file: URLs?

@jroper
Copy link
Member

jroper commented Feb 11, 2014

The difficulty is finding the methods that will do this. URLs are supposed to be canonical, but they aren't in practice, you can put a dot dot in a URL and many things that read it will accept it with no problems.

What I'd like to try and avoid is put this logic into Play, I'd rather reuse existing functionality that we know works.

@marcospereira
Copy link
Member

I would say this is fixed in 2.6.latest and 2.7. as well. So closing.

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

No branches or pull requests

5 participants