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.

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
         }
     }

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
     /// </summary>
     internal sealed class PosixPath
     {
-        private PosixPath()
+        private PosixPath(string directory, string? file)
         {
+            Directory = directory;
+            File = file;
         }
 
         /// <summary>
@@ -17,7 +20,7 @@ namespace Renci.SshNet.Common
         /// <value>
         /// The directory of the path.
         /// </value>
-        public string Directory { get; private set; }
+        public string Directory { get; }
 
         /// <summary>
         /// Gets the file part of the path.
@@ -25,7 +28,7 @@ namespace Renci.SshNet.Common
         /// <value>
         /// The file part of the path, or <see langword="null"/> if the path represents a directory.
         /// </value>
-        public string File { get; private set; }
+        public string? File { get; }
 
         /// <summary>
         /// Create a <see cref="PosixPath"/> 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);
         }
 
         /// <summary>
@@ -87,7 +92,7 @@ namespace Renci.SshNet.Common
         /// is returned.
         /// </para>
         /// <para>
-        /// If path has a trailing slash, <see cref="GetFileName(string)"/> return a zero-length string.
+        /// If path has a trailing slash, <see cref="GetFileName(string)"/> returns a zero-length string.
         /// </para>
         /// </remarks>
         public static string GetFileName(string path)

src/Renci.SshNet/ScpClient.cs

@@ -351,7 +351,10 @@ namespace Renci.SshNet
         /// <param name="source">The <see cref="Stream"/> to upload.</param>
         /// <param name="path">A relative or absolute path for the remote file.</param>
         /// <exception cref="ArgumentNullException"><paramref name="path" /> is <see langword="null"/>.</exception>
-        /// <exception cref="ArgumentException"><paramref name="path"/> is a zero-length <see cref="string"/>.</exception>
+        /// <exception cref="ArgumentException">
+        /// <paramref name="path"/> is a zero-length <see cref="string"/>, does not have a file name part (for example,
+        /// it ends with <c>/</c>), or its file name part contains a line feed character.
+        /// </exception>
         /// <exception cref="ScpException">A directory with the specified path exists on the remote host.</exception>
         /// <exception cref="SshException">The secure copy execution request was rejected by the server.</exception>
         /// <exception cref="SshConnectionException">Client is not connected.</exception>
@@ -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
         /// <param name="path">A relative or absolute path for the remote file.</param>
         /// <exception cref="ArgumentNullException"><paramref name="fileInfo" /> is <see langword="null"/>.</exception>
         /// <exception cref="ArgumentNullException"><paramref name="path" /> is <see langword="null"/>.</exception>
-        /// <exception cref="ArgumentException"><paramref name="path"/> is a zero-length <see cref="string"/>.</exception>
+        /// <exception cref="ArgumentException">
+        /// <paramref name="path"/> is a zero-length <see cref="string"/>, does not have a file name part (for example,
+        /// it ends with <c>/</c>), or its file name part contains a line feed character.
+        /// </exception>
         /// <exception cref="ScpException">A directory with the specified path exists on the remote host.</exception>
         /// <exception cref="SshException">The secure copy execution request was rejected by the server.</exception>
         /// <exception cref="SshConnectionException">Client is not connected.</exception>
@@ -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.");
+                }
+            }
         }
 
         /// <summary>
@@ -842,6 +860,28 @@ namespace Renci.SshNet
             CheckReturnCode(input);
         }
 
+        /// <summary>
+        /// Splits the remote path passed to <see cref="Upload(Stream, string)"/> or
+        /// <see cref="Upload(FileInfo, string)"/> into its directory and file name parts, and ensures
+        /// that the file name part can be sent in an SCP C record.
+        /// </summary>
+        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);
+        }
+
         /// <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

test/Renci.SshNet.IntegrationTests/ScpTests.cs

@@ -2070,6 +2070,129 @@ namespace Renci.SshNet.IntegrationTests
             }
         }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        [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<ArgumentException>(
+                (client, remoteDirectory) =>
+                {
+                    using (var source = new MemoryStream(content))
+                    {
+                        client.Upload(source, remoteDirectory + "/" + fileName);
+                    }
+                });
+        }
+
+        /// <inheritdoc cref="Scp_Upload_Stream_FileNameWithLineFeed_DoesNotCreateFiles"/>
+        [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<ArgumentException>((client, remoteDirectory)
+                    => client.Upload(new FileInfo(localFile), remoteDirectory + "/report.txt\n" + new string('x', 22)));
+            }
+            finally
+            {
+                File.Delete(localFile);
+            }
+        }
+
+        /// <summary>
+        /// A local file whose name contains a line feed must not be sent in a C record when uploading a
+        /// directory; see <see cref="Scp_Upload_Stream_FileNameWithLineFeed_DoesNotCreateFiles"/>.
+        /// </summary>
+        [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<ScpException>((client, remoteDirectory)
+                    => client.Upload(localDirectory, remoteDirectory));
+            }
+            finally
+            {
+                localDirectory.Delete(recursive: true);
+            }
+        }
+
+        /// <summary>
+        /// Runs <paramref name="upload"/> into a new, empty remote directory, and asserts that it throws
+        /// <typeparamref name="TException"/> without creating anything in that directory.
+        /// </summary>
+        private void AssertNameWithLineFeedDoesNotCreateFiles<TException>(Action<ScpClient, string> 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<TException>(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);

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
+{
+    /// <summary>
+    /// 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 <c>/</c>).
+    /// </summary>
+    /// <remarks>
+    /// 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 (<c>ScpTests</c>) show the effect on a real server: another file is created.
+    /// </remarks>
+    [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<ArgumentException>(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<ScpException>(result.Exception);
+            Assert.Contains("line feed", result.Exception.Message, StringComparison.Ordinal);
+            Assert.IsEmpty(result.SentData);
+        }
+    }
+}