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.
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.
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.
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.
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.
Add `#if NETFRAMEWORK` block to call `_serverMac.Initialize()` after `TransformFinalBlock` in `ReceiveMessage`. Ensures MAC is reset for reuse, addressing issues on older .NET Framework builds (e.g., mscorlib.dll 4.8.4110.0).
Co-authored-by: Wojciech Nagórski <wojtpl2@gmail.com>
Remove the explicit SendBufferSize/ReceiveBufferSize assignment
from both SocketConnect and SocketConnectAsync. This lets the
operating system auto-tune the buffer for every connection by
default, and resolves the sync/async multiplier mismatch.
* Drop some dead internal code
* remove some more unused events and their callbacks
* more
---------
Co-authored-by: Robert Hague <rh@johnstreetcapital.com>
* Bump the dependencies group with 8 updates
---------
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
A malicious or compromised SCP server could return file or directory names containing
path separators, drive qualifiers, or parent-directory references.
ScpClient.Download(string, DirectoryInfo) combined these into a local path without
validation, allowing writes outside the destination directory. Server-supplied C and D
record names are now validated before being combined into a local path.
Signed-off-by: Nadav0077 <18245584+Nadav0077@users.noreply.github.com>
SCP performs a transfer by running scp on the server with the remote path
embedded in a command. On a shell-based server that command is interpreted
by a shell, so a path that is not quoted to suit that shell can be executed
as a command on the server (GHSA-mggc-4xg6-vcxf); on a non-shell-based
server the path is used literally and must not be quoted at all. The right
encoding therefore depends on the server, and no single transformation is
safe for every server.
Rather than default this choice, obsolete the ScpClient constructors that
implicitly used DoubleQuote and add constructors that take an
IRemotePathTransformation explicitly, so callers must choose one suited to
their server and trust environment. DoubleQuote remains the default for the
obsolete constructors, so existing behaviour is unchanged. Document the
consideration on ScpClient and IRemotePathTransformation, and recommend
using SFTP.
Add upper bounds on the number of banner lines and line length before the SSH identification
string, analogous to OpenSSH. Also don't buffer all the data unnecessarily.
* Downgrade testcontainers to 4.11
Running the .NET Framework integration tests locally (not CI since we don't use the library
there) is broken in 4.12. Also add .dockerignore since the image was including 200MB of
stuff under bin/.
* bump to 4.13
* Tweak internal IProgress usage for APM and sync UploadFile/DownloadFile
Changes to support IProgress<> callback on UploadAsync/DownloadAsync meant wrapping
the Action<> callback on existing methods in a Progress<>, which posts the callback
onto the current synchronisation context rather than the threadpool. For the legacy
APM methods (Begin[..]), let's just preserve their old behaviour.
For the synchronous methods, posting to the synchronisation context is probably the
worst choice (because if there is one, the method itself is running there). We can
either revert to the threadpool as well, or take the opportunity to invoke the
callback synchronously, which is a behavioural change but probably the least
surprising behaviour for a synchronous method.
* keep callbacks on the threadpool
Actually, we could call the Download callback synchronously easily enough, but the Upload progress
reports are being made on the message listener thread upon request ack. A more involved
scheme could drain callbacks to fire during the read loop. For now just make it all the
same behaviour as in 2025.1.0.
* Use the read buffer in UploadFile for the SFTP write packets
In SftpClient.UploadFile, a buffer is allocated to read from the given stream, and for
each read, another array is allocated for the SFTP write packet (which consists of that
data prepended with headers). This change effectively leaves space at the start of the
buffer for the headers such that it can be used to assemble the packets without that
per-packet array allocation.
There are cleaner/more general ways to do this (e.g. for all packet types, leave space
for the SSH headers as well), but this gets the most impact for about as much effort as
I can be bothered with.
* Rent from pool
* Initial plan
* Refactor collection assertions to use CollectionAssert.AreEqual
Replace Assert.IsTrue(xxx.IsEqualTo(yyy)) and Assert.IsTrue(xxx.SequenceEqual(yyy))
with CollectionAssert.AreEqual(expected, actual) across 55 test files
Co-authored-by: Rob-Hague <5132141+Rob-Hague@users.noreply.github.com>
* Fix argument order in CollectionAssert.AreEqual and remove unnecessary using directives
- Fixed argument order in KeyExchangeDhGroupExchangeReplyTest.cs (expected first, actual second)
- Fixed argument order in KeyExchangeInitMessageTest.cs (expected first, actual second)
- Removed unnecessary 'using System.Linq' directives from all affected test files
Co-authored-by: Rob-Hague <5132141+Rob-Hague@users.noreply.github.com>
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Rob-Hague <5132141+Rob-Hague@users.noreply.github.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Bump the dependencies group with 11 updates
Bumps coverlet.collector from 6.0.4 to 10.0.1
Bumps coverlet.msbuild from 6.0.4 to 10.0.1
Bumps GitHubActionsTestLogger from 3.0.1 to 3.0.4
Bumps Meziantou.Analyzer from 3.0.18 to 3.0.114
Bumps Microsoft.Bcl.Cryptography from 10.0.3 to 10.0.9
Bumps Microsoft.Extensions.Logging.Console from 10.0.3 to 10.0.9
Bumps MSTest from 4.1.0 to 4.2.3
Bumps PolySharp from 1.15.0 to 1.16.0
Bumps SonarAnalyzer.CSharp from 10.20.0.135146 to 10.27.0.140913
Bumps System.Formats.Asn1 from 10.0.3 to 10.0.9
Bumps Testcontainers from 4.10.0 to 4.12.0
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Robert Hague <rh@johnstreetcapital.com>
#1752 added a persistent buffer into which to decrypt packets, rather than allocating
a new array for each packet. This was on the back of #1733 which added support in the
cipher types for decrypting into a given buffer, but for the case of AES-CTR, not into
the same buffer in-place. #1787 adds that missing support, meaning we can now decrypt
in-place, and the plaintext buffer becomes unnecessary.
Support in-place encryption in the cipher types, then use it on the plaintext packets
instead of allocating a new array each time. Removes 2 of 4 bytes allocated for each
byte uploaded over SFTP.
For AES-CTR, supporting in-place encryption in this case means adding a persistent buffer
for the keystream and encrypting in chunks. The performance difference is ~1-2% i.e.
marginal versus one-shotting it. The variance is similar also for different choices of
buffer size (here 4096 is used).
* fix Build with newer .NET 10 SDKs
The IDE0370 are a mess since they only affect certain target frameworks.
Maybe we should disable this one completely instead?
Also set a fixed SDK Version in CI so this doesn't randomly break again.
* Remove IDE0370
* global.json: use latestMinor
to make sure that dotnet-setup installs the
exact version, see https://github.com/sshnet/SSH.NET/pull/1772#discussion_r2941495945
* Update global.json
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
---------
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
* Bump the dependencies group
---------
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Robert Hague <rh@johnstreetcapital.com>
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
Currently an array is allocated to read each packet from the socket, followed by
decryption which allocates another array for the plaintext payload. We can save one
of these two allocations by adding a persistent buffer for socket receives, and
allowing the cipher implementations to decrypt into the given payload array.
We can save the other allocation similarly, but in a separate change.
Several commonly used Write methods on SshDataStream end up calling Write(ReadOnlySpan)
on the base MemoryStream. But since SshDataStream is a derived type, that method just
rents a buffer and hands it to Write(byte[], int, int), which defeats any stackalloc'ing
or renting that SshDataStream does itself. Instead, with a bit extra accounting we can
just write directly into the underlying buffer.
An SFTP download performs several reads from the server in parallel, allocating an array
to store each result until it's ready to be consumed. Since these buffers are short-lived
and normally of the same large-ish size (32KB), it seems like a good candidate for pooling.
Rob Hague