tinaselenge commented on PR #14995: URL: https://github.com/apache/kafka/pull/14995#issuecomment-1876028036
> I came up with another path traversal, but this one is less severe. > > If you try to get `/arbitrary/path/../../<tmpdir>/dir/dirFile` you can perform "exists" and "isDirectory" checks for `/arbitrary/path` depending on the error returned. > > (edit): This attack isn't reproducible in the tests, because they mock out `reader(Path)` 🙃 > > This would allow an attacker to map out the whole filesystem and permissions of the current process. This could inform another attack, such as: finding vulnerable software, finding user accounts, finding active processes, etc. > > This attack works because the normalized form is used in pathIsAllowed(), but the non-normalized form is used in the actual read. I think we need to make it so that the normalized path is used instead of the given path. > > I noticed this caveat in the documentation for Path.normalize: > > > This method does not access the file system; the path may not locate a file that exists. Eliminating ".." and a preceding name from a path may result in the path that locates a different file than the original path. This can arise when the preceding name is a symbolic link. > > I think this would cause a backwards-incompatible change if we were to apply it in the default case, so we should only use the normalized form when `allowed.paths` is specified. > > I tried and failed to come up with a arbitrary file-read attack using the symlink behavior. If the symlink is within the allowed-paths (and possibly lets the user escape the allowed-paths) it should probably work normally. If the symlink is outside of the allowed paths, we can exploit it with the same prefixing attack from above, but the allowed part of the path prevents actually reading arbitrary files. Regardless, I think that using the normalized form for file accesses prevents accessing external symlinks completely. > > Thanks so much for working on this feature! Thank you very much @gharris1727 for the detailed comment on this issue. These are really good points and I agree, I hadn't put enough thoughts into this. I took the suggestions and updated it to use the normalisedPath in the actual read when `allowed.paths` is specified. Please let me know what you think. I also tried addressing your other comments. Thank you for reviewing the PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
