Repository navigation
ScpClient hardening - #1847
Open
Rob-Hague wants to merge 6 commits into
Open
ScpClient hardening#1847Rob-Hague wants to merge 6 commits into
Rob-Hague wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Existing links can still redirect downloads outside the destination, and the transcript timeout path leaks its worker task.
2 open findings
What changed in this PR
Hardens SCP parsing, path containment, upload-name validation, and premature channel closure handling.
Changes:
- Adds strict SCP record parsing and bounded control lines.
- Strengthens download path validation and upload filename checks.
- Adds extensive unit/integration coverage and fixes legacy MD5 calculation.
| File | Description |
|---|---|
src/Renci.SshNet/ScpClient.cs |
Implements SCP hardening and parser changes. |
src/Renci.SshNet/Common/PosixPath.cs |
Adds nullable-safe immutable path parsing. |
src/Renci.SshNet/Abstractions/StringExtensions.cs |
Adds compatibility Contains(char). |
test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs |
Defines transcript execution results. |
test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs |
Adds mocked SCP transcript harness. |
test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs |
Adds isolated download-test infrastructure. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_Upload_InvalidName.cs |
Tests unsafe upload names. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_ResolveChild_RootDirectory.cs |
Tests root-directory resolution. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs |
Removes superseded validation tests. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_Names.cs |
Tests local-name containment behavior. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs |
Tests malformed and unexpected records. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ControlLineTooLong.cs |
Tests control-line limits. |
test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs |
Tests truncated transfers. |
test/Renci.SshNet.IntegrationTests/ScpTests.cs |
Adds end-to-end hardening scenarios. |
test/Renci.SshNet.IntegrationTests/OldIntegrationTests/ScpClientTest.cs |
Corrects MD5 formatting. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
When the channel was closed before all of the file content announced in a C record had been received, PipeStream.Read kept returning 0 and the copy loop spun forever. Throw an SshException in this case, matching ReadByte.
ReadString accumulated bytes until a line feed with no upper bound, so a server could exhaust client memory by never sending one. Control lines and error messages are now limited to 64 KiB; a longer line throws ScpException.
The file name part of the path passed to Upload(Stream, string) and Upload(FileInfo, string) is written verbatim into the C record, as are local file and directory names in Upload(DirectoryInfo, string). A line feed in such a name ends the record early, so the remainder of the name and the file content can be interpreted as further SCP commands in the byte stream, for example creating additional files in the target directory. Throw ArgumentException in Upload(Stream, string) and Upload(FileInfo, string) for this case, and ScpException in Upload(DirectoryInfo, string) when it comes across such a local file or directory. Also reject paths without a file name part.
The C, D and T record patterns were not anchored and D was tried before C, so a file named, for example, "zD0755 0 x" was treated as a directory and its content read as SCP records, letting whoever controls a file on the server choose the names and contents of files written locally. The patterns now match the whole line. An invalid or unexpected record is reported to the server and thrown as ScpException; previously an unknown record was reported and ignored, and Download(string, Stream) then returned normally with an empty stream. Download(string, FileInfo) no longer shares the directory loop. It accepts an optional T record followed by a C record, so a D or E record can no longer cause an InvalidCastException or let later C records overwrite the file. The directory loop moves to DownloadDirectoryContent and the content copy is renamed DownloadFileContent.
An E record received before any D record made the directory depth counter negative and moved the current directory to the parent of the destination, so that a following C record wrote outside it; each further E record climbed another level. On Windows, a D record named "...", ".. " or " .." (legal names on a Unix server) normalizes to the current directory, so a D and E pair climbed one level with a balanced counter. An unmatched E record is now rejected before it is acknowledged. Every C and D record name goes through ResolveChild, which requires the full path to be a direct child of the current directory with exactly the name that was sent. This also rejects Windows device names such as "CON", and names that Windows would silently change, such as a trailing dot, unless the destination uses the \\?\ prefix. Rejected names are reported to the server and thrown as ScpException. ResolveChild replaces EnsureValidLocalName.
Rob-Hague
force-pushed
the
scp-hardening
branch
from
October 11, 2026 20:02
9951f67 to
805a884
Compare
This branch has not been deployed
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.
by commit (intended for rebase-merge)
written (and found) largely by AI under pretty heavy oversight and manual edits