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

v1: Prevent directory path traversal in FileHandler #2388

Merged
merged 3 commits into from Dec 4, 2019

Conversation

christi3k
Copy link

Presently FileHandler allows ../ which enables directory path traversal when a Files endpoint is defined with a wildcard.

Example API design definition:

var _ = Resource("docs", func() {
	Description("Documentation endpoint")

	Origin("*", func() {
		Methods("GET", "OPTIONS")
	})

	Files("/docs/*filepath", "swagger-ui")
})

Example exploit:

curl "http://localhost/docs/..%2F..%2F..%2Fetc%2F"

This patch updates FileHandler to return ErrNotFound if a ../ is found in req.URL.Path.

I wanted to submit tests along with this but got stuck trying to make them work. If someone could provide guidance, I would be happy to make another attempt.

Existing tests do pass:

$ make test
Ginkgo ran 25 suites in 1m1.149089831s
Test Suite Passed
go test ./_integration_tests
ok  	github.com/goadesign/goa/_integration_tests	20.423s

Additionally, I've applied this patch to a local dev instance of our app and verified the changes operates as expected. A curl request such as the one does return a 404.

@raphael
Copy link
Member

raphael commented Nov 28, 2019

Thank you for the PR, this is great! I wonder if it wouldn't be more robust to use os.PathSeparator though instead of hard-coding the file path separator - this would alleviate the need to check for both types of separators which could actually result in faulty behavior (e.g. if a unix file has a backslash in its name). Another (better?) approach would be to use IsPathSeparator instead of a custom function to splice.

@christi3k
Copy link
Author

Thanks for the suggestion to use IsPathSeparator. I've updated the isSlashRune() function to use it.

While I was at it, I also adjusted the check to allow:

  • .. in filenames (legal in *nix and Windows, I believe)
  • relative paths using .. within the defined path mount point (but not outside/above)

make tests all passed and I tested a few different paths with a running version against our app.

@raphael
Copy link
Member

raphael commented Dec 4, 2019

Looks great, thank you.

@raphael raphael merged commit 70b5a19 into goadesign:v1 Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants