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

Possibility of directory traversal vulnerability on Static File delivery #44

Closed
jeevatkm opened this issue May 21, 2017 · 10 comments
Closed
Assignees
Labels
bug

Comments

@jeevatkm
Copy link
Member

@jeevatkm jeevatkm commented May 21, 2017

On Reddit user epiris reported the possibility of directory traversal vulnerability on Static File delivery.

I have analyzed the issue and pointers from epiris. aah framework uses http.Dir internally for serving directory listing. http.Dir has checks for Dot-Dot, \ path separator and \x00 char to prevent directory traversal vulnerability.

However it is good to place the check at framework before processing an incoming directory listing request.

Thanks to epiris for taking out his time.

Note: Static file/directory delivery scenario's protected by http.Dir.

Note: As per framework design, this issue possibility is only applicable to directory listing, not for static file serve. Since static file config is defined by application user in the routes.conf. aah framework will not entertaint any request if the definition doesn't match from routes.conf.

@jeevatkm jeevatkm added the bug label May 21, 2017
@jeevatkm jeevatkm self-assigned this May 21, 2017
jeevatkm added a commit that referenced this issue May 21, 2017
@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 21, 2017

Addressed, need to make a release of v0.5.1 with issue details here

@cstockton
Copy link

@cstockton cstockton commented May 21, 2017

I have analyzed the issue and pointers from epiris. aah framework uses http.Dir internally for serving directory listing. http.Dir has checks for Dot-Dot, \ path separator and \x00 char to prevent directory traversal vulnerability.

You are correct that http.Dir is safe, but only if the usage itself is safe and the prior use was not. You must have a http.Dir that was created with trusted input, but you passed it the result of a filepath.Join that come from a user request as mentioned in my post. I.E.:

static.go starts like:

func (e *engine) serveStatic(ctx *Context) error {
	// TODO static assets Dynamic minify for JS and CSS for non-dev profile
	dir, file := filepath.Split(getFilepath(ctx))
        ...
       	fs := ahttp.Dir(dir, ctx.route.ListDir)
       // you use http.Dir but your use is incorrect, because the "dir" value comes from user input
       // created by getFilePath which is created like:
      filepath.Join(AppBaseDir(), ctx.route.Dir, ctx.Req.PathValue("filepath"))
      // Which will take the filepath from the request and join it to to AppBaseDir and ctx.route.Dir. But
      // this join is relative and traversal is possible, for example if I request ../../../../.

I replied to you with what I would do. I'm not sure about the "static" routes.conf stuff, if you are saying ctx.route.File comes from a static user defined config setting, then that is fine, but you should probably just avoid all of that getFilePath code to begin with then. Check if ctx.route.IsDir() is false first before you ever worry about httpDir etc, and serve the file immediately. Good turn around time on your change it looks pretty good aside from what I mentioned in my latest reply.

@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 21, 2017

Yes, ctx.route.File value comes from configuration defined by user. Thank you.

I will be making a release tomorrow.

@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 21, 2017

@cstockton I have read your latest reply, I will update it with clear flow.

jeevatkm added a commit that referenced this issue May 21, 2017
@jeevatkm jeevatkm changed the title Possibility of directory traversal vulnerability on directory listing Possibility of directory traversal vulnerability May 21, 2017
@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 21, 2017

@cstockton using http.Dir directly and I have updated with clear flow.

jeevatkm added a commit to go-aah/website that referenced this issue May 21, 2017
@jeevatkm jeevatkm mentioned this issue May 21, 2017
@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 21, 2017

@cstockton v0.5.1 released.

@jeevatkm jeevatkm closed this May 21, 2017
@jeevatkm jeevatkm changed the title Possibility of directory traversal vulnerability Possibility of directory traversal vulnerability on Static File delivery May 21, 2017
@cstockton
Copy link

@cstockton cstockton commented May 21, 2017

Wow, fantastic job @jeevatkm static.go is concise and secure. I like that you even jail the ctx.route.File despite your earlier reservations due to it being from a config file. Good stuff.

@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 21, 2017

@cstockton Actually with the latest change I have revisited, both scenario is protected by http.Dir even though value come from config.

Would you like to give github star for aah framework?

@cstockton
Copy link

@cstockton cstockton commented May 22, 2017

Actually with the latest change I have revisited, both scenario is protected by http.Dir even though value come from config.

@jeevatkm Yep! I noticed and was complimenting the initiative, good job. Have a :)

@jeevatkm
Copy link
Member Author

@jeevatkm jeevatkm commented May 22, 2017

@cstockton Thank you :)

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.