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");
+ }
+ }
+}