Possible path traversal attack

[dxa4481]

I might be making a mountain out of a molehill here but taking user route parameters and passing them into the "open" function seems pretty dangerous. The open function does support path traversal above the application directory. It looks like the yaml parser would error out if a user could perform a path traversal but this seems like it should be looked at just in case.

[ryansb]

The app (when public) runs in OpenShift which has both cgroup and selinux layered protection.

I'm not saying this doesn't matter, but for this case it's not very important. The code and data are all on github, and there isn't other sensitive data on OpenShift. Pull requests are (as always) welcome.

[msoucy]

this seems like it should be refactored just in case

Not sure you're saying what you mean to say. Refactoring, by definition, means that you change how the code looks without changing end behavior. Also, not only what @ryansb mentioned above, but the URI parsing also prevents users from putting in malicious URLs - to show that this is an actual problem, please verify that there exists a URL such that one can access arbitrary files using this.

[dxa4481]

@msoucy No.

I took my best shot at finding one, but just because I didn't find one that doesn't mean it's not dangerous to take arbitrary user provided string and feed it into open. I've mitigated any potential issue with my PR. If you don't think a string exists that can cause an issue that's fine. I exercised an instance of responsible disclosure to a potential issue, and then I submitted a patch.

And as for your constructive criticism on my use of the word "refactored", I appreciate the feedback, and have edited the original post.

[msoucy]

I'll put this another way - if there is any chance for "arbitrary input" to get put into open, it's an issue upstream in Flask or Werkzeug. I guarantee you that the app properly handles any sort of attempt at escaping its prison that I've tested, and that the "potential issue" isn't one.

[dxa4481]

Please explain what you mean. Did you look at the line number? A URI crafted at

/checkblogs/as%2E%2Edf/fall/bob

gets fed into open as follows:

scripts/people/as..df/fall/bob.yaml

Seeing as how open('../../../../../etc/passwd') is valid python albeit impossible with cgroup and selinux correctly configured, it's enough for me to mitigate any possibility however unlikely of a path traversal attack.

[msoucy]

Of course I looked at the line, I make sure to do research before commenting. Your statement is correct in that that first call to open will be run, but upon failing to read that file (barring, of course, a instructor who decides "as..df" is a nice year number) flask will handle an appropriate error. Your second statement isn't a flaw of open, but an ordinary use case for the function on its own. This type of case will never be reached because of the above URL handling.

[dxa4481]

I never claimed that path traversal is a flaw in open. You keep saying Flask is going to catch the error. What error are you suggesting it's going to catch if open is operating the way it's intended to operate? There is no white listing being done on years, or terms, which allows me to craft a year with such as "as..df" and allows it to be fed into open. I was able to get ".." into the open function with a little bit of URLencoding. What if with the proper URLencoding I was able to make the year "../../../../etc/passwd #" or "../../../../etc/passwd EOF" . Do you see how that would make its way into open, and ultimately fed into the yaml parser? What if the yaml parser didn't error out, and instead in some capacity returned a piece of the file to the user? It's a contrived example, but do you see where I'm going with it? I don't know if it's possible, or even likely, but any chance of it happening is mitigated with my PR.

[msoucy]

The error I was talking about was nonexistent files, or anything that yaml can't parse properly. The ".." that you inserted isn't actually "..", which has a meaning distinct from "a..b". Since you're unsure if the flaw you claim to fix exists, I'm asking again that you show actual proof that can inject a string such that you get to a file you shouldn't and render "dangerous" days.. "What if" isn't sufficient for a security claim like the one you make. I've understood the issue you've had the entire time, however I've tried to make it clear that the way it exists at the moment is safer than you seem to believe. The escapes you show don't actually matter to the special name "..", which flask collapses in the URL.

[dxa4481]

If you want to make the claim that no such string exists, you are entirely welcome to make that claim. I've made the claim that "such a string COULD exist" which is more than enough reason in the space of software security to submit a patch.

I'm not going to keep arguing this either. @ryansb can make a decision in the morning to either accept the pull request or not.

[ryansb]

I've made comments on your pull request.

And while "such a string could exist" is a good enough reason for some applications, it's not very relevant for this particular case so I'd rather you prove a vulnerability exists than just submit patches for potential vulns that would be mitigated at a different layer of the stack in any case.

@dxa4481 your pull request contains style errors, so it can't be merged in any case.

[dxa4481]

Closing because I couldn't find an active exploit.

You can drop raw hex chars into the open function so \x2f..\x2f..\x2f..\x2f..\x2fetc\x2fpasswd would work, but a URL encoded backslash %5C is being escaped by flask.

Null characters are allowed %00 can make it into the open function, but it looks like the open function rejects it.

I'm not 100% satisfied but I gave it my best shot and couldn't find a way to exploit it.

[ryansb]

Thanks for your effort, reopen if you do find a vulnerability.