codeql
codeql copied to clipboard
github
Go: Add and Modify Sanitizers For TaintedPath
This PR adds some additional sanitizers to the TaintedPath Customization due to a high number of false positives I have found after doing some research. I have added: filepath.Base strings.ReplaceAll http.ParseMultipartForm I have also removed path.Clean as it does not sufficiently clean paths for software that can run on Windows. Please see: https://github.com/labstack/echo/pull/1718 as an example.
What's your reasoning on
filepath.Base? In particularfilepath.Base("..")is"..", which seems like we shouldn't be letting that through?
Thats fair, I found that most developers have used filepath.Base to correctly prevent path traversal that is why I included it. Since ParseMultipartForm uses filepath.Base on Filename, do you want to include it? It "cleans" the filename of the multipart form and since ".." is not a valid filename across platforms, it should normally error out on any file operations.
@Kwstubbs I would have to appeal to the security lab for a definitive opinion, but my gut take is that filepath.Base is an inadequate sanitizer since I can still traverse one layer outside the directory I was expected to. I might get lucky with some file ops because $cwd/.. is a directory, but in general that still strikes me as dangerous.
@smowton Is the policy for the sanitizers that no true positive can be discarded in exchange for a better FP rate? If so I will remove it because I agree that while is possible that ".." can be possibly smuggled in if let's say the developer wants to create/remove a directory with a similar name to the file, I just find it highly unlikely. In general, I suggested filepath.Base and ParseMultipartForm because I found developers correctly used them as files, not as directories and because golang has seperate file/directory APIs I thought the lower FP rate would be worth missing a few TPs.
I made a PR with a few follow-ups here: https://github.com/github/codeql/pull/16180