Fix security issues found during code review#75
Open
Gahrcoder wants to merge 6 commits intoNVIDIA:mainfrom
Open
Fix security issues found during code review#75Gahrcoder wants to merge 6 commits intoNVIDIA:mainfrom
Gahrcoder wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
This file contains hidden or 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
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.
Summary
Found several security and correctness issues while reading through the server and model loading code. Fixes below, one per commit.
Changes
1. Path traversal in voice prompt loading (
server.py)The
voice_promptquery parameter from WebSocket clients was passed directly toos.path.join()without sanitization. A request likevoice_prompt=../../etc/passwdcould escape the voice prompt directory. Now rejects any filename containing path separators.2. Unsafe
torch.load()calls (lm.py,loaders.py)Four
torch.load()calls were missingweights_only=True, which means they use pickle deserialization under the hood. Since voice prompt.ptfiles can come from user-controllable paths, this is a code execution risk. Addedweights_only=Trueto all four call sites.3. Tarfile extraction without member validation (
server.py,offline.py)tar.extractall()was called without checking member paths. Malicious tar archives can include entries like../../.bashrcthat write outside the target directory (zip-slip). Added a_safe_tar_extract()helper that validates all member paths resolve within the destination before extracting.4. Static file server follows symlinks (
server.py)follow_symlinks=Trueon the static file route means symlinks in the static directory can serve arbitrary files. Changed tofollow_symlinks=False.5. Wrong
requestaccess for seed parameter (server.py)int(request["seed"])raises aTypeError— should beint(request.query["seed"])to match thein request.queryguard above it.6. Duplicate
import random(server.py)randomwas imported twice (lines 31 and 46). Removed the duplicate.Testing
All changes are backwards-compatible. The
weights_only=Trueflag works with standard state dict checkpoints. The tar extraction helper is a strict superset ofextractall()behavior for well-formed archives.