fix(security): replace path traversal blocklist with canonical containment validation (CWE-22) - #589
Open
aryan4859 wants to merge 2 commits into
Open
fix(security): replace path traversal blocklist with canonical containment validation (CWE-22)#589aryan4859 wants to merge 2 commits into
aryan4859 wants to merge 2 commits into
Conversation
…locklisting ".." The previous check only rejected paths containing the literal substring "..", which is bypassed by absolute paths, alternate path separators, and extended-length path prefixes — none of which contain "..". Canonicalize the resolved path with Path.GetFullPath() and verify it is a descendant of the intended base directory before opening the file, rather than pattern-matching the raw input string. Fixes path traversal / arbitrary file read (CWE-22, CWE-23) in ComputeFileHashAsync when filePath originates from untrusted input.
…ngComparison.Ordinal))
{
throw new ArgumentException("Invalid file path");
}
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
1 participant
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
Fixes two path traversal issues found during security review, both stemming from
the same root cause: validating file paths against a substring blocklist or no
check at all, instead of canonicalizing and checking containment within an
intended base directory.
Changes
1.
ComputeFileHashAsyncPreviously rejected paths only if they contained the literal substring
"..".This is bypassed by absolute paths, alternate path separators, and extended-length
path prefixes - none of which contain
".."- allowing arbitrary file reads iffilePathis ever influenced by untrusted input.Fix: Canonicalize with
Path.GetFullPath()and verify the resolved path is adescendant of an explicit
allowedBaseDirectoryparameter before opening the file.2.
ImportFileAsyncNo path validation at all -
filePathwas built directly fromPath.Combine(context.WorkDir, "files", hash)with no check that
hashcouldn't escape that directory via traversal sequencesor a rooted/absolute value.
Fix: Same canonicalization pattern resolve the full path and confirm it
stays within
context.WorkDir/filesbefore checking existence or opening the file.Why this approach
Blocklisting specific substrings (
"..") doesn't validate the resolved locationof a path it only blocks one known technique.
Path.GetFullPath()+StartsWith(base + separator)validates the actual outcome regardless of how thetraversal is encoded.
Testing
../sequences,and alternate separators are rejected by both methods
Related
Addresses path traversal reported in here (CWE-22 / CWE-23).