Fix isAllowedDir matching by prefix #1276
Conversation
github-actions
bot
added
bug
| if err != nil { | ||
| if !os.IsNotExist(err) { |
There was a problem hiding this comment.
As said above, I think this whole Stat and err handling isn't useful, but if I'm missing the reason then can we please handle the errors as one block rather than as three separate ifs, one of which is needlessly nested?
switch {
case os.IsNotExists(err): // nothing to do so why did we do this?
case err != nil:
return false, fmt.Errorf("PathError %s %w", path, err) // disk i/o error surfaced in a weird location
case !info.IsDir():
// we don't trust our code to handle files that _do_ exist, so get the parent directory
path = filepath.Dir(path)
}| if strings.HasPrefix(directoryPath, allowedDirectory) { | ||
| for _, dir := range allowedDirs { | ||
| // Check if the path is a direct match, or is a subdirectory of the allowed directory | ||
| if slices.Contains(allowedDirs, path) || strings.HasPrefix(path, dir+"/") { |
There was a problem hiding this comment.
I don't think this is a good check
- for dir in alloweddirs, do a slices.Contains on the alloweddirs list; this is needlessly O(n^2).
- if the AllowedDirs list is
["/"]this is going to check if the path has the prefix//
Look at this implementation
We walk the given path up the tree ensuring that it is an exact match against the allowed prefix. we use strings.HasPrefix as a shortcircuit out -- e.g. /var/www2/foo/bar/bam/jack/ checked against /var/www exits out once it is comparing /var/www2 against /var/www. Thinking about it right now, that hasprefix shortcircuit might take more computation then just continuing to walk up the directory tree-- would need a benchmark to really evaluate.
See https://go.dev/play/p/tYMSZ6FbL31 as an example of walking up the tree
| // Does the path exist? | ||
| info, err := os.Stat(path) | ||
| if err == nil && !info.IsDir() { | ||
| // Path exists and is not a directory | ||
| // It's a file so get the parent directory | ||
| path = filepath.Dir(path) | ||
| } |
There was a problem hiding this comment.
Why do we think this stat and parent is useful?
If this function is called with isAllowedDir("/etc/nginx/non-existant-file.conf", ["/etc/nginx"]) then it will skip this branch and needs to work.
If this function is called with isAllowedDir("/etc/nginx/exists.conf", ["/etc/nginx"]) then it will be converted to "/etc/nginx" and it needs to work.
What are we gaining on this Stat and IsDir check? The only case I can think of is to try and catch a case where the file does exist and this process doesn't have read permission to it. But that's not the purpose of this function, and surfacing the error here feels really odd...
| "/etc/nginx", | ||
| }, | ||
| filePath: "/etc/nginx-test/nginx.conf", | ||
| }, |
There was a problem hiding this comment.
| }, | |
| }, | |
| { | |
| name: "Test 8: allow root should allow all", | |
| allowed: true, | |
| allowedDirs: []string{"/"}, | |
| filePath: "/etc/nginx-test/nginx.conf", | |
| }, |
| AllowedDirectories: []string{ | ||
| "/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/", | ||
| "/usr/share/nginx/modules/", "/etc/app_protect/", | ||
| "/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx", "/var/run/nginx", "/var/log/nginx", |
There was a problem hiding this comment.
Why are we removing the trailing slash on some of the paths in this test?
There was a problem hiding this comment.
When I played with this briefly it looked like these paths were normlaized and had the trailing slash stripped as config was being parsed and processed.
This change is confusing though, why remove it from some and not all?
Proposed changes
Stops isAllowedDir function from matching by prefix. Originally this function would allow any path which was prefixed by an allowed directory, i.e
/var/log/nginxwas present in allowed dirs, then the path/var/log/nginx-testwould also be allowed. Not good!This PR stops this match from taking place, but still matches if the provided path is a subdirectory of an already allowed directory, i.e
/var/log/nginxis in allowed dirs,/var/log/nginx/mysubdirwill be allowed but/var/log/nginx-testwill be blocked.Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentmake install-toolsand have attached any dependency changes to this pull requestREADME.md)