From 7f839b7b4ea1fcb5a517a81018c31697177b824e Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Fri, 2 Oct 2026 22:07:57 +0200 Subject: [PATCH 1/6] Fix ScpClientTest.CalculateMD5 --- .../OldIntegrationTests/ScpClientTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/ScpClientTest.cs b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/ScpClientTest.cs index 702139c05..116bf9f06 100644 --- a/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/ScpClientTest.cs +++ b/test/Renci.SshNet.IntegrationTests/OldIntegrationTests/ScpClientTest.cs @@ -311,7 +311,7 @@ protected static string CalculateMD5(string fileName) for (var i = 0; i < hash.Length; i++) { - _ = sb.Append(i.ToString("x2")); + _ = sb.Append(hash[i].ToString("x2")); } return sb.ToString(); From 616d3d2ed9801fc59d259d91d19ea32322c62e50 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 08:45:39 +0000 Subject: [PATCH 2/6] Fail SCP download when the channel closes mid-file 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. --- src/Renci.SshNet/ScpClient.cs | 5 + ...Download_ChannelClosedDuringFileContent.cs | 76 +++++++++++ .../Common/ScpClientTranscriptTestBase.cs | 124 +++++++++++++++++ .../Common/ScpServerTranscript.cs | 129 ++++++++++++++++++ .../Common/ScpTranscriptResult.cs | 39 ++++++ 5 files changed, 373 insertions(+) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs create mode 100644 test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs create mode 100644 test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs create mode 100644 test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index 275e000ae..20704502a 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -862,6 +862,11 @@ private void InternalDownload(IChannel channel, Stream input, Stream output, str { var read = input.Read(buffer, 0, (int)Math.Min(needToRead, BufferSize)); + if (read == 0 && needToRead > 0) + { + throw new SshException("Stream has been closed."); + } + output.Write(buffer, 0, read); RaiseDownloadingEvent(filename, length, length - needToRead); diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs new file mode 100644 index 000000000..d84533100 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ChannelClosedDuringFileContent.cs @@ -0,0 +1,76 @@ +using System; +using System.IO; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Verifies that a download fails, rather than spinning forever, when the channel is closed + /// before all of the announced file content has been received. + /// + [TestClass] + public class ScpClientTest_Download_ChannelClosedDuringFileContent : ScpClientTranscriptTestBase + { + [TestMethod] + public void DownloadDirectoryInfo_ShouldThrow() + { + var transcript = new ScpServerTranscript() + .Line("D0755 0 remote") + .Line("C0644 100 file") + .Raw("0123456789"); + + AssertThrows(transcript, client => client.Download("/remote", new DirectoryInfo(Destination))); + } + + [TestMethod] + public void DownloadFileInfo_ShouldThrow() + { + var transcript = new ScpServerTranscript() + .Line("C0644 100 file") + .Raw("0123456789"); + + AssertThrows(transcript, client => client.Download("/remote/file", new FileInfo(Path.Combine(Destination, "file")))); + } + + [TestMethod] + public void DownloadStream_ShouldThrow() + { + var transcript = new ScpServerTranscript() + .Line("C0644 100 file") + .Raw("0123456789"); + + using (var destination = new MemoryStream()) + { + AssertThrows(transcript, client => client.Download("/remote/file", destination)); + } + } + + [TestMethod] + public void DownloadEmptyFile_ShouldStillSucceed() + { + var transcript = new ScpServerTranscript() + .File("file", string.Empty); + + using (var destination = new MemoryStream()) + { + var result = transcript.Run(client => client.Download("/remote/file", destination), closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsNull(result.Exception, result.Exception?.ToString()); + Assert.AreEqual(0, destination.Length); + } + } + + private static void AssertThrows(ScpServerTranscript transcript, Action download) + { + var result = transcript.Run(download, closeChannel: true, timeout: TimeSpan.FromSeconds(5)); + + Assert.IsTrue(result.Completed); + Assert.IsInstanceOfType(result.Exception); + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs b/test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs new file mode 100644 index 000000000..bcf83de9b --- /dev/null +++ b/test/Renci.SshNet.Tests/Common/ScpClientTranscriptTestBase.cs @@ -0,0 +1,124 @@ +using System; +using System.IO; +using System.Linq; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Renci.SshNet.Tests.Common +{ + /// + /// Base class for tests that replay a scripted server transcript (see ) + /// into a download, with an isolated destination directory. + /// + /// + /// The destination is <root>/a/b/dest below a unique temporary root, so that anything + /// written above it, into a/b or a, is detected by . + /// + public abstract class ScpClientTranscriptTestBase + { + /// + /// Gets the temporary root directory, which is deleted after each test. + /// + protected string Root { get; private set; } + + /// + /// Gets the destination directory, <root>/a/b/dest. + /// + protected string Destination { get; private set; } + + /// + /// Gets the parent of the destination directory, <root>/a/b. + /// + protected string DestinationParent + { + get + { + return Path.GetDirectoryName(Destination); + } + } + + [TestInitialize] + public void CreateDestination() + { + Root = Path.Combine(Path.GetTempPath(), "sshnet-scp-" + Guid.NewGuid().ToString("N")); + Destination = Directory.CreateDirectory(Path.Combine(Root, "a", "b", "dest")).FullName; + } + + [TestCleanup] + public void DeleteRoot() + { + if (Root != null && Directory.Exists(Root)) + { + Directory.Delete(LiteralPath(Root), recursive: true); + } + } + + /// + /// Returns the same path with the \\?\ prefix on Windows, which turns off Win32 path + /// normalization, so that entries with names such as file. or CON (which some + /// tests create on purpose) can be read and deleted. + /// + protected static string LiteralPath(string path) + { + return Path.DirectorySeparatorChar == '\\' ? @"\\?\" + path : path; + } + + /// + /// Lists every file system entry below , as paths relative to it using + /// '/' as separator, in no particular order. + /// + protected static string[] ListEntries(string directory) + { + return Directory.GetFileSystemEntries(directory, "*", SearchOption.AllDirectories) + .Select(p => p.Substring(directory.Length + 1).Replace(Path.DirectorySeparatorChar, '/')) + .ToArray(); + } + + /// + /// Runs a recursive download of into the destination directory, or + /// into if specified (for example, the destination spelled + /// differently). + /// + protected ScpTranscriptResult DownloadDirectory(ScpServerTranscript transcript, string destinationPath = null) + { + return transcript.Run( + client => client.Download("/remote", new DirectoryInfo(destinationPath ?? Destination)), + closeChannel: true); + } + + /// + /// Asserts that nothing exists below the root other than the directories leading to the + /// destination, the contents of the destination, and (a path + /// relative to the root, using '/' as separator), if specified. + /// + /// An entry outside of the destination that the test created itself. + /// A description of the scenario, included in the failure message. + protected void AssertNothingOutsideDestination(string expectedOutside = null, string context = null) + { + var outside = ListEntries(Root).Where(p => p is not "a" and not "a/b" and not "a/b/dest" && + !p.StartsWith("a/b/dest/", StringComparison.Ordinal) && + !string.Equals(p, expectedOutside, StringComparison.Ordinal)) + .ToArray(); + + Assert.IsEmpty(outside, context + " Written outside of the destination: " + string.Join(", ", outside)); + } + + /// + /// Deletes everything in the destination directory. + /// + protected void ClearDestination() + { + foreach (var entry in Directory.GetFileSystemEntries(LiteralPath(Destination))) + { + if (Directory.Exists(entry)) + { + Directory.Delete(entry, recursive: true); + } + else + { + File.Delete(entry); + } + } + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs b/test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs new file mode 100644 index 000000000..3fdd44982 --- /dev/null +++ b/test/Renci.SshNet.Tests/Common/ScpServerTranscript.cs @@ -0,0 +1,129 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +using Microsoft.Extensions.Logging.Abstractions; + +using Moq; + +using Renci.SshNet.Channels; +using Renci.SshNet.Common; +using Renci.SshNet.Connection; + +namespace Renci.SshNet.Tests.Common +{ + /// + /// Builds the byte stream an SCP server sends to the client and replays it into a real + /// through mocked session and channel objects. + /// + public sealed class ScpServerTranscript + { + private readonly List _bytes = new List(); + + /// + /// Appends a control line, terminated by a line feed. + /// + public ScpServerTranscript Line(string line) + { + return Raw(line + "\n"); + } + + /// + /// Appends encoded as UTF-8, which is the default + /// . + /// + public ScpServerTranscript Raw(string data) + { + return Raw(Encoding.UTF8.GetBytes(data)); + } + + public ScpServerTranscript Raw(byte[] data) + { + _bytes.AddRange(data); + return this; + } + + /// + /// Appends a C (file) record, the file content, and the status byte that follows it. + /// + public ScpServerTranscript File(string name, string content) + { + var bytes = Encoding.UTF8.GetBytes(content); + var length = bytes.Length.ToString(CultureInfo.InvariantCulture); + + return Line($"C0644 {length} {name}").Raw(bytes).Raw([0]); + } + + /// + /// Replays the transcript into a connected and runs + /// on it. + /// + /// The client operation to run. + /// + /// to signal end of stream after the transcript, as when the + /// server closes the channel; otherwise a read past the end blocks. + /// + /// How long to wait for to finish. + public ScpTranscriptResult Run(Action operation, bool closeChannel = false, TimeSpan? timeout = null) + { + var connectionInfo = new ConnectionInfo("host", 22, "user", new PasswordAuthenticationMethod("user", "pwd")); + var serviceFactoryMock = new Mock(MockBehavior.Strict); + var socketFactoryMock = new Mock(MockBehavior.Strict); + var sessionMock = new Mock(MockBehavior.Strict); + var channelMock = new Mock(MockBehavior.Strict); + var remotePathTransformationMock = new Mock(MockBehavior.Strict); + var sentData = new List(); + var execRequests = new List(); + + var pipeStream = new PipeStream(); + pipeStream.Write(_bytes.ToArray(), 0, _bytes.Count); + if (closeChannel) + { + pipeStream.Dispose(); + } + + _ = sessionMock.Setup(p => p.SessionLoggerFactory).Returns(NullLoggerFactory.Instance); + _ = serviceFactoryMock.Setup(p => p.CreateSocketFactory()).Returns(socketFactoryMock.Object); + _ = serviceFactoryMock.Setup(p => p.CreateSession(connectionInfo, socketFactoryMock.Object)).Returns(sessionMock.Object); + _ = sessionMock.Setup(p => p.Connect()); + _ = serviceFactoryMock.Setup(p => p.CreatePipeStream()).Returns(pipeStream); + _ = sessionMock.Setup(p => p.CreateChannelSession()).Returns(channelMock.Object); + _ = channelMock.Setup(p => p.Open()); + _ = remotePathTransformationMock.Setup(p => p.Transform(It.IsAny())).Returns(path => path); + _ = channelMock.Setup(p => p.SendExecRequest(It.IsAny())) + .Callback(execRequests.Add) + .Returns(true); + _ = channelMock.Setup(p => p.SendData(It.IsAny())).Callback(sentData.Add); + _ = channelMock.Setup(p => p.SendData(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((data, offset, count) => sentData.Add(data.Skip(offset).Take(count).ToArray())); + _ = channelMock.Setup(p => p.Dispose()); + + var scpClient = new ScpClient(connectionInfo, false, serviceFactoryMock.Object, remotePathTransformationMock.Object); + scpClient.Connect(); + + Exception exception = null; + var task = Task.Run(() => + { + try + { + operation(scpClient); + } + catch (Exception ex) + { + exception = ex; + } + }); + + var completed = task.Wait(timeout ?? TimeSpan.FromSeconds(10)); + + pipeStream.Dispose(); + + _ = task.Wait(TimeSpan.FromSeconds(10)); + + return new ScpTranscriptResult(completed, exception, sentData, execRequests); + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs b/test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs new file mode 100644 index 000000000..425c994f9 --- /dev/null +++ b/test/Renci.SshNet.Tests/Common/ScpTranscriptResult.cs @@ -0,0 +1,39 @@ +using System; +using System.Collections.Generic; + +namespace Renci.SshNet.Tests.Common +{ + /// + /// The outcome of . + /// + public sealed class ScpTranscriptResult + { + public ScpTranscriptResult(bool completed, Exception exception, IReadOnlyList sentData, IReadOnlyList execRequests) + { + Completed = completed; + Exception = exception; + SentData = sentData; + ExecRequests = execRequests; + } + + /// + /// Gets a value indicating whether the operation finished within the timeout. + /// + public bool Completed { get; } + + /// + /// Gets the exception thrown by the operation, if any. + /// + public Exception Exception { get; } + + /// + /// Gets the data the client sent on the channel, one entry per send. + /// + public IReadOnlyList SentData { get; } + + /// + /// Gets the commands the client sent in exec requests. + /// + public IReadOnlyList ExecRequests { get; } + } +} From ed801258afce4d29f39ea560c4791b21faf9d2a8 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 1 Oct 2026 21:00:57 +0000 Subject: [PATCH 3/6] Limit the length of SCP control lines 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. --- src/Renci.SshNet/ScpClient.cs | 26 +++++++-- ...pClientTest_Download_ControlLineTooLong.cs | 54 +++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ControlLineTooLong.cs diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index 20704502a..a630bb182 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -5,6 +5,9 @@ using System.Globalization; using System.IO; using System.Net; +#if NET +using System.Runtime.InteropServices; +#endif using System.Text.RegularExpressions; using System.Threading; @@ -86,6 +89,12 @@ public partial class ScpClient : BaseClient private static readonly Regex TimestampRegex = new Regex(TimestampPattern, RegexOptions.Compiled); #endif + /// + /// The maximum length, in bytes and excluding the terminating line feed, of a control line + /// or error message received from the server. + /// + internal const int MaxControlLineLength = 64 * 1024; + private static readonly byte[] SuccessConfirmationCode = { 0 }; private static readonly byte[] ErrorConfirmationCode = { 1 }; @@ -753,18 +762,25 @@ private string ReadString(Stream stream) while (b != SshNet.Session.LineFeed) { + if (buffer.Count == MaxControlLineLength) + { + throw new ScpException(string.Format("The server sent a line that exceeds the maximum length of {0} bytes.", MaxControlLineLength)); + } + buffer.Add((byte)b); b = ReadByte(stream); } - - var readBytes = buffer.ToArray(); - +#if NET + var line = ConnectionInfo.Encoding.GetString(CollectionsMarshal.AsSpan(buffer)); +#else + var line = ConnectionInfo.Encoding.GetString(buffer.ToArray(), 0, buffer.Count); +#endif if (hasError) { - throw new ScpException(ConnectionInfo.Encoding.GetString(readBytes, 0, readBytes.Length)); + throw new ScpException(line); } - return ConnectionInfo.Encoding.GetString(readBytes, 0, readBytes.Length); + return line; } /// diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ControlLineTooLong.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ControlLineTooLong.cs new file mode 100644 index 000000000..ed9bd6df0 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_ControlLineTooLong.cs @@ -0,0 +1,54 @@ +using System; +using System.IO; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Verifies that the length of a control line received from the server is bounded. + /// + [TestClass] + public class ScpClientTest_Download_ControlLineTooLong + { + [TestMethod] + public void LineLongerThanMaximum_ShouldThrowScpException() + { + var transcript = new ScpServerTranscript() + .Raw("C0644 1 " + new string('a', ScpClient.MaxControlLineLength)) + .Raw("\n"); + + using (var destination = new MemoryStream()) + { + var result = transcript.Run(client => client.Download("/remote/file", destination), closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsInstanceOfType(result.Exception); + Assert.Contains("maximum length", result.Exception.Message, StringComparison.Ordinal); + } + } + + [TestMethod] + public void LineOfMaximumLength_ShouldBeAccepted() + { + const string Header = "C0644 1 "; + + var transcript = new ScpServerTranscript() + .Line(Header + new string('a', ScpClient.MaxControlLineLength - Header.Length)) + .Raw("x") + .Raw([0]); + + using (var destination = new MemoryStream()) + { + var result = transcript.Run(client => client.Download("/remote/file", destination), closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsNull(result.Exception, result.Exception?.ToString()); + Assert.AreEqual(1, destination.Length); + } + } + } +} From d8f64d9d5b331557ff215ece5155821bc15847be Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 08:49:53 +0000 Subject: [PATCH 4/6] Reject line feeds in names sent in SCP upload records 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. --- .../Abstractions/StringExtensions.cs | 5 + src/Renci.SshNet/Common/PosixPath.cs | 31 +++-- src/Renci.SshNet/ScpClient.cs | 58 ++++++-- .../Renci.SshNet.IntegrationTests/ScpTests.cs | 123 ++++++++++++++++ .../ScpClientTest_Upload_InvalidName.cs | 131 ++++++++++++++++++ 5 files changed, 326 insertions(+), 22 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Upload_InvalidName.cs diff --git a/src/Renci.SshNet/Abstractions/StringExtensions.cs b/src/Renci.SshNet/Abstractions/StringExtensions.cs index 900a37e3c..d62c5684c 100644 --- a/src/Renci.SshNet/Abstractions/StringExtensions.cs +++ b/src/Renci.SshNet/Abstractions/StringExtensions.cs @@ -29,6 +29,11 @@ public int IndexOf(char value, StringComparison comparisonType) { return text.IndexOf(value.ToString(), comparisonType); } + + public bool Contains(char value) + { + return text.IndexOf(value) >= 0; + } #endif } } diff --git a/src/Renci.SshNet/Common/PosixPath.cs b/src/Renci.SshNet/Common/PosixPath.cs index 3a487b1ed..122ddd5cb 100644 --- a/src/Renci.SshNet/Common/PosixPath.cs +++ b/src/Renci.SshNet/Common/PosixPath.cs @@ -1,4 +1,5 @@ -using System; +#nullable enable +using System; namespace Renci.SshNet.Common { @@ -7,8 +8,10 @@ namespace Renci.SshNet.Common /// internal sealed class PosixPath { - private PosixPath() + private PosixPath(string directory, string? file) { + Directory = directory; + File = file; } /// @@ -17,7 +20,7 @@ private PosixPath() /// /// The directory of the path. /// - public string Directory { get; private set; } + public string Directory { get; } /// /// Gets the file part of the path. @@ -25,7 +28,7 @@ private PosixPath() /// /// The file part of the path, or if the path represents a directory. /// - public string File { get; private set; } + public string? File { get; } /// /// Create a from the specified path. @@ -40,7 +43,8 @@ public static PosixPath CreateAbsoluteOrRelativeFilePath(string path) { ArgumentNullException.ThrowIfNull(path); - var posixPath = new PosixPath(); + string directory; + string? file = null; var pathEnd = path.LastIndexOf('/'); if (pathEnd == -1) @@ -50,27 +54,28 @@ public static PosixPath CreateAbsoluteOrRelativeFilePath(string path) throw new ArgumentException("The path is a zero-length string.", nameof(path)); } - posixPath.Directory = "."; - posixPath.File = path; + directory = "."; + file = path; } else if (pathEnd == 0) { - posixPath.Directory = "/"; + directory = "/"; + if (path.Length > 1) { - posixPath.File = path.Substring(pathEnd + 1); + file = path.Substring(pathEnd + 1); } } else { - posixPath.Directory = path.Substring(0, pathEnd); + directory = path.Substring(0, pathEnd); if (pathEnd < path.Length - 1) { - posixPath.File = path.Substring(pathEnd + 1); + file = path.Substring(pathEnd + 1); } } - return posixPath; + return new PosixPath(directory, file); } /// @@ -87,7 +92,7 @@ public static PosixPath CreateAbsoluteOrRelativeFilePath(string path) /// is returned. /// /// - /// If path has a trailing slash, return a zero-length string. + /// If path has a trailing slash, returns a zero-length string. /// /// public static string GetFileName(string path) diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index a630bb182..dda845915 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -351,7 +351,10 @@ internal ScpClient(ConnectionInfo connectionInfo, bool ownsConnectionInfo, IServ /// The to upload. /// A relative or absolute path for the remote file. /// is . - /// is a zero-length . + /// + /// is a zero-length , does not have a file name part (for example, + /// it ends with /), or its file name part contains a line feed character. + /// /// A directory with the specified path exists on the remote host. /// The secure copy execution request was rejected by the server. /// Client is not connected. @@ -362,7 +365,7 @@ public void Upload(Stream source, string path) throw new SshConnectionException("Client not connected."); } - var posixPath = PosixPath.CreateAbsoluteOrRelativeFilePath(path); + var (remoteDirectory, remoteFileName) = GetUploadPathParts(path); using (var input = ServiceFactory.CreatePipeStream()) using (var channel = Session.CreateChannelSession()) @@ -373,15 +376,15 @@ public void Upload(Stream source, string path) // Pass only the directory part of the path to the server, and optionally use the (hidden) -d option to signal // that we expect the target to be a directory. - if (!channel.SendExecRequest($"scp -t {EnsureIsDirectoryArg} {_remotePathTransformation.Transform(posixPath.Directory)}")) + if (!channel.SendExecRequest($"scp -t {EnsureIsDirectoryArg} {_remotePathTransformation.Transform(remoteDirectory)}")) { throw SecureExecutionRequestRejectedException(); } CheckReturnCode(input); - UploadFileModeAndName(channel, input, source.Length, posixPath.File); - UploadFileContent(channel, input, source, posixPath.File); + UploadFileModeAndName(channel, input, source.Length, remoteFileName); + UploadFileContent(channel, input, source, remoteFileName); } } @@ -392,7 +395,10 @@ public void Upload(Stream source, string path) /// A relative or absolute path for the remote file. /// is . /// is . - /// is a zero-length . + /// + /// is a zero-length , does not have a file name part (for example, + /// it ends with /), or its file name part contains a line feed character. + /// /// A directory with the specified path exists on the remote host. /// The secure copy execution request was rejected by the server. /// Client is not connected. @@ -405,7 +411,7 @@ public void Upload(FileInfo fileInfo, string path) throw new SshConnectionException("Client not connected."); } - var posixPath = PosixPath.CreateAbsoluteOrRelativeFilePath(path); + var (remoteDirectory, remoteFileName) = GetUploadPathParts(path); using (var input = ServiceFactory.CreatePipeStream()) using (var channel = Session.CreateChannelSession()) @@ -416,7 +422,7 @@ public void Upload(FileInfo fileInfo, string path) // Pass only the directory part of the path to the server, and optionally use the (hidden) -d option to signal // that we expect the target to be a directory. - if (!channel.SendExecRequest($"scp -t {EnsureIsDirectoryArg} {_remotePathTransformation.Transform(posixPath.Directory)}")) + if (!channel.SendExecRequest($"scp -t {EnsureIsDirectoryArg} {_remotePathTransformation.Transform(remoteDirectory)}")) { throw SecureExecutionRequestRejectedException(); } @@ -426,7 +432,7 @@ public void Upload(FileInfo fileInfo, string path) using (var source = fileInfo.OpenRead()) { UploadTimes(channel, input, fileInfo); - UploadFileModeAndName(channel, input, source.Length, posixPath.File); + UploadFileModeAndName(channel, input, source.Length, remoteFileName); UploadFileContent(channel, input, source, fileInfo.Name); } } @@ -811,6 +817,8 @@ private void UploadDirectoryContent(IChannelSession channel, Stream input, Direc var files = directoryInfo.GetFiles(); foreach (var file in files) { + EnsureValidRecordName(file.Name); + using (var source = file.OpenRead()) { UploadTimes(channel, input, file); @@ -823,6 +831,8 @@ private void UploadDirectoryContent(IChannelSession channel, Stream input, Direc var directories = directoryInfo.GetDirectories(); foreach (var directory in directories) { + EnsureValidRecordName(directory.Name); + UploadTimes(channel, input, directory); UploadDirectoryModeAndName(channel, input, directory.Name); UploadDirectoryContent(channel, input, directory); @@ -831,6 +841,14 @@ private void UploadDirectoryContent(IChannelSession channel, Stream input, Direc // Mark upload of current directory complete SendData(channel, "E\n"); CheckReturnCode(input); + + static void EnsureValidRecordName(string name) + { + if (name.Contains('\n')) + { + throw new ScpException($"The name \"{name}\" contains a line feed character and cannot be sent to the server."); + } + } } /// @@ -842,6 +860,28 @@ private void UploadDirectoryModeAndName(IChannelSession channel, Stream input, s CheckReturnCode(input); } + /// + /// Splits the remote path passed to or + /// into its directory and file name parts, and ensures + /// that the file name part can be sent in an SCP C record. + /// + private static (string Directory, string FileName) GetUploadPathParts(string path) + { + var posixPath = PosixPath.CreateAbsoluteOrRelativeFilePath(path); + + if (posixPath.File is null) + { + throw new ArgumentException("The path must include a file name.", nameof(path)); + } + + if (posixPath.File.Contains('\n')) + { + throw new ArgumentException("The file name part of the path must not contain a line feed character.", nameof(path)); + } + + return (posixPath.Directory, posixPath.File); + } + /// /// Ensures that a file or directory name received from the remote host in the SCP /// protocol stream is a plain local name that cannot redirect a write outside of the diff --git a/test/Renci.SshNet.IntegrationTests/ScpTests.cs b/test/Renci.SshNet.IntegrationTests/ScpTests.cs index e19f603d4..971a624ac 100644 --- a/test/Renci.SshNet.IntegrationTests/ScpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ScpTests.cs @@ -2070,6 +2070,129 @@ public void Scp_Download_DirectoryInfo_ServerNameWithSeparator_StaysInsideDestin } } + /// + /// A line feed in the file name part of the upload path must not be sent in the C record. It would + /// end the record early: the server reads the rest of the name as the start of the file content, + /// and content crafted to match is then read as further records, creating another file. + /// + [TestMethod] + public void Scp_Upload_Stream_FileNameWithLineFeed_DoesNotCreateFiles() + { + // The server reads the rest of the name (22 x's) and the line feed that ends the record as the + // start of the 29 bytes of content, followed by "report". It then reads the "\0" as the status + // byte that ends the file, and the remaining 22 bytes as a C record for extra.txt and its content. + var fileName = "report.txt\n" + new string('x', 22); + var content = "report\0C0644 4 extra.txt\ndata"u8.ToArray(); + + AssertNameWithLineFeedDoesNotCreateFiles( + (client, remoteDirectory) => + { + using (var source = new MemoryStream(content)) + { + client.Upload(source, remoteDirectory + "/" + fileName); + } + }); + } + + /// + [TestMethod] + public void Scp_Upload_FileInfo_FileNameWithLineFeed_DoesNotCreateFiles() + { + var localFile = Path.GetTempFileName(); + File.WriteAllBytes(localFile, "report\0C0644 4 extra.txt\ndata"u8.ToArray()); + + try + { + AssertNameWithLineFeedDoesNotCreateFiles((client, remoteDirectory) + => client.Upload(new FileInfo(localFile), remoteDirectory + "/report.txt\n" + new string('x', 22))); + } + finally + { + File.Delete(localFile); + } + } + + /// + /// A local file whose name contains a line feed must not be sent in a C record when uploading a + /// directory; see . + /// + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, + IgnoreMessage = "A line feed is not valid in a file name on Windows.")] + public void Scp_Upload_DirectoryInfo_LocalFileNameWithLineFeed_DoesNotCreateFiles() + { + var localDirectory = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), "sshnet-scp-lf-" + Guid.NewGuid().ToString("N"))); + File.WriteAllBytes(Path.Combine(localDirectory.FullName, "report.txt\n" + new string('x', 22)), + "report\0C0644 4 extra.txt\ndata"u8.ToArray()); + + try + { + AssertNameWithLineFeedDoesNotCreateFiles((client, remoteDirectory) + => client.Upload(localDirectory, remoteDirectory)); + } + finally + { + localDirectory.Delete(recursive: true); + } + } + + /// + /// Runs into a new, empty remote directory, and asserts that it throws + /// without creating anything in that directory. + /// + private void AssertNameWithLineFeedDoesNotCreateFiles(Action upload) + where TException : Exception + { + var remoteDirectory = "/tmp/sshnet-scp-lf-" + Guid.NewGuid().ToString("N"); + + using (var client = new SshClient(_connectionInfoFactory.Create())) + { + client.Connect(); + _ = client.RunCommand("mkdir -p '" + remoteDirectory + "'"); + } + + try + { + Exception exception = null; + + using (var client = new ScpClient(_connectionInfoFactory.Create(), _remotePathTransformation)) + { + client.Connect(); + + try + { + upload(client, remoteDirectory); + } + catch (Exception ex) + { + exception = ex; + } + } + + using (var client = new SftpClient(_connectionInfoFactory.Create())) + { + client.Connect(); + + var entries = client.ListDirectory(remoteDirectory) + .Where(f => f.Name is not "." and not "..") + .Select(f => f.Name) + .ToArray(); + + Assert.IsEmpty(entries, "Files were created on the server: " + string.Join(", ", entries)); + } + + Assert.IsInstanceOfType(exception); + } + finally + { + using (var client = new SshClient(_connectionInfoFactory.Create())) + { + client.Connect(); + _ = client.RunCommand("rm -rf '" + remoteDirectory + "'"); + } + } + } + private static void CreateRemoteFile(ScpClient client, string remoteFile, int size) { var file = CreateTempFile(size); diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Upload_InvalidName.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Upload_InvalidName.cs new file mode 100644 index 000000000..7d0306b74 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Upload_InvalidName.cs @@ -0,0 +1,131 @@ +using System; +using System.IO; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Verifies that the upload methods reject names that cannot be sent in an SCP C or D record: a name + /// containing a line feed, or an empty file name (a remote path that ends with /). + /// + /// + /// A line feed would end the record early, so that the server reads the rest of the name as the + /// start of the file content, and content crafted to match is then read as further records. The + /// integration tests (ScpTests) show the effect on a real server: another file is created. + /// + [TestClass] + public class ScpClientTest_Upload_InvalidName : ScpClientTranscriptTestBase + { + [TestMethod] + public void UploadStream_FileNameWithLineFeed_ShouldThrowArgumentExceptionBeforeOpeningChannel() + { + using (var source = new MemoryStream("content"u8.ToArray())) + { + var result = new ScpServerTranscript().Run( + client => client.Upload(source, "/home/sshnet/report.txt\nrest-of-name"), + timeout: TimeSpan.FromSeconds(5)); + + AssertRejectedBeforeOpeningChannel(result); + } + } + + [TestMethod] + public void UploadFileInfo_FileNameWithLineFeed_ShouldThrowArgumentExceptionBeforeOpeningChannel() + { + var filePath = Path.Combine(Root, "file.txt"); + File.WriteAllText(filePath, "content"); + + var result = new ScpServerTranscript().Run( + client => client.Upload(new FileInfo(filePath), "/home/sshnet/report.txt\nrest-of-name"), + timeout: TimeSpan.FromSeconds(5)); + + AssertRejectedBeforeOpeningChannel(result); + } + + [TestMethod] + [DataRow("/home/sshnet/")] + [DataRow("dir/")] + [DataRow("/")] + public void UploadStream_PathWithoutFileName_ShouldThrowArgumentExceptionBeforeOpeningChannel(string remotePath) + { + using (var stream = new MemoryStream("content"u8.ToArray())) + { + var result = new ScpServerTranscript().Run( + client => client.Upload(stream, remotePath), + timeout: TimeSpan.FromSeconds(5)); + + AssertRejectedBeforeOpeningChannel(result); + } + } + + [TestMethod] + [DataRow("/home/sshnet/")] + [DataRow("dir/")] + public void UploadFileInfo_PathWithoutFileName_ShouldThrowArgumentExceptionBeforeOpeningChannel(string remotePath) + { + var localPath = Path.Combine(Root, "file.txt"); + File.WriteAllText(localPath, "content"); + + var result = new ScpServerTranscript().Run( + client => client.Upload(new FileInfo(localPath), remotePath), + timeout: TimeSpan.FromSeconds(5)); + + AssertRejectedBeforeOpeningChannel(result); + } + + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, + IgnoreMessage = "A line feed is not valid in a file name on Windows.")] + public void UploadDirectoryInfo_LocalFileNameWithLineFeed_ShouldThrowScpException() + { + var localDir = Directory.CreateDirectory(Path.Combine(Root, "source")); + File.WriteAllText(Path.Combine(localDir.FullName, "report.txt\nrest-of-name"), "content"); + + var transcript = new ScpServerTranscript().Raw([0]); // Status byte for the exec request. + + var result = transcript.Run( + client => client.Upload(localDir, "/home/sshnet/target"), + timeout: TimeSpan.FromSeconds(5)); + + AssertRejectedBeforeSendingAnyRecord(result); + } + + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, + IgnoreMessage = "A line feed is not valid in a directory name on Windows.")] + public void UploadDirectoryInfo_LocalDirectoryNameWithLineFeed_ShouldThrowScpException() + { + var localDir = Directory.CreateDirectory(Path.Combine(Root, "source")); + _ = Directory.CreateDirectory(Path.Combine(localDir.FullName, "dir\nrest-of-name")); + + var transcript = new ScpServerTranscript().Raw([0]); // Status byte for the exec request. + + var result = transcript.Run( + client => client.Upload(localDir, "/home/sshnet/target"), + timeout: TimeSpan.FromSeconds(5)); + + AssertRejectedBeforeSendingAnyRecord(result); + } + + private static void AssertRejectedBeforeOpeningChannel(ScpTranscriptResult result) + { + Assert.IsTrue(result.Completed); + var ex = Assert.IsInstanceOfType(result.Exception); + Assert.AreEqual("path", ex.ParamName); + Assert.IsEmpty(result.ExecRequests); + Assert.IsEmpty(result.SentData); + } + + private static void AssertRejectedBeforeSendingAnyRecord(ScpTranscriptResult result) + { + Assert.IsTrue(result.Completed); + Assert.IsInstanceOfType(result.Exception); + Assert.Contains("line feed", result.Exception.Message, StringComparison.Ordinal); + Assert.IsEmpty(result.SentData); + } + } +} From cf878c268418fe77f7e08a761bdf760e3d741d70 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 08:53:06 +0000 Subject: [PATCH 5/6] Tighten SCP record parsing and handling 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. --- src/Renci.SshNet/ScpClient.cs | 186 ++++++++++++----- .../Renci.SshNet.IntegrationTests/ScpTests.cs | 49 +++++ .../ScpClientTest_Download_InvalidRecord.cs | 194 ++++++++++++++++++ 3 files changed, 378 insertions(+), 51 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index dda845915..4afe49208 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; using System.Net; @@ -66,9 +67,9 @@ public partial class ScpClient : BaseClient "taking an IRemotePathTransformation which suits the escaping rules of the remote server and " + "the trust environment in which this code runs, and consider using SFTP where possible."; - private const string FileInfoPattern = @"C(?\d{4}) (?\d+) (?.+)"; - private const string DirectoryInfoPattern = @"D(?\d{4}) (?\d+) (?.+)"; - private const string TimestampPattern = @"T(?\d+) 0 (?\d+) 0"; + private const string FileInfoPattern = @"^C(?[0-7]{4}) (?[0-9]+) (?.+)$"; + private const string DirectoryInfoPattern = @"^D(?[0-7]{4}) (?[0-9]+) (?.+)$"; + private const string TimestampPattern = @"^T(?[0-9]+) 0 (?[0-9]+) 0$"; #if NET private static readonly Regex FileInfoRegex = GetFileInfoRegex(); @@ -489,7 +490,7 @@ public void Upload(DirectoryInfo directoryInfo, string path) /// Local file information. /// is . /// is or empty. - /// exists on the remote host, and is not a regular file. + /// exists on the remote host, and is not a regular file, or the server sent an invalid or unexpected SCP record. /// The secure copy execution request was rejected by the server. /// Client is not connected. public void Download(string filename, FileInfo fileInfo) @@ -518,7 +519,41 @@ public void Download(string filename, FileInfo fileInfo) // Send reply SendSuccessConfirmation(channel); - InternalDownload(channel, input, fileInfo); + var modifiedUtc = DateTime.UtcNow; + var accessedUtc = modifiedUtc; + + var message = ReadString(input); + + var match = TimestampRegex.Match(message); + if (match.Success) + { + if (!long.TryParse(match.Result("${mtime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var mtime) || + !long.TryParse(match.Result("${atime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var atime) || + !TryFromUnixTimeSeconds(mtime, out modifiedUtc) || + !TryFromUnixTimeSeconds(atime, out accessedUtc)) + { + SendInvalidRecordErrorAndThrow(channel); + } + + SendSuccessConfirmation(channel); + + message = ReadString(input); + } + + match = FileInfoRegex.Match(message); + if (match.Success && + long.TryParse(match.Result("${length}"), NumberStyles.None, CultureInfo.InvariantCulture, out var length)) + { + var fileName = match.Result("${filename}"); + + SendSuccessConfirmation(channel); + + DownloadFileContent(channel, input, fileInfo, fileName, length, modifiedUtc, accessedUtc); + } + else + { + SendInvalidRecordErrorAndThrow(channel); + } } } @@ -529,7 +564,7 @@ public void Download(string filename, FileInfo fileInfo) /// Local directory information. /// is or empty. /// is . - /// File or directory with the specified path does not exist on the remote host. + /// File or directory with the specified path does not exist on the remote host, or the server sent an invalid or unexpected SCP record. /// The secure copy execution request was rejected by the server. /// Client is not connected. public void Download(string directoryName, DirectoryInfo directoryInfo) @@ -558,7 +593,7 @@ public void Download(string directoryName, DirectoryInfo directoryInfo) // Send reply SendSuccessConfirmation(channel); - InternalDownload(channel, input, directoryInfo); + DownloadDirectoryContent(channel, input, directoryInfo); } } @@ -569,7 +604,7 @@ public void Download(string directoryName, DirectoryInfo directoryInfo) /// The to download the remote file to. /// is or contains only whitespace characters. /// is . - /// exists on the remote host, and is not a regular file. + /// exists on the remote host, and is not a regular file, or the server sent an invalid or unexpected SCP record. /// The secure copy execution request was rejected by the server. /// Client is not connected. public void Download(string filename, Stream destination) @@ -600,19 +635,19 @@ public void Download(string filename, Stream destination) var message = ReadString(input); var match = FileInfoRegex.Match(message); - if (match.Success) + if (match.Success && + long.TryParse(match.Result("${length}"), NumberStyles.None, CultureInfo.InvariantCulture, out var length)) { + var fileName = match.Result("${filename}"); + // Read file SendSuccessConfirmation(channel); // Send reply - var length = long.Parse(match.Result("${length}"), CultureInfo.InvariantCulture); - var fileName = match.Result("${filename}"); - - InternalDownload(channel, input, destination, fileName, length); + DownloadFileContent(channel, input, destination, fileName, length); } else { - SendErrorConfirmation(channel, string.Format("\"{0}\" is not valid protocol message.", message)); + SendInvalidRecordErrorAndThrow(channel); } } } @@ -746,6 +781,34 @@ private void SendData(IChannel channel, string command) channel.SendData(ConnectionInfo.Encoding.GetBytes(command)); } + [DoesNotReturn] + private void SendInvalidRecordErrorAndThrow(IChannel channel) + { + SendErrorConfirmation(channel, "Invalid or unexpected SCP record."); + + throw new ScpException("The server sent an invalid or unexpected SCP record."); + } + + /// + /// Converts a number of seconds since the Unix epoch, as sent in a T record, to a . + /// + /// The number of seconds since the Unix epoch. + /// The time, in UTC, if can be represented. + /// if can be represented as a ; otherwise, . + private static bool TryFromUnixTimeSeconds(long seconds, out DateTime dateTime) + { + try + { + dateTime = DateTimeOffset.FromUnixTimeSeconds(seconds).UtcDateTime; + return true; + } + catch (ArgumentOutOfRangeException) + { + dateTime = default; + return false; + } + } + /// /// Read a LF-terminated string from the . /// @@ -909,7 +972,11 @@ internal static void EnsureValidLocalName(string name) } } - private void InternalDownload(IChannel channel, Stream input, Stream output, string filename, long length) + /// + /// Downloads the content announced by a C record into a stream, and exchanges the confirmations + /// that follow it. + /// + private void DownloadFileContent(IChannel channel, Stream input, Stream output, string filename, long length) { var buffer = new byte[Math.Min(length, BufferSize)]; var needToRead = length; @@ -942,12 +1009,42 @@ private void InternalDownload(IChannel channel, Stream input, Stream output, str CheckReturnCode(input); } - private void InternalDownload(IChannelSession channel, Stream input, FileSystemInfo fileSystemInfo) + /// + /// Downloads the content announced by a C record into a local file, and applies the times. + /// + private void DownloadFileContent( + IChannelSession channel, + Stream input, + FileInfo fileInfo, + string fileName, + long length, + DateTime modifiedUtc, + DateTime accessedUtc) + { + Debug.Assert(modifiedUtc.Kind == DateTimeKind.Utc); + Debug.Assert(accessedUtc.Kind == DateTimeKind.Utc); + + using (var output = fileInfo.Open(FileMode.Create, FileAccess.Write)) + { + DownloadFileContent(channel, input, output, fileName, length); + } + + fileInfo.LastAccessTimeUtc = accessedUtc; + fileInfo.LastWriteTimeUtc = modifiedUtc; + } + + /// + /// Downloads the records and content of a directory tree (scp -prf) into a local directory. + /// + /// The channel to perform the download in. + /// A from which the server's records and content are read. + /// The local directory to download into. + private void DownloadDirectoryContent(IChannelSession channel, Stream input, DirectoryInfo directoryInfo) { - var modifiedTime = DateTime.Now; - var accessedTime = DateTime.Now; + var modifiedUtc = DateTime.UtcNow; + var accessedUtc = modifiedUtc; - var startDirectoryFullName = fileSystemInfo.FullName; + var startDirectoryFullName = directoryInfo.FullName; var currentDirectoryFullName = startDirectoryFullName; var directoryCounter = 0; @@ -991,13 +1088,13 @@ private void InternalDownload(IChannelSession channel, Stream input, FileSystemI EnsureValidLocalName(filename); newDirectoryInfo = Directory.CreateDirectory(Path.Combine(currentDirectoryFullName, filename)); - newDirectoryInfo.LastAccessTime = accessedTime; - newDirectoryInfo.LastWriteTime = modifiedTime; + newDirectoryInfo.LastAccessTimeUtc = accessedUtc; + newDirectoryInfo.LastWriteTimeUtc = modifiedUtc; } else { // Don't create directory for first level - newDirectoryInfo = (DirectoryInfo)fileSystemInfo; + newDirectoryInfo = directoryInfo; } directoryCounter++; @@ -1007,30 +1104,21 @@ private void InternalDownload(IChannelSession channel, Stream input, FileSystemI } match = FileInfoRegex.Match(message); - if (match.Success) + if (match.Success && + long.TryParse(match.Result("${length}"), NumberStyles.None, CultureInfo.InvariantCulture, out var length)) { - // Read file - SendSuccessConfirmation(channel); // Send reply - - var length = long.Parse(match.Result("${length}"), CultureInfo.InvariantCulture); var fileName = match.Result("${filename}"); - if (fileSystemInfo is not FileInfo fileInfo) - { - // The server-supplied name is combined into a local path; ensure it - // cannot escape the destination directory. - EnsureValidLocalName(fileName); + // Read file + SendSuccessConfirmation(channel); // Send reply - fileInfo = new FileInfo(Path.Combine(currentDirectoryFullName, fileName)); - } + // The server-supplied name is combined into a local path; ensure it + // cannot escape the destination directory. + EnsureValidLocalName(fileName); - using (var output = fileInfo.Open(FileMode.Create, FileAccess.Write)) - { - InternalDownload(channel, input, output, fileName, length); - } + var fileInfo = new FileInfo(Path.Combine(currentDirectoryFullName, fileName)); - fileInfo.LastAccessTime = accessedTime; - fileInfo.LastWriteTime = modifiedTime; + DownloadFileContent(channel, input, fileInfo, fileName, length, modifiedUtc, accessedUtc); if (directoryCounter == 0) { @@ -1041,21 +1129,17 @@ private void InternalDownload(IChannelSession channel, Stream input, FileSystemI } match = TimestampRegex.Match(message); - if (match.Success) + if (match.Success && + long.TryParse(match.Result("${mtime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var mtime) && + long.TryParse(match.Result("${atime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var atime) && + TryFromUnixTimeSeconds(mtime, out modifiedUtc) && + TryFromUnixTimeSeconds(atime, out accessedUtc)) { - // Read timestamp - SendSuccessConfirmation(channel); // Send reply - - var mtime = long.Parse(match.Result("${mtime}"), CultureInfo.InvariantCulture); - var atime = long.Parse(match.Result("${atime}"), CultureInfo.InvariantCulture); - - var zeroTime = DateTime.UnixEpoch; - modifiedTime = zeroTime.AddSeconds(mtime); - accessedTime = zeroTime.AddSeconds(atime); + SendSuccessConfirmation(channel); continue; } - SendErrorConfirmation(channel, string.Format("\"{0}\" is not valid protocol message.", message)); + SendInvalidRecordErrorAndThrow(channel); } } } diff --git a/test/Renci.SshNet.IntegrationTests/ScpTests.cs b/test/Renci.SshNet.IntegrationTests/ScpTests.cs index 971a624ac..5e6704d58 100644 --- a/test/Renci.SshNet.IntegrationTests/ScpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ScpTests.cs @@ -2070,6 +2070,55 @@ public void Scp_Download_DirectoryInfo_ServerNameWithSeparator_StaysInsideDestin } } + /// + /// A file whose name contains the text of a D record must be downloaded as a file, with its + /// content written verbatim rather than interpreted as SCP records. + /// + [TestMethod] + public void Scp_Download_DirectoryInfo_FileNameContainingRecordText_IsDownloadedAsFile() + { + const string FileName = "zD0755 0 x"; + const string Content = "E\nC0644 8 injected.txt\ninjected"; + + var remoteDirectory = "/tmp/sshnet-scp-records-" + Guid.NewGuid().ToString("N"); + + using (var client = new SftpClient(_connectionInfoFactory.Create())) + { + client.Connect(); + client.CreateDirectory(remoteDirectory); + client.WriteAllText(remoteDirectory + "/" + FileName, Content); + } + + var localRoot = Path.Combine(Path.GetTempPath(), "sshnet-scp-records-" + Guid.NewGuid().ToString("N")); + var destination = Directory.CreateDirectory(Path.Combine(localRoot, "download")); + + try + { + using (var client = new ScpClient(_connectionInfoFactory.Create(), _remotePathTransformation)) + { + client.Connect(); + client.Download(remoteDirectory, destination); + } + + var filePath = Path.Combine(destination.FullName, FileName); + + CollectionAssert.AreEquivalent( + new[] { destination.FullName, filePath }, + Directory.GetFileSystemEntries(localRoot, "*", SearchOption.AllDirectories)); + Assert.AreEqual(Content, File.ReadAllText(filePath)); + } + finally + { + using (var client = new SshClient(_connectionInfoFactory.Create())) + { + client.Connect(); + _ = client.RunCommand("rm -rf '" + remoteDirectory + "'"); + } + + Directory.Delete(localRoot, recursive: true); + } + } + /// /// A line feed in the file name part of the upload path must not be sent in the C record. It would /// end the record early: the server reads the rest of the name as the start of the file content, diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs new file mode 100644 index 000000000..dd348445e --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs @@ -0,0 +1,194 @@ +using System; +using System.IO; +using System.Linq; +using System.Text; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Verifies how downloads handle control records: names that contain record-like text, records + /// that are malformed or unknown, and records that are valid but not expected at that point. The + /// single-file downloads accept only the records that a server sends for a single file: for + /// a C record, and for + /// an optional T record followed by a C record. + /// + [TestClass] + public class ScpClientTest_Download_InvalidRecord : ScpClientTranscriptTestBase + { + private string Target + { + get + { + return Path.Combine(Destination, "target"); + } + } + + [TestMethod] + public void FileNameContainingDirectoryRecord_ShouldBeDownloadedAsFile() + { + // An honest server sends a file whose (legitimate) name contains the text of a D + // record. Its content must be written to that file, not interpreted as records. + const string Name = "zD0755 0 x"; + const string Content = "E\nC0644 8 injected.txt\ninjected"; + + var transcript = new ScpServerTranscript() + .Line("D0755 0 remote") + .File(Name, Content) + .Line("E"); + + var result = transcript.Run( + client => client.Download("/remote", new DirectoryInfo(Destination)), + closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { Name }, ListEntries(Destination)); + Assert.AreEqual(Content, File.ReadAllText(Path.Combine(Destination, Name))); + } + + [TestMethod] + [DataRow("Xjunk")] + [DataRow("garbageC0644 1 f")] + [DataRow("C0644 99999999999999999999 f")] + [DataRow("C0644 +1 f")] + [DataRow("C0899 1 f")] + [DataRow("T99999999999999999999 0 1 0")] + [DataRow("T999999999999 0 1 0")] + [DataRow("E trailing")] + [DataRow("C0644 1 f")] + [DataRow("C064 1 f")] + [DataRow("C0644 1 ")] + [DataRow("D0755 0")] + [DataRow("C0644 \u0663 f")] + [DataRow("C\u0660644 1 f")] + [DataRow("T1 0 2")] + [DataRow("T\u0661 0 2 0")] + public void DownloadDirectoryInfo_InvalidRecord_ShouldThrowScpException(string line) + { + var transcript = new ScpServerTranscript() + .Line("D0755 0 remote") + .Line(line) + .File("after.txt", "abc") + .Line("E"); + + var result = transcript.Run( + client => client.Download("/remote", new DirectoryInfo(Destination)), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 2); + AssertNothingOutsideDestination(); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + + [TestMethod] + [DataRow("Xjunk")] + [DataRow("C0644 99999999999999999999 f")] + [DataRow("T999999999999 0 1 0")] + [DataRow("D0755 0 dir")] + [DataRow("E")] + public void DownloadFileInfo_RecordOtherThanTimesOrFile_ShouldThrowScpException(string line) + { + var transcript = new ScpServerTranscript() + .Line(line) + .File("f", "abc"); + + var result = transcript.Run( + client => client.Download("/remote/f", new FileInfo(Target)), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 1); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + + [TestMethod] + [DataRow("T1 0 2 0")] + [DataRow("D0755 0 dir")] + [DataRow("E")] + [DataRow("Xjunk")] + public void DownloadFileInfo_TimesFollowedByRecordOtherThanFile_ShouldThrowScpException(string line) + { + // Only one T record is allowed, and it must be followed by the C record. + var transcript = new ScpServerTranscript() + .Line("T1700000000 0 1700000000 0") + .Line(line) + .File("f", "abc"); + + var result = transcript.Run( + client => client.Download("/remote/f", new FileInfo(Target)), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 2); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + + [TestMethod] + public void DownloadFileInfo_TimesThenFile_ShouldWriteToTargetIgnoringServerName() + { + var transcript = new ScpServerTranscript() + .Line("T1700000000 0 1700000000 0") + .File("server-name", "content"); + + var result = transcript.Run( + client => client.Download("/remote/f", new FileInfo(Target)), + closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { "target" }, ListEntries(Destination)); + Assert.AreEqual("content", File.ReadAllText(Target)); + Assert.AreEqual(DateTime.UnixEpoch.AddSeconds(1700000000), File.GetLastWriteTimeUtc(Target)); + } + + [TestMethod] + [DataRow("Xjunk")] + [DataRow("garbageC0644 1 f")] + [DataRow("C0644 99999999999999999999 f")] + [DataRow("C0644 9223372036854775808 f")] + [DataRow("D0755 0 dir")] + [DataRow("E")] + [DataRow("T1 0 2 0")] + public void DownloadStream_RecordOtherThanFile_ShouldThrowScpException(string line) + { + var transcript = new ScpServerTranscript() + .Line(line) + .File("f", "abc"); + + using (var destination = new MemoryStream()) + { + var result = transcript.Run( + client => client.Download("/remote/f", destination), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 1); + Assert.AreEqual(0, destination.Length); + } + } + + private static void AssertRejected(ScpTranscriptResult result, int expectedSuccessStatuses) + { + Assert.IsTrue(result.Completed); + Assert.IsInstanceOfType(result.Exception); + Assert.Contains("invalid or unexpected SCP record", result.Exception.Message, StringComparison.Ordinal); + + // The rejected record is not acknowledged: the only success status bytes are the one sent + // after the exec request and one for each record accepted before it. + Assert.AreEqual( + expectedSuccessStatuses, + result.SentData.Count(data => data.Length == 1 && data[0] == 0), + "Unexpected number of success status bytes; was the rejected record acknowledged?"); + + // The error is also reported to the server: a 0x01 status byte followed by a message. + Assert.IsTrue( + result.SentData.Any(data => data.Length == 1 && data[0] == 1), + "No error status was sent to the server."); + Assert.IsTrue( + result.SentData.Any(data => string.Equals(Encoding.UTF8.GetString(data), "Invalid or unexpected SCP record.\n", StringComparison.Ordinal)), + "No error message was sent to the server."); + } + } +} From 805a8849aa49a4160622c13945021f1c70d54bdd Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 09:04:40 +0000 Subject: [PATCH 6/6] Harden directory escape in SCP recursive download 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. --- src/Renci.SshNet/ScpClient.cs | 165 ++++++++--- .../Renci.SshNet.IntegrationTests/ScpTests.cs | 55 ++++ .../ScpClientTest_Download_InvalidRecord.cs | 20 ++ ...est_Download_PathAndDirectoryInfo_Names.cs | 262 ++++++++++++++++++ .../ScpClientTest_EnsureValidLocalName.cs | 102 ------- ...cpClientTest_ResolveChild_RootDirectory.cs | 35 +++ 6 files changed, 498 insertions(+), 141 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_Names.cs delete mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_ResolveChild_RootDirectory.cs diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index 4afe49208..074227531 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -564,9 +564,27 @@ public void Download(string filename, FileInfo fileInfo) /// Local directory information. /// is or empty. /// is . - /// File or directory with the specified path does not exist on the remote host, or the server sent an invalid or unexpected SCP record. + /// + /// File or directory with the specified path does not exist on the remote host, the server sent an invalid + /// or unexpected SCP record, or the server sent a file or directory name that is not a valid local name. + /// /// The secure copy execution request was rejected by the server. /// Client is not connected. + /// + /// + /// Each file and directory name sent by the server must be a plain name that refers to an entry + /// directly inside the directory being downloaded into. The download is aborted with an + /// if a name contains a directory separator or another character that is + /// not valid in a local file name, refers to the current or parent directory, or would be resolved by + /// the local file system to a different name or location, so that the names sent by the server cannot + /// refer to a location outside of . + /// + /// + /// Existing files in are overwritten by files with the same name, and + /// symbolic links and junctions that already exist within it are followed. If the download fails, the + /// files received so far, including part of the file being received, are left in place. + /// + /// public void Download(string directoryName, DirectoryInfo directoryInfo) { ArgumentException.ThrowIfNullOrEmpty(directoryName); @@ -946,30 +964,61 @@ private static (string Directory, string FileName) GetUploadPathParts(string pat } /// - /// Ensures that a file or directory name received from the remote host in the SCP - /// protocol stream is a plain local name that cannot redirect a write outside of the - /// caller-supplied destination directory. + /// Combines a file or directory name received from the remote host with a local directory, + /// ensuring that the result is a direct child of that directory with exactly that name. /// + /// The local directory, as returned by . /// The file or directory name as sent by the server. - /// - /// is empty, refers to the current or parent directory, or - /// contains a character that is not valid in a local file name (such as a directory - /// separator). - /// - internal static void EnsureValidLocalName(string name) + /// + /// The full path of the child, or if is empty, refers + /// to the current or parent directory, contains a character that is not valid in a local file name + /// (such as a directory separator), or the local file system would resolve it to a different name or + /// location (for example, Windows removes trailing dots and spaces, so "..." refers to + /// itself). + /// + internal static string? ResolveChild(string directory, string name) + { + if (string.IsNullOrEmpty(name) || name.IndexOfAny(InvalidLocalNameChars) >= 0) + { + return null; + } + + var path = Path.GetFullPath(Path.Combine(directory, name)); + + if (!string.Equals(Path.GetDirectoryName(path), directory, PathHelpers.StringComparison) || + !string.Equals(Path.GetFileName(path), name, PathHelpers.StringComparison)) + { + return null; + } + + return path; + } + + /// + /// Returns the full path of a local directory without a trailing directory separator, unless + /// the directory is a root. + /// + /// The path of the directory. + /// The normalized full path. + internal static string NormalizeDirectory(string path) { - // A legitimate SCP server only ever sends a single, plain path component in a - // C (file) or D (directory) record. Reject anything that is empty, refers to the - // current/parent directory, or carries a directory separator, drive qualifier or - // other character that is not valid in a local file name. Path.GetInvalidFileNameChars() - // is platform-aware: on Windows it includes '\', '/' and ':'; on Unix it includes '/'. - if (string.IsNullOrEmpty(name) || - name.Equals(".", StringComparison.Ordinal) || - name.Equals("..", StringComparison.Ordinal) || - name.IndexOfAny(InvalidLocalNameChars) >= 0) + var fullPath = Path.GetFullPath(path); + var root = Path.GetPathRoot(fullPath); + + if (root is not null && fullPath.Length > root.Length) { - throw new ScpException($"The server sent a file or directory name (\"{name}\") that is not a valid local name."); + fullPath = fullPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); } + + return fullPath; + } + + [DoesNotReturn] + private void SendInvalidLocalNameErrorAndThrow(IChannel channel) + { + SendErrorConfirmation(channel, "File or directory name is not a valid local name."); + + throw new ScpException("The server sent a file or directory name that is not a valid local name."); } /// @@ -1044,8 +1093,8 @@ private void DownloadDirectoryContent(IChannelSession channel, Stream input, Dir var modifiedUtc = DateTime.UtcNow; var accessedUtc = modifiedUtc; - var startDirectoryFullName = directoryInfo.FullName; - var currentDirectoryFullName = startDirectoryFullName; + var destination = NormalizeDirectory(directoryInfo.FullName); + var currentDirectoryFullName = destination; var directoryCounter = 0; while (true) @@ -1054,6 +1103,11 @@ private void DownloadDirectoryContent(IChannelSession channel, Stream input, Dir if (message == "E") { + if (directoryCounter <= 0) + { + SendInvalidRecordErrorAndThrow(channel); + } + SendSuccessConfirmation(channel); // Send reply directoryCounter--; @@ -1065,7 +1119,7 @@ private void DownloadDirectoryContent(IChannelSession channel, Stream input, Dir var currentDirectoryParent = new DirectoryInfo(currentDirectoryFullName).Parent; - Debug.Assert(currentDirectoryParent is not null, $"Should be {directoryCounter.ToString(CultureInfo.InvariantCulture)} levels deeper than {startDirectoryFullName}."); + Debug.Assert(currentDirectoryParent is not null, $"Should be {directoryCounter.ToString(CultureInfo.InvariantCulture)} levels deeper than {destination}."); currentDirectoryFullName = currentDirectoryParent.FullName; @@ -1075,31 +1129,35 @@ private void DownloadDirectoryContent(IChannelSession channel, Stream input, Dir var match = DirectoryInfoRegex.Match(message); if (match.Success) { - SendSuccessConfirmation(channel); // Send reply - // Read directory var filename = match.Result("${filename}"); - DirectoryInfo newDirectoryInfo; if (directoryCounter > 0) { - // The server-supplied name is combined into a local path; ensure it - // cannot escape the destination directory. - EnsureValidLocalName(filename); + var path = ResolveChild(currentDirectoryFullName, filename); + + if (path is null) + { + SendInvalidLocalNameErrorAndThrow(channel); + } - newDirectoryInfo = Directory.CreateDirectory(Path.Combine(currentDirectoryFullName, filename)); + SendSuccessConfirmation(channel); // Send reply + + var newDirectoryInfo = Directory.CreateDirectory(path); newDirectoryInfo.LastAccessTimeUtc = accessedUtc; newDirectoryInfo.LastWriteTimeUtc = modifiedUtc; + + currentDirectoryFullName = path; } else { + SendSuccessConfirmation(channel); // Send reply + // Don't create directory for first level - newDirectoryInfo = directoryInfo; + currentDirectoryFullName = destination; } directoryCounter++; - - currentDirectoryFullName = newDirectoryInfo.FullName; continue; } @@ -1109,14 +1167,17 @@ private void DownloadDirectoryContent(IChannelSession channel, Stream input, Dir { var fileName = match.Result("${filename}"); - // Read file - SendSuccessConfirmation(channel); // Send reply + var path = ResolveChild(currentDirectoryFullName, fileName); - // The server-supplied name is combined into a local path; ensure it - // cannot escape the destination directory. - EnsureValidLocalName(fileName); + if (path is null) + { + SendInvalidLocalNameErrorAndThrow(channel); + } - var fileInfo = new FileInfo(Path.Combine(currentDirectoryFullName, fileName)); + var fileInfo = new FileInfo(path); + + // Read file + SendSuccessConfirmation(channel); // Send reply DownloadFileContent(channel, input, fileInfo, fileName, length, modifiedUtc, accessedUtc); @@ -1142,5 +1203,31 @@ private void DownloadDirectoryContent(IChannelSession channel, Stream input, Dir SendInvalidRecordErrorAndThrow(channel); } } + + // https://github.com/dotnet/runtime/blob/a1115d3990ce45403dd80d96e39c43066e9ece65/src/libraries/Common/src/System/IO/PathInternal.CaseSensitivity.cs + private static class PathHelpers + { + internal static StringComparison StringComparison + { + get + { + return IsCaseSensitive ? + StringComparison.Ordinal : + StringComparison.OrdinalIgnoreCase; + } + } + + internal static bool IsCaseSensitive + { + get + { +#if NET + return !(OperatingSystem.IsWindows() || OperatingSystem.IsMacOS() || OperatingSystem.IsIOS() || OperatingSystem.IsTvOS()); +#else + return Path.DirectorySeparatorChar != '\\'; +#endif + } + } + } } } diff --git a/test/Renci.SshNet.IntegrationTests/ScpTests.cs b/test/Renci.SshNet.IntegrationTests/ScpTests.cs index 5e6704d58..e98dc0131 100644 --- a/test/Renci.SshNet.IntegrationTests/ScpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ScpTests.cs @@ -2070,6 +2070,61 @@ public void Scp_Download_DirectoryInfo_ServerNameWithSeparator_StaysInsideDestin } } + /// + /// A recursive download must never write outside of the destination directory when the + /// remote tree contains a directory named "...". It is an ordinary name on the (Linux) + /// server, but Windows path normalization resolves it to the current directory. + /// + [TestMethod] + public void Scp_Download_DirectoryInfo_DirectoryNamedThreeDots_StaysInsideDestination() + { + var remoteDirectory = "/tmp/sshnet-scp-dots-" + Guid.NewGuid().ToString("N"); + + using (var client = new SftpClient(_connectionInfoFactory.Create())) + { + client.Connect(); + client.CreateDirectory(remoteDirectory); + client.CreateDirectory(remoteDirectory + "/..."); + client.WriteAllText(remoteDirectory + "/.../inner.txt", "inner"); + client.WriteAllText(remoteDirectory + "/outer.txt", "outer"); + } + + var localRoot = Path.Combine(Path.GetTempPath(), "sshnet-scp-dots-" + Guid.NewGuid().ToString("N")); + var destination = Directory.CreateDirectory(Path.Combine(localRoot, "download")); + + try + { + using (var client = new ScpClient(_connectionInfoFactory.Create(), _remotePathTransformation)) + { + client.Connect(); + + if (Path.DirectorySeparatorChar == '\\') + { + _ = Assert.ThrowsExactly(() => client.Download(remoteDirectory, destination)); + } + else + { + client.Download(remoteDirectory, destination); + Assert.AreEqual("inner", File.ReadAllText(Path.Combine(destination.FullName, "...", "inner.txt"))); + Assert.AreEqual("outer", File.ReadAllText(Path.Combine(destination.FullName, "outer.txt"))); + } + } + + // Nothing should be written outside of the destination directory on any platform. + CollectionAssert.AreEqual(new[] { destination.FullName }, Directory.GetFileSystemEntries(localRoot)); + } + finally + { + using (var client = new SshClient(_connectionInfoFactory.Create())) + { + client.Connect(); + _ = client.RunCommand("rm -rf '" + remoteDirectory + "'"); + } + + Directory.Delete(localRoot, recursive: true); + } + } + /// /// A file whose name contains the text of a D record must be downloaded as a file, with its /// content written verbatim rather than interpreted as SCP records. diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs index dd348445e..9573bd68f 100644 --- a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs @@ -85,6 +85,26 @@ public void DownloadDirectoryInfo_InvalidRecord_ShouldThrowScpException(string l Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); } + [TestMethod] + public void DownloadDirectoryInfo_EndDirectoryBeforeAnyDirectory_ShouldThrowScpException() + { + // Without the check, the E record would move the download to the parent of the destination, + // and the following file would overwrite the existing file there. + var existing = Path.Combine(DestinationParent, "existing.txt"); + File.WriteAllText(existing, "original"); + + var transcript = new ScpServerTranscript() + .Line("E") + .File("existing.txt", "data"); + + var result = DownloadDirectory(transcript); + + AssertRejected(result, expectedSuccessStatuses: 1); + Assert.AreEqual("original", File.ReadAllText(existing)); + AssertNothingOutsideDestination(expectedOutside: "a/b/existing.txt"); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + [TestMethod] [DataRow("Xjunk")] [DataRow("C0644 99999999999999999999 f")] diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_Names.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_Names.cs new file mode 100644 index 000000000..acbaeebb8 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_Names.cs @@ -0,0 +1,262 @@ +using System; +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Verifies how a recursive download handles the file and directory names sent by the server: + /// plain names are created as-is, and names that are not a plain local name, or that the local + /// file system would resolve to a different name or location, are rejected. + /// + /// + /// Each name is checked both in a C (file) record and in a nested D (directory) record; the name + /// in the first D record is ignored, as the destination itself is used for it. + /// + [TestClass] + public class ScpClientTest_Download_PathAndDirectoryInfo_Names : ScpClientTranscriptTestBase + { + [TestMethod] + [DataRow("file.txt")] + [DataRow(".hidden")] + [DataRow("файл.txt")] + [DataRow("~")] + public void PlainName_ShouldBeDownloaded(string name) + { + var result = DownloadDirectory(new ScpServerTranscript() + .Line("D0755 0 remote") + .Line("D0755 0 " + name) + .File(name, "content") + .Line("E") + .Line("E")); + + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { name, name + "/" + name }, ListEntries(Destination)); + } + + [TestMethod] + [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "These are ordinary names on Unix-like platforms.")] + [DataRow("...")] + [DataRow(".. ")] + [DataRow("file.")] + [DataRow("file ")] + [DataRow("CON")] + [DataRow("PROGRA~1")] + [DataRow("name\\with\\backslashes")] + [DataRow("file.txt:stream")] + [DataRow("C:file.txt")] + [DataRow("\\\\?\\C:\\file.txt")] + [DataRow("\\\\.\\C:\\file.txt")] + [DataRow("\\??\\C:\\file.txt")] + public void OrdinaryUnixName_ShouldBeDownloaded(string name) + { + var result = DownloadDirectory(new ScpServerTranscript() + .Line("D0755 0 remote") + .Line("D0755 0 " + name) + .File(name, "content") + .Line("E") + .Line("E")); + + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { name, name + "/" + name }, ListEntries(Destination)); + } + + [TestMethod] + [OSCondition(OperatingSystems.Windows, IgnoreMessage = "The \\\\?\\ prefix only exists on Windows.")] + public void PlainName_LiteralPathDestination_OnWindows_ShouldBeDownloaded() + { + var result = Download("sub", isDirectory: true, LiteralPath(Destination)); + + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { "sub", "sub/inner.txt" }, ListEntries(Destination)); + } + + [TestMethod] + [DataRow(".")] + [DataRow("..")] + [DataRow("sub/child")] + [DataRow("../escaped")] + [DataRow("/etc/passwd")] + [DataRow("nul\0byte")] + public void InvalidName_ShouldThrowScpException(string name) + { + foreach (var isDirectory in new[] { false, true }) + { + var result = Download(name, isDirectory); + + AssertNothingOutsideDestination(context: name); + Assert.IsInstanceOfType(result.Exception, result.Exception?.ToString()); + Assert.Contains("not a valid local name", result.Exception.Message, StringComparison.Ordinal); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + + // The record with the invalid name is reported to the server instead of being acknowledged: + // the only success statuses are for the exec request and the first D record. + Assert.AreEqual(2, result.SentData.Count(data => data.Length == 1 && data[0] == 0)); + Assert.IsTrue(result.SentData.Any(data => data.Length == 1 && data[0] == 1), "No error status was sent to the server."); + } + } + + [TestMethod] + [OSCondition(OperatingSystems.Windows, IgnoreMessage = "These names are only invalid, or changed by path normalization, on Windows.")] + [DataRow("..\\escaped")] + [DataRow("C:\\Windows\\file.txt")] + [DataRow("C:file.txt")] + [DataRow("file.txt:stream")] + [DataRow("\\\\?\\C:\\file.txt")] + [DataRow("\\\\.\\C:\\file.txt")] + [DataRow("\\??\\C:\\file.txt")] + [DataRow("\\\\server\\share\\file.txt")] + [DataRow("...")] // Path normalization removes trailing dots and spaces. + [DataRow(".. ")] + [DataRow(" ..")] + [DataRow(". ")] + [DataRow("file.")] + [DataRow("file ")] + public void InvalidName_OnWindows_ShouldThrowScpException(string name) + { + foreach (var isDirectory in new[] { false, true }) + { + var result = Download(name, isDirectory); + + AssertNothingOutsideDestination(context: name); + Assert.IsInstanceOfType(result.Exception, result.Exception?.ToString()); + Assert.Contains("not a valid local name", result.Exception.Message, StringComparison.Ordinal); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + Assert.AreEqual(2, result.SentData.Count(data => data.Length == 1 && data[0] == 0)); + Assert.IsTrue(result.SentData.Any(data => data.Length == 1 && data[0] == 1), "No error status was sent to the server."); + } + } + + [TestMethod] + [OSCondition(OperatingSystems.Windows, IgnoreMessage = "The \\\\?\\ prefix only exists on Windows.")] + [DataRow(".")] + [DataRow("..")] + [DataRow("...")] + [DataRow("file.")] + [DataRow("file ")] + public void NameChangedByNormalization_LiteralPathDestination_OnWindows_ShouldStayWithinDestination(string name) + { + // Path.GetFullPath does not normalize a path with the \\?\ prefix, so these names reach the + // file system as they are: "." and ".." are refused with an IOException, and the others are + // created with exactly that name. Either way, nothing may be written outside the destination. + foreach (var isDirectory in new[] { false, true }) + { + var result = Download(name, isDirectory, LiteralPath(Destination)); + + AssertNothingOutsideDestination(context: name); + Assert.IsTrue(result.Exception is null or ScpException or IOException, result.Exception?.ToString()); + + ClearDestination(); + } + } + + [TestMethod] + [OSCondition(OperatingSystems.Windows, IgnoreMessage = "Device names are only reserved on Windows.")] + [DataRow("CON")] + [DataRow("con.txt")] + [DataRow("CON .txt")] + [DataRow("NUL")] + [DataRow("COM1")] + [DataRow("LPT1")] + public void ReservedDeviceName_OnWindows_ShouldBeRejectedOrWrittenToDestination(string name) + { + // Where Windows treats the name as a device in any directory, Path.GetFullPath resolves it to + // a device path (such as \\.\CON), which is not a child of the destination and is rejected. + // Where it does not (as in recent versions of Windows), it is an ordinary name. + foreach (var isDirectory in new[] { false, true }) + { + var result = Download(name, isDirectory); + + AssertNothingOutsideDestination(context: name); + + if (result.Exception is null) + { + // Read through a literal path, so that the name cannot be redirected to a device. + var file = isDirectory ? Path.Combine(Destination, name, "inner.txt") : Path.Combine(Destination, name); + Assert.AreEqual("content", File.ReadAllText(LiteralPath(file))); + } + else + { + Assert.IsInstanceOfType(result.Exception, result.Exception.ToString()); + Assert.Contains("not a valid local name", result.Exception.Message, StringComparison.Ordinal); + } + + ClearDestination(); + } + } + + [TestMethod] + [OSCondition(OperatingSystems.Windows, IgnoreMessage = "8.3 short names only exist on Windows.")] + public void ShortNameOfExistingDirectory_OnWindows_ShouldStayWithinDestination() + { + var existing = Directory.CreateDirectory(Path.Combine(Destination, "LongDirectoryName")); + + var buffer = new char[1024]; + var length = GetShortPathName(existing.FullName, buffer, (uint)buffer.Length); + var shortName = length is 0 or > 1024 ? existing.Name : Path.GetFileName(new string(buffer, 0, (int)length)); + + if (string.Equals(shortName, existing.Name, StringComparison.OrdinalIgnoreCase)) + { + Assert.Inconclusive("8.3 short names are not generated on this volume."); + } + + var result = Download(shortName, isDirectory: true); + + // .NET Framework's Path.GetFullPath expands a short name, so the resolved name differs from + // the name that was sent and is rejected. Elsewhere, the short name is used as-is, and the + // file system resolves it to the existing directory. Either way, the download must stay + // within the destination. + if (result.Exception is null) + { + Assert.AreEqual("content", File.ReadAllText(Path.Combine(existing.FullName, "inner.txt"))); + } + else + { + Assert.IsInstanceOfType(result.Exception); + Assert.Contains("not a valid local name", result.Exception.Message, StringComparison.Ordinal); + } + + AssertNothingOutsideDestination(); + } + + [TestMethod] + public void DestinationWithTrailingSeparator_ShouldBeDownloaded() + { + var result = Download("sub", isDirectory: true, Destination + Path.DirectorySeparatorChar); + + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { "sub", "sub/inner.txt" }, ListEntries(Destination)); + } + +#pragma warning disable SYSLIB1054 // Use LibraryImport; not available on .NET Framework, and only used by a Windows-only test. + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true, EntryPoint = "GetShortPathNameW")] + private static extern uint GetShortPathName(string longPath, [Out] char[] shortPath, uint bufferLength); +#pragma warning restore SYSLIB1054 + + /// + /// Downloads a remote directory containing either a file named , or a + /// directory named that contains a file named inner.txt. + /// + private ScpTranscriptResult Download(string name, bool isDirectory, string destinationPath = null) + { + var transcript = new ScpServerTranscript().Line("D0755 0 remote"); + + if (isDirectory) + { + _ = transcript.Line("D0755 0 " + name).File("inner.txt", "content").Line("E"); + } + else + { + _ = transcript.File(name, "content"); + } + + return DownloadDirectory(transcript.Line("E"), destinationPath); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs deleted file mode 100644 index 866ce823a..000000000 --- a/test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs +++ /dev/null @@ -1,102 +0,0 @@ -using Microsoft.VisualStudio.TestTools.UnitTesting; - -using Renci.SshNet.Common; - -namespace Renci.SshNet.Tests.Classes -{ - /// - /// Tests for , which guards the recursive - /// download against server-supplied SCP file/directory names that would write outside the - /// caller-supplied destination directory. - /// - [TestClass] - public class ScpClientTest_EnsureValidLocalName - { - [TestMethod] - public void PlainFileName_DoesNotThrow() - { - ScpClient.EnsureValidLocalName("owned.txt"); - ScpClient.EnsureValidLocalName("2024-report.tar.gz"); - ScpClient.EnsureValidLocalName("file with spaces.dat"); - ScpClient.EnsureValidLocalName("..."); - } - - [TestMethod] - public void UnicodeFileName_DoesNotThrow() - { - ScpClient.EnsureValidLocalName("файл.txt"); - } - - [TestMethod] - public void Empty_ThrowsScpException() - { - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName(string.Empty)); - } - - [TestMethod] - public void CurrentDirectory_ThrowsScpException() - { - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName(".")); - } - - [TestMethod] - public void ParentDirectory_ThrowsScpException() - { - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("..")); - } - - [TestMethod] - public void ForwardSlashPath_ThrowsScpException() - { - // '/' is an invalid file name character on every platform. - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("sub/child")); - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("../escaped/owned.txt")); - } - - [TestMethod] - public void RootedUnixPath_ThrowsScpException() - { - // Contains '/', so it is rejected regardless of platform. - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("/tmp/sshnet-owned.txt")); - } - - [TestMethod] - public void NullCharacter_ThrowsScpException() - { - // NUL is an invalid file name character on every platform. - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("safe\0evil")); - } - - [TestMethod] - [OSCondition(OperatingSystems.Windows, IgnoreMessage = "'\\' is only a path separator (and invalid file name char) on Windows.")] - public void BackslashPath_OnWindows_ThrowsScpException() - { - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("..\\escaped\\owned.txt")); - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("sub\\child")); - } - - [TestMethod] - [OSCondition(OperatingSystems.Windows, IgnoreMessage = "':' is only an invalid file name char on Windows.")] - public void DriveQualifiedPath_OnWindows_ThrowsScpException() - { - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("C:\\Windows\\System32\\evil.dll")); - } - - [TestMethod] - [OSCondition(OperatingSystems.Windows, IgnoreMessage = "':' is only an invalid file name char on Windows.")] - public void AlternateDataStreamName_OnWindows_ThrowsScpException() - { - // NTFS alternate data stream syntax: writes a hidden stream of "safe.txt". - _ = Assert.ThrowsExactly(() => ScpClient.EnsureValidLocalName("safe.txt:evil")); - } - - [TestMethod] - [OSCondition(ConditionMode.Exclude, OperatingSystems.Windows, IgnoreMessage = "'\\' is a valid file name byte only on Unix-like platforms.")] - public void BackslashName_OnUnix_DoesNotThrow() - { - // On Unix, '\' is an ordinary file name byte and cannot traverse directories, - // so a name containing it must remain accepted (no behaviour change). - ScpClient.EnsureValidLocalName("name\\with\\backslashes.txt"); - } - } -} diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_ResolveChild_RootDirectory.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_ResolveChild_RootDirectory.cs new file mode 100644 index 000000000..842f4ccee --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_ResolveChild_RootDirectory.cs @@ -0,0 +1,35 @@ +using System.IO; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Tests for and + /// with a root directory as the destination, + /// which a download test cannot use. Other cases are covered by + /// . + /// + [TestClass] + public class ScpClientTest_ResolveChild_RootDirectory + { + [TestMethod] + public void RootDirectory_ShouldReturnDirectChild() + { + var root = ScpClient.NormalizeDirectory(Path.GetPathRoot(Path.GetTempPath())); + + Assert.AreEqual(Path.Combine(root, "file.txt"), ScpClient.ResolveChild(root, "file.txt")); + } + + [TestMethod] + [DataRow(".")] + [DataRow("..")] + public void RootDirectory_DotOrDotDot_ShouldBeRejected(string name) + { + // "." and ".." resolve to the root itself, which has no parent directory. + var root = ScpClient.NormalizeDirectory(Path.GetPathRoot(Path.GetTempPath())); + + Assert.IsNull(ScpClient.ResolveChild(root, name)); + } + } +}