fix(security): driver uncontrolled data used in path expression injection#740
fix(security): driver uncontrolled data used in path expression injection#740odaysec wants to merge 2 commits intoOpenListTeam:mainfrom odaysec:patch-1
Conversation
Signed-off-by: Zeroday BYTE <github@zerodaysec.org>
There was a problem hiding this comment.
Pull Request Overview
This PR adds input validation for the dirName parameter in MakeDir to prevent path traversal vulnerabilities by rejecting names containing /, \, or ...
- Enforce validation on
dirNamebefore constructing the path. - Return an error if
dirNamecontains invalid traversal characters. - Proceed to create the directory when validation passes.
Comments suppressed due to low confidence (1)
drivers/local/driver.go:255
- Add unit tests for
MakeDircovering both valid names (e.g. alphanumeric only) and invalid cases (/,.., backslashes) to verify the new validation logic.
if strings.Contains(dirName, "/") || strings.Contains(dirName, "\\") || strings.Contains(dirName, "..") {
| func (d *Local) MakeDir(ctx context.Context, parentDir model.Obj, dirName string) error { | ||
| // Validate dirName to ensure it does not contain invalid characters | ||
| if strings.Contains(dirName, "/") || strings.Contains(dirName, "\\") || strings.Contains(dirName, "..") { | ||
| return fmt.Errorf("invalid directory name: %s", dirName) |
There was a problem hiding this comment.
Avoid echoing raw user input in error messages to reduce risk of log injection; use %q to quote and escape the dirName value.
| return fmt.Errorf("invalid directory name: %s", dirName) | |
| return fmt.Errorf("invalid directory name: %q", dirName) |
| // Validate dirName to ensure it does not contain invalid characters | ||
| if strings.Contains(dirName, "/") || strings.Contains(dirName, "\\") || strings.Contains(dirName, "..") { |
There was a problem hiding this comment.
[nitpick] Consider extracting the validation into a helper (e.g. isValidDirName) or consolidating the checks into a single regex or filepath.Base comparison for clearer intent and reuse.
| // Validate dirName to ensure it does not contain invalid characters | |
| if strings.Contains(dirName, "/") || strings.Contains(dirName, "\\") || strings.Contains(dirName, "..") { | |
| // Validate dirName using helper function | |
| if !isValidDirName(dirName) { |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zeroday BYTE <github@zerodaysec.org>
|
Thanks for pointing that out! The MakeDir function is not affected by this issue, but a similar risk does exist in the Rename function, which we're addressing in #744. Appreciate your thoughtful review! |
OpenList/drivers/local/driver.go
Line 255 in 0c46199
Accessing files using paths constructed from user-controlled data can allow an attacker to access unexpected resources. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files. paths that are naively constructed from data controlled by a user may be absolute paths, or may contain unexpected special characters such as "..". Such a path could point anywhere on the file system.
fix the issue, we need to validate the
dirNameparameter to ensure it does not contain any path traversal sequences (../,.., or path separators like/or\). SincedirNameis expected to be a single directory name, we can enforce this by rejecting any input that contains these invalid characters.dirNamebefore constructing thefullPath.The changes will be made in the
MakeDirfunction indrivers/local/driver.go.