New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Dashboard] Prevent Directory Traversal #39018
[Dashboard] Prevent Directory Traversal #39018
Conversation
dashboard/http_server_head.py
Outdated
|
||
# If the destination is not relative to the expected directoy, then the user is attempting | ||
# path traversal, so deny the request | ||
if not pathlib.Path(os.path.abspath(request.path)).is_relative_to(parent): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not pathlib.Path(os.path.abspath(request.path)).is_relative_to(parent): | |
if not pathlib.Path(request.path).absolute().is_relative_to(parent): |
Mixing path libraries for a particular reason? (If its load bearing can you explain it in a comment?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to resolve()
@@ -128,6 +129,17 @@ def get_address(self): | |||
assert self.http_host and self.http_port | |||
return self.http_host, self.http_port | |||
|
|||
@aiohttp.web.middleware | |||
async def path_clean_middleware(self, request, handler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to normalized all path values before passing it forward in the routing handler? (can we even do that in a middleware?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that since these APIs lean on the request path itself and not a parameter or data field; if we could normalize the request.Path
field itself before it was handled by the router/mux, then this would 404 instead of getting into the final handler.
That said, that almost certainly isn't something we can do from within Starlette middleware? Noop this idea xD
Signed-off-by: Ian Rodney <ian.rodney@gmail.com>
Signed-off-by: Ian Rodney <ian.rodney@gmail.com>
Signed-off-by: Ian Rodney <ian.rodney@gmail.com>
Signed-off-by: Ian Rodney <ian.rodney@gmail.com>
2f2f980
to
2e72429
Compare
Signed-off-by: Ian <ian.rodney@gmail.com>
Signed-off-by: Ian Rodney <ian.rodney@gmail.com>
Signed-off-by: Ian Rodney <ian.rodney@gmail.com>
Failing test is unrelated--merging |
Ensure we can only go to subdirectories of logs and static resources. Signed-off-by: Simon Zehnder <simon.zehnder@gmail.com>
Ensure we can only go to subdirectories of logs and static resources. Signed-off-by: Victor <vctr.y.m@example.com>
Ensure we can only go to subdirectories of logs and static resources.
* [Dashboard] Prevent Directory Traversal (#39018) Ensure we can only go to subdirectories of logs and static resources. * [core][state][log] State API should not allow reading files outside of the ray log directory on all ray nodes. (#41467) State API log retrieval has a security bug where one could pass: relative paths like "../../../xxx" to get file outside of ray's log dir absolute path that's refers to other files to get file somewhere else. This PR fixes both issues such that one could only read logs under the ray logs directory. --------- Signed-off-by: rickyyx <rickyx@anyscale.com> * [Dashboard] Migrate Logs page to use state api. (#41474) Migrates to use state-api, to unify behavior with CLI and UI Delete log_proxy API, it's legacy and has some security issues. --------- Signed-off-by: Alan Guo <aguo@anyscale.com> * [core][state][log] Enable following symlinks that point outside of the `root_log_dir` when resolving paths (#41502) Follow-up to: #41467. The change incidentally broke log retrieval on mac os because /tmp is a symlink to /private/tmp. This PR avoids resolving the symlink until after we do the subdir check. This solves the mac os problem and generically enables file paths that contain symlinks outside of the root_log_dir. --------- Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> --------- Signed-off-by: rickyyx <rickyx@anyscale.com> Signed-off-by: Alan Guo <aguo@anyscale.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com> Co-authored-by: Ian Rodney <ian.rodney@gmail.com> Co-authored-by: Ricky Xu <xuchen727@hotmail.com> Co-authored-by: Alan Guo <aguo@anyscale.com> Co-authored-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Why are these changes needed?
logs
andstatic
resources.Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.