LGTM.com - false positive - Zip Slip when guard FileNameUtils#normalize is used #9205

Activity

1. JLLeitschuh

   added

   false-positive

   on May 18, 2022
2. hvitved

   added

   Java

   on May 18, 2022
3. hvitved commented on May 19, 2022

   

   hvitved

   on May 19, 2022

   Contributor

   More actions

   @github/codeql-java one for you.
4. Marcono1234 commented on May 21, 2022

   

   Marcono1234

   on May 21, 2022

   Contributor

   More actions

   Depending on how sophisticated the guard detection should be, FileNameUtils#normalize might not on its own be enough to sanitize the path. In the code example you have shown, the code uses java.io.File to construct the path and therefore seems to be safe.

   However, if code uses java.nio.file.Path, then FileNameUtils#normalize would not suffice for sanitization because Path.resolve behaves differently for absolute paths and paths with a different root component. For example on Windows:

   jshell> Path.of("C:\\safe").resolve("\\not-safe")
   ==> C:\not-safe
   
   jshell> Path.of("C:\\safe").resolve("D:not-safe")
   ==> D:not-safe
   

   ---

   Note that I am not a member of this project.

   👍React with 👍1Reacted by Jonathan Leitschuh
5. atorralba commented on May 23, 2022

   

   atorralba

   on May 23, 2022

   Last edited by atorralba

   Contributor

   More actions

   Aside from what @Marcono1234 pointed out, depending on how FilenameUtils.normalize itself is used, it won't be a proper sanitizer either. For example:

   FilenameUtils.normalize("/safe/" + userInput()); // userInput() = "../unsafe/"

   would return /unsafe/.

   So apparently FilenameUtils.normalize would only be a valid sanitizer if it's used directly on the user input AND the result is prefixed with a trusted path (i.e. not used as an absolute path), and Path.resolve is NOT used to do so.

   👍React with 👍1Reacted by Jonathan Leitschuh