This part is kind of interesting — it looks like gosec at least gave the opportunity to catch this but that was missed due a misunderstanding of what was in scope for filepath.Clean:
...
3. Eliminate each inner .. path name element (the parent directory)
along with the non-.. element that precedes it.
4. Eliminate .. elements that begin a rooted path:
that is, replace "/.." by "/" at the beginning of a path,
assuming Separator is '/'.
There is no mention that this function cannot be used for security, or that it does not safely ensure a path can't refer to a parent directory.
A quick reading of 3 and 4 will make you assume that a path has no ".."s after being "Clean"d. If you actually think about it more, you'll realize that of course it will leave ".." at the beginning of relative paths, but the docs do not make it clear, and I can understand why a programmer might reach for this.
There's no clearer function to reach for either, like "filepath.DirectoryContains(parent, path) bool".
There is no mention that this function cannot be used for security, or that it does not safely ensure a path can't refer to a parent directory.
It seems to me that both of those guarantees are something that would need to be explicitly stated if provided, and one wouldn't just assume that they hold by omission. In general if the docs were to list all postconditions that are not guaranteed to hold, it would take a while to write them :).
Just to be clear, I’m not blaming them. I should have clarified that I was thinking along the lines of “how could the tool/documentation have made it easier to recognize that gap?” — and I especially agree with your suggestion that there could be a solid library improvement with a purpose-built function which is clearly identified as the right way to address this relatively common need.
Actually no. Testing even a fairly comprehensive set of test cases could have given a false sense of security and possibly missed some actual vulnerable cases.
Best thing would have been to either implement a function that does give the required guarantees or look for an existing library that explicitly does.
Comments
Good ol path traversal https://github.com/grafana/grafana/commit/c798c0e958d15d9cc7...
This part is kind of interesting — it looks like gosec at least gave the opportunity to catch this but that was missed due a misunderstanding of what was in scope for filepath.Clean:
https://github.com/grafana/grafana/commit/c798c0e958d15d9cc7...
I mean, can you blame them?
The docs for filepath.Clean state the following:
There is no mention that this function cannot be used for security, or that it does not safely ensure a path can't refer to a parent directory.A quick reading of 3 and 4 will make you assume that a path has no ".."s after being "Clean"d. If you actually think about it more, you'll realize that of course it will leave ".." at the beginning of relative paths, but the docs do not make it clear, and I can understand why a programmer might reach for this.
There's no clearer function to reach for either, like "filepath.DirectoryContains(parent, path) bool".
It seems to me that both of those guarantees are something that would need to be explicitly stated if provided, and one wouldn't just assume that they hold by omission. In general if the docs were to list all postconditions that are not guaranteed to hold, it would take a while to write them :).
Just to be clear, I’m not blaming them. I should have clarified that I was thinking along the lines of “how could the tool/documentation have made it easier to recognize that gap?” — and I especially agree with your suggestion that there could be a solid library improvement with a purpose-built function which is clearly identified as the right way to address this relatively common need.
A good example case for why testing your assumptions is usually a good thing.
Actually no. Testing even a fairly comprehensive set of test cases could have given a false sense of security and possibly missed some actual vulnerable cases.
Best thing would have been to either implement a function that does give the required guarantees or look for an existing library that explicitly does.