Fix for Directory Traversal Vulnerability in recursivelyDelete Method #67
Conversation
Description: This PR fixes a critical path traversal vulnerability in the recursivelyDelete method that could potentially allow deletion of files outside the intended base directory. This issue, originally reported in CVE-2022-24329, was resolved in the repository via this commit AdoptOpenJDK/IcedTea-Web@b09c6a4. References: 1. AdoptOpenJDK/IcedTea-Web@b09c6a4 2. https://nvd.nist.gov/vuln/detail/cve-2022-24329
|
Thank you for both identifying the vulnerability in Robocode, and also provide a PR for fixing it. ❤️ I will study the CVE and fix, and probably merge the PR as-is, unless it needs some extra consideration. |
flemming-n-larsen
left a comment
flemming-n-larsen
left a comment
There was a problem hiding this comment.
Your proposed fix is quite good. It addresses the directory traversal vulnerability effectively by implementing proper security checks. 👍😊
Here's an analysis of your solution:
Your implementation adds a base directory parameter and verifies that files being deleted stay within this base directory's scope, which effectively prevents directory traversal attacks. The solution includes proper canonicalization of paths and exception handling.
The key strengths of your solution are:
- Path canonicalization - Using and resolves symbolic links and normalizes paths.
getCanonicalFile()``getCanonicalPath() - Path validation - The check
file.getCanonicalFile().toPath().startsWith(base.getCanonicalFile().toPath())ensures files are only deleted if they're within the base directory. - Proper error handling - Throwing a specific security violation exception with a clear message.
- Null check for - Prevents null pointer exceptions if directory listing fails.
listFiles()
I will merge you changes as is but also add some more improvements to the deleteFile() method and add additional sanity checks.
|
@flemming-n-larsen thank you for accepting the PR! I will be submitting this as a CVE, do let me know whether it's okay with you guys. Thanks! |
|
Thank you for your help. You are welcome to summit this as a CVE. 👍 |
2 participants
Description:
This PR fixes a critical path traversal vulnerability in the recursivelyDelete method that could potentially allow deletion of files outside the intended base directory.
This issue, originally reported in CVE-2022-24329, was resolved in the repository via this commit AdoptOpenJDK/IcedTea-Web@b09c6a4.
References: