Vulnerabilities Corrected: Tar Slip and Path Traversal #90
+47
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david6915
approved these changes
Sep 22, 2024
adefossez
requested changes
Sep 27, 2024
| async def handle_root(_): | ||
| return web.FileResponse(os.path.join(static_path, "index.html")) | ||
| # Resolve the static path to an absolute path | ||
| static_path = Path(static_path).resolve() |
There was a problem hiding this comment.
I think the typer will complain with this. I see you didn't run pre-commit hooks
| log("error", f"The provided static path is not a valid directory: {static_path}") | ||
| sys.exit(1) | ||
|
|
||
| # Optional: Define a base directory to restrict static content |
There was a problem hiding this comment.
We do not want to have features that are based on uncommented lines of code. Could you remove this block?
| # log("error", "Attempt to access a directory outside the allowed base directory.") | ||
| # sys.exit(1) | ||
|
|
||
| async def handle_root(_): |
There was a problem hiding this comment.
As we do not pretend to be resilient to a wrongly formed static, I think I would rather have a mucher simpler implementation, closer to the original, simply adding before the serving:
assert (Path(static_path) / 'index.html').is_file(), "index.html missing in static folder."
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
cargo check,cargo clippy,cargo test.PR Description
I, ThewsyRum, confirm that I have read and understood the terms of the CLA of Kyutai-labs, as outlined in the repository's CONTRIBUTING.md, and I agree to be bound by these terms.
Previously, calling extractall to unpack files from a tar archive didn't properly check the file paths, which could allow files to be extracted outside the intended directory.
tar.extractall(path=dist_tgz.parent)Input from a command line argument is passed directly to aiohttp.web.FileResponse as a path. This could create a Path Traversal vulnerability, enabling an attacker to read arbitrary files on the server.
return web.FileResponse(os.path.join(static_path, "index.html"))