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