New project surfaced in our company, unoriginally called IAP GCS proxy. As the name suggests, it is a small golang proxy that provides IAP restricted access to Google Cloud Storage buckets.
Within minutes one very interesting comment by Martin Bajanik, a senior application security engineer, appeared in the slack thread that caught my attention:
This might allow path traversal from the outside to traverse to a different bucket, which might be a problem if we ever reuse the same SA on two different buckets
talking about this piece of code:
upstreamUrl, err := url.Parse(fmt.Sprintf("https://storage.googleapis.com/%s%s", gp.GCSBucket, req.Path))
Don’t trust the user
So, what’s the issue with this line? Assume that someone send the following request to the IAP GCS proxy:
with the code above this results into the upstream url being
effectively allowing path traversal to other buckets in the google cloud storage.
Initial attempt to solve this was using
request.EscapedPath() instead of
- upstreamUrl, err := url.Parse(fmt.Sprintf("https://storage.googleapis.com/%s%s", gp.GCSBucket, req.Path)) + upstreamUrl, err := url.Parse(fmt.Sprintf("https://storage.googleapis.com/%s%s", gp.GCSBucket, req.EscapedPath()))
This would work, but would result into weird requests being send upstream, such as
left me wondering, whether there’s a more elegant solution to the problem at hand.
After a quick search I landed on this issue at github.com/golang/go:
proposal: path/filepath: addition of SecureJoin helper
that suggests new
SecureJoin that would prevent path
filepath.Join. Sadly, but rightful, the issue was closed as there
wasn’t compelling reason to include this into the standard library.
Some people suggested using
but as one of the great golang proverbs goes, A little copying is better than a little dependency.1.
For sure, there must be a way to solve this using the tools at hand.
Further googling landed me at ServeMux and a path traversal vulnerability article by Ilya Glotov. There, the proposed solution is lean and clean:
cleanPath := filepath.FromSlash(path.Clean("/"+strings.Trim(req.URL.Path, "/")))
path.Clean cleans the path getting rid of any parent directory references
../), that is, if the path is absolute which is a very important gotcha.
Of course, it makes sense, there’s no way to clean path such as
since the parent directory could be anything and we don’t know the file system hierarchy.
On the other hand, path such as
/a/../../b can be safely cleaned. First
sequence lends us in the root directory, the second one is basically ineffective
as the root directory doesn’t have parent directory and the final
a directory within the root.
In the proposed solution this is solved by forcing the request path to always
be absolute by prefixing it with
path.Clean than cleans
the path from any parent directory references which in turn prevents the
path traversal on the upstream.
One could end here and consider the issue done and solved. What I didn’t
like about the solution though is the call to
strings.Trim and the
string concatenation. Surely we can do without it.
The final solution
As it turns out, the whole problem of request path based path traversal on the upstream can be solved using single standard library function call.
cleanPath := path.Join("/", req.URL.Path)
path.Join not only joins the path parts in the variadic arguments list
but also calls
path.Clean on the result. Thus the
in one swift call both prefixes the request path with
/, ensuring that the
path is absolute and will be properly cleaned, and also calls
path.Clean on the result.
Since this behaviour of
path.Join might not be fully intuitive, it is a good idea
to add a comment before the call stating that we rely upon the internal
call. And if that isn’t sufficient ensurement that the call won’t be removed some time
in the future, one can go as far as adding an explicit
path.Clean call on the result:
That’s it for today. I hope you find this useful in case you are solving an issue of similar nature.