Reject unsafe server-supplied names in SCP recursive download

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>

src/Renci.SshNet/ScpClient.cs

@@ -89,6 +89,8 @@ namespace Renci.SshNet
         private static readonly byte[] SuccessConfirmationCode = { 0 };
         private static readonly byte[] ErrorConfirmationCode = { 1 };
 
+        private static readonly char[] InvalidLocalNameChars = Path.GetInvalidFileNameChars();
+
         private IRemotePathTransformation _remotePathTransformation;
         private TimeSpan _operationTimeout;
 
@@ -824,6 +826,33 @@ namespace Renci.SshNet
             CheckReturnCode(input);
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        /// <param name="name">The file or directory name as sent by the server.</param>
+        /// <exception cref="ScpException">
+        /// <paramref name="name"/> 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).
+        /// </exception>
+        internal static void EnsureValidLocalName(string name)
+        {
+            // 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)
+            {
+                throw new ScpException($"The server sent a file or directory name (\"{name}\") that is not a valid local name.");
+            }
+        }
+
         private void InternalDownload(IChannel channel, Stream input, Stream output, string filename, long length)
         {
             var buffer = new byte[Math.Min(length, BufferSize)];
@@ -896,6 +925,10 @@ namespace Renci.SshNet
                     DirectoryInfo newDirectoryInfo;
                     if (directoryCounter > 0)
                     {
+                        // The server-supplied name is combined into a local path; ensure it
+                        // cannot escape the destination directory.
+                        EnsureValidLocalName(filename);
+
                         newDirectoryInfo = Directory.CreateDirectory(Path.Combine(currentDirectoryFullName, filename));
                         newDirectoryInfo.LastAccessTime = accessedTime;
                         newDirectoryInfo.LastWriteTime = modifiedTime;
@@ -923,6 +956,10 @@ namespace Renci.SshNet
 
                     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);
+
                         fileInfo = new FileInfo(Path.Combine(currentDirectoryFullName, fileName));
                     }
 

test/Renci.SshNet.IntegrationTests/ScpTests.cs

@@ -2005,6 +2005,71 @@ namespace Renci.SshNet.IntegrationTests
             yield return new object[] { RemotePathTransformation.None, "scp-directorydoesnotexist" };
         }
 
+        /// <summary>
+        /// A recursive download must never write outside of the destination directory, even
+        /// when the server returns a file name containing a path separator. Backslash is a
+        /// legal byte in a Unix file name, so a stock OpenSSH server transmits it verbatim;
+        /// on a Windows client it would otherwise be interpreted as a directory separator.
+        /// </summary>
+        [TestMethod]
+        public void Scp_Download_DirectoryInfo_ServerNameWithSeparator_StaysInsideDestination()
+        {
+            var remoteDirectory = "/tmp/sshnet-scp-guard-" + Guid.NewGuid().ToString("N");
+
+            // Set up a remote directory containing a normal file and a file whose name
+            // contains a backslash (a single, valid Unix file name).
+            using (var client = new SshClient(_connectionInfoFactory.Create()))
+            {
+                client.Connect();
+                _ = client.RunCommand("mkdir -p '" + remoteDirectory + "'");
+                _ = client.RunCommand("printf '%s' good > '" + remoteDirectory + "/good.txt'");
+                _ = client.RunCommand("printf '%s' ESCAPED > '" + remoteDirectory + "/..\\owned.txt'");
+            }
+
+            var localRoot = Path.GetTempFileName();
+            File.Delete(localRoot);
+            _ = Directory.CreateDirectory(localRoot);
+
+            var destination = Path.Combine(localRoot, "download");
+            _ = Directory.CreateDirectory(destination);
+
+            // Where "..\owned.txt", combined with the destination, would land on a Windows client.
+            var escapedFile = Path.Combine(localRoot, "owned.txt");
+
+            try
+            {
+                using (var client = new ScpClient(_connectionInfoFactory.Create()))
+                {
+                    client.Connect();
+
+                    try
+                    {
+                        client.Download(remoteDirectory, new DirectoryInfo(destination));
+                    }
+                    catch (ScpException)
+                    {
+                        // Expected on platforms where '\' is a directory separator: the
+                        // download is aborted rather than allowed to escape.
+                    }
+                }
+
+                // The security invariant, asserted on every platform: nothing is written
+                // outside of the caller-supplied destination directory.
+                Assert.IsFalse(File.Exists(escapedFile),
+                    "A file was written outside of the destination directory: " + escapedFile);
+            }
+            finally
+            {
+                using (var client = new SshClient(_connectionInfoFactory.Create()))
+                {
+                    client.Connect();
+                    _ = client.RunCommand("rm -rf '" + remoteDirectory + "'");
+                }
+
+                Directory.Delete(localRoot, recursive: true);
+            }
+        }
+
         private static void CreateRemoteFile(ScpClient client, string remoteFile, int size)
         {
             var file = CreateTempFile(size);

test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_ServerSendsUnsafeName.cs

@@ -0,0 +1,123 @@
+using System;
+using System.IO;
+using System.Text;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Moq;
+
+using Renci.SshNet.Common;
+
+namespace Renci.SshNet.Tests.Classes
+{
+    /// <summary>
+    /// Verifies that a recursive download aborts with a <see cref="ScpException"/> when the
+    /// server sends a file/directory name in the SCP stream that would write outside of the
+    /// caller-supplied destination directory.
+    /// </summary>
+    [TestClass]
+    public class ScpClientTest_Download_PathAndDirectoryInfo_ServerSendsUnsafeName : ScpClientTestBase
+    {
+        private ConnectionInfo _connectionInfo;
+        private ScpClient _scpClient;
+        private DirectoryInfo _destination;
+        private string _destinationRoot;
+        private string _path;
+        private string _transformedPath;
+        private PipeStream _pipeStream;
+        private Exception _actualException;
+
+        protected override void SetupData()
+        {
+            _connectionInfo = new ConnectionInfo("host", 22, "user", new PasswordAuthenticationMethod("user", "pwd"));
+
+            // A real, isolated destination directory so that, were the guard absent, an escape
+            // would be observable rather than silently corrupting the test working directory.
+            _destinationRoot = Path.Combine(Path.GetTempPath(), "sshnet-scp-guard-" + Guid.NewGuid().ToString("N"));
+            _destination = Directory.CreateDirectory(_destinationRoot);
+
+            _path = "/home/sshnet/remote";
+            _transformedPath = "transformed";
+
+            // The very first SCP record is a C (file) record whose server-controlled name is the
+            // parent-directory reference "..". Without the guard this is combined into a local
+            // path and opened for writing; with the guard it must be rejected up-front.
+            _pipeStream = new PipeStream();
+            var record = Encoding.ASCII.GetBytes("C0644 0 ..\n");
+            _pipeStream.Write(record, 0, record.Length);
+        }
+
+        protected override void SetupMocks()
+        {
+            _ = ServiceFactoryMock.Setup(p => p.CreateRemotePathDoubleQuoteTransformation())
+                                  .Returns(_remotePathTransformationMock.Object);
+            _ = 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(_channelSessionMock.Object);
+            _ = _channelSessionMock.Setup(p => p.Open());
+            _ = _remotePathTransformationMock.Setup(p => p.Transform(_path))
+                                             .Returns(_transformedPath);
+            _ = _channelSessionMock.Setup(p => p.SendExecRequest("scp -prf " + _transformedPath))
+                                   .Returns(true);
+            _ = _channelSessionMock.Setup(p => p.SendData(It.IsAny<byte[]>()));
+            _ = _channelSessionMock.Setup(p => p.Dispose());
+        }
+
+        protected override void Arrange()
+        {
+            base.Arrange();
+
+            _scpClient = new ScpClient(_connectionInfo, false, ServiceFactoryMock.Object);
+            _scpClient.Connect();
+        }
+
+        protected override void Act()
+        {
+            // Capture any exception type: the guard must turn an unsafe name into a clean
+            // ScpException. Without the guard the download instead proceeds to a local file
+            // operation, which is exactly what this test asserts must not happen.
+            try
+            {
+                _scpClient.Download(_path, _destination);
+            }
+            catch (Exception ex)
+            {
+                _actualException = ex;
+            }
+        }
+
+        protected override void TearDown()
+        {
+            base.TearDown();
+
+            _pipeStream?.Dispose();
+
+            if (_destinationRoot != null && Directory.Exists(_destinationRoot))
+            {
+                Directory.Delete(_destinationRoot, recursive: true);
+            }
+        }
+
+        [TestMethod]
+        public void DownloadShouldHaveThrownScpException()
+        {
+            Assert.IsNotNull(_actualException, "Download did not abort on the unsafe server name.");
+            Assert.IsInstanceOfType<ScpException>(_actualException);
+            Assert.Contains("not a valid local name", _actualException.Message, StringComparison.Ordinal);
+        }
+
+        [TestMethod]
+        public void NothingShouldHaveBeenWritten()
+        {
+            // The guard rejects the name before any local file/directory is created, so the
+            // destination directory must remain empty.
+            Assert.IsEmpty(Directory.GetFileSystemEntries(_destinationRoot));
+        }
+    }
+}

test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs

@@ -0,0 +1,102 @@
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Renci.SshNet.Common;
+
+namespace Renci.SshNet.Tests.Classes
+{
+    /// <summary>
+    /// Tests for <see cref="ScpClient.EnsureValidLocalName(string)"/>, which guards the recursive
+    /// download against server-supplied SCP file/directory names that would write outside the
+    /// caller-supplied destination directory.
+    /// </summary>
+    [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<ScpException>(() => ScpClient.EnsureValidLocalName(string.Empty));
+        }
+
+        [TestMethod]
+        public void CurrentDirectory_ThrowsScpException()
+        {
+            _ = Assert.ThrowsExactly<ScpException>(() => ScpClient.EnsureValidLocalName("."));
+        }
+
+        [TestMethod]
+        public void ParentDirectory_ThrowsScpException()
+        {
+            _ = Assert.ThrowsExactly<ScpException>(() => ScpClient.EnsureValidLocalName(".."));
+        }
+
+        [TestMethod]
+        public void ForwardSlashPath_ThrowsScpException()
+        {
+            // '/' is an invalid file name character on every platform.
+            _ = Assert.ThrowsExactly<ScpException>(() => ScpClient.EnsureValidLocalName("sub/child"));
+            _ = Assert.ThrowsExactly<ScpException>(() => ScpClient.EnsureValidLocalName("../escaped/owned.txt"));
+        }
+
+        [TestMethod]
+        public void RootedUnixPath_ThrowsScpException()
+        {
+            // Contains '/', so it is rejected regardless of platform.
+            _ = Assert.ThrowsExactly<ScpException>(() => ScpClient.EnsureValidLocalName("/tmp/sshnet-owned.txt"));
+        }
+
+        [TestMethod]
+        public void NullCharacter_ThrowsScpException()
+        {
+            // NUL is an invalid file name character on every platform.
+            _ = Assert.ThrowsExactly<ScpException>(() => 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<ScpException>(() => ScpClient.EnsureValidLocalName("..\\escaped\\owned.txt"));
+            _ = Assert.ThrowsExactly<ScpException>(() => 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<ScpException>(() => 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<ScpException>(() => 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");
+        }
+    }
+}