From 600be0de543765995a189b5d7cd4efac5007f3ce Mon Sep 17 00:00:00 2001 From: Nadav0077 <18245584+Nadav0077@users.noreply.github.com> Date: Sun, 9 Aug 2026 18:32:00 +0200 Subject: [PATCH] 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 | 37 ++++++ .../Renci.SshNet.IntegrationTests/ScpTests.cs | 65 +++++++++ ...hAndDirectoryInfo_ServerSendsUnsafeName.cs | 123 ++++++++++++++++++ .../ScpClientTest_EnsureValidLocalName.cs | 102 +++++++++++++++ 4 files changed, 327 insertions(+) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_ServerSendsUnsafeName.cs create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index ac2d3496..275e000a 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/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); } + /// + /// 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. + /// + /// 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) + { + // 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)); } diff --git a/test/Renci.SshNet.IntegrationTests/ScpTests.cs b/test/Renci.SshNet.IntegrationTests/ScpTests.cs index 8724ae3d..e19f603d 100644 --- a/test/Renci.SshNet.IntegrationTests/ScpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ScpTests.cs @@ -2005,6 +2005,71 @@ namespace Renci.SshNet.IntegrationTests yield return new object[] { RemotePathTransformation.None, "scp-directorydoesnotexist" }; } + /// + /// 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. + /// + [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); diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_ServerSendsUnsafeName.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_PathAndDirectoryInfo_ServerSendsUnsafeName.cs new file mode 100644 index 00000000..98b63d7f --- /dev/null +++ b/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 +{ + /// + /// Verifies that a recursive download aborts with a when the + /// server sends a file/directory name in the SCP stream that would write outside of the + /// caller-supplied destination directory. + /// + [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())); + _ = _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(_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)); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_EnsureValidLocalName.cs new file mode 100644 index 00000000..866ce823 --- /dev/null +++ b/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 +{ + /// + /// 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"); + } + } +}