From 75ab0f507ae1b200508234fb8908aeb0375d6ad0 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 08:49:53 +0000 Subject: [PATCH] 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 900a37e3..d62c5684 100644 --- a/src/Renci.SshNet/Abstractions/StringExtensions.cs +++ b/src/Renci.SshNet/Abstractions/StringExtensions.cs @@ -29,6 +29,11 @@ namespace System { 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 3a487b1e..122ddd5c 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 @@ namespace Renci.SshNet.Common /// /// 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 @@ namespace Renci.SshNet.Common /// /// 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 @@ namespace Renci.SshNet.Common { ArgumentNullException.ThrowIfNull(path); - var posixPath = new PosixPath(); + string directory; + string? file = null; var pathEnd = path.LastIndexOf('/'); if (pathEnd == -1) @@ -50,27 +54,28 @@ namespace Renci.SshNet.Common 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 @@ namespace Renci.SshNet.Common /// 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 a630bb18..dda84591 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -351,7 +351,10 @@ namespace Renci.SshNet /// 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 @@ namespace Renci.SshNet 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 @@ namespace Renci.SshNet // 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 @@ namespace Renci.SshNet /// 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 @@ namespace Renci.SshNet 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 @@ namespace Renci.SshNet // 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 @@ namespace Renci.SshNet 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 @@ namespace Renci.SshNet 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 @@ namespace Renci.SshNet 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 @@ namespace Renci.SshNet // 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 @@ namespace Renci.SshNet 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 e19f603d..971a624a 100644 --- a/test/Renci.SshNet.IntegrationTests/ScpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ScpTests.cs @@ -2070,6 +2070,129 @@ namespace Renci.SshNet.IntegrationTests } } + /// + /// 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 00000000..7d0306b7 --- /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); + } + } +}