Catch an edge case in expand._assert_local() #3595
Conversation
Using str.startswith() has an edge case where someone can access files
outside the root directory. For example, consider the case where the
root directory is "/home/user/my-package" but some secrets are stored in
"/home/user/my-package-secrets". Evaluating a check that
"/home/user/my-package-secrets".startswith("/home/user/my-package") will
return True, but the statement's intention is that no file outside of
"/home/user/my-package" can be accessed.
Using pathlib.Path.resolve() and pathlib.Path.parents eliminates this
edge case.
|
Thank you very much for working on this, @mssalvatore. I can see that you have used |
|
@abravalheri Using If it's acceptable or desirable to allow symlinks to be traversed, I can reimplement this using |
|
Hi @mssalvatore thank you very much for having a look on this. I think it is fair to assume that if the user has a symbolic link, they want it to be considered as a local file. I am also concerned about the backward compatibility... My opinion is that it would be better to go with a loose interpretation and use |
4249da1 uses `pathlib.Path.resolve()` instead of `os.path.abspath()` to canonicalize path names. `resolve()` resolves symlinks, whereas `abspath()` does not. `resolve()` can also raise a `RuntimeError` if infinite loops are discovered while resolving the path. There is some concern that using `resolve()` would not be backwards compatible. This commit switches back to `abspath()` but still uses `Path.parents` to avoid the edge case. See PR #3595 for more details.
|
@abravalheri I've pushed a commit to use |
Summary of changes
Using
str.startswith()for sandboxing file access has an edge case where someone can access files outside the root directory. For example, consider the case where the root directory is "/home/user/my-package" but some secrets are stored in "/home/user/my-package-secrets". Evaluating a check that "/home/user/my-package-secrets".startswith("/home/user/my-package") will return True, but the statement's intention is that no file outside of "/home/user/my-package" can be accessed.Using
pathlib.Path.resolve()andpathlib.Path.parentseliminates this edge case.See https://salvatoresecurity.com/preventing-directory-traversal-vulnerabilities-in-python/ for more details.
Caveats
pathlib.Path.resolve()will raise aRuntimeError.pathlib.Path.resolve()resolves symlinks, whereas the old code usingos.pathlib.abspath()did not.I am not intimately familiar with setuptools and could use some guidance as to whether or not either of these two caveats would cause an issue. My guess would be that the first would not impact users and the second might, but probably won't. If either of the above caveats is an issue, I can provide an alternate implementation using
os.path.abspath(), though that comes with its own caveats.Pull Request Checklist
changelog.d/. - This PR should not change the way users interact with setuptools, with the possible exception of the caveats above.