diff --git a/src/Renci.SshNet/ScpClient.cs b/src/Renci.SshNet/ScpClient.cs index dda84591..4afe4920 100644 --- a/src/Renci.SshNet/ScpClient.cs +++ b/src/Renci.SshNet/ScpClient.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; using System.Net; @@ -66,9 +67,9 @@ namespace Renci.SshNet "taking an IRemotePathTransformation which suits the escaping rules of the remote server and " + "the trust environment in which this code runs, and consider using SFTP where possible."; - private const string FileInfoPattern = @"C(?\d{4}) (?\d+) (?.+)"; - private const string DirectoryInfoPattern = @"D(?\d{4}) (?\d+) (?.+)"; - private const string TimestampPattern = @"T(?\d+) 0 (?\d+) 0"; + private const string FileInfoPattern = @"^C(?[0-7]{4}) (?[0-9]+) (?.+)$"; + private const string DirectoryInfoPattern = @"^D(?[0-7]{4}) (?[0-9]+) (?.+)$"; + private const string TimestampPattern = @"^T(?[0-9]+) 0 (?[0-9]+) 0$"; #if NET private static readonly Regex FileInfoRegex = GetFileInfoRegex(); @@ -489,7 +490,7 @@ namespace Renci.SshNet /// Local file information. /// is . /// is or empty. - /// exists on the remote host, and is not a regular file. + /// exists on the remote host, and is not a regular file, or the server sent an invalid or unexpected SCP record. /// The secure copy execution request was rejected by the server. /// Client is not connected. public void Download(string filename, FileInfo fileInfo) @@ -518,7 +519,41 @@ namespace Renci.SshNet // Send reply SendSuccessConfirmation(channel); - InternalDownload(channel, input, fileInfo); + var modifiedUtc = DateTime.UtcNow; + var accessedUtc = modifiedUtc; + + var message = ReadString(input); + + var match = TimestampRegex.Match(message); + if (match.Success) + { + if (!long.TryParse(match.Result("${mtime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var mtime) || + !long.TryParse(match.Result("${atime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var atime) || + !TryFromUnixTimeSeconds(mtime, out modifiedUtc) || + !TryFromUnixTimeSeconds(atime, out accessedUtc)) + { + SendInvalidRecordErrorAndThrow(channel); + } + + SendSuccessConfirmation(channel); + + message = ReadString(input); + } + + match = FileInfoRegex.Match(message); + if (match.Success && + long.TryParse(match.Result("${length}"), NumberStyles.None, CultureInfo.InvariantCulture, out var length)) + { + var fileName = match.Result("${filename}"); + + SendSuccessConfirmation(channel); + + DownloadFileContent(channel, input, fileInfo, fileName, length, modifiedUtc, accessedUtc); + } + else + { + SendInvalidRecordErrorAndThrow(channel); + } } } @@ -529,7 +564,7 @@ namespace Renci.SshNet /// Local directory information. /// is or empty. /// is . - /// File or directory with the specified path does not exist on the remote host. + /// File or directory with the specified path does not exist on the remote host, or the server sent an invalid or unexpected SCP record. /// The secure copy execution request was rejected by the server. /// Client is not connected. public void Download(string directoryName, DirectoryInfo directoryInfo) @@ -558,7 +593,7 @@ namespace Renci.SshNet // Send reply SendSuccessConfirmation(channel); - InternalDownload(channel, input, directoryInfo); + DownloadDirectoryContent(channel, input, directoryInfo); } } @@ -569,7 +604,7 @@ namespace Renci.SshNet /// The to download the remote file to. /// is or contains only whitespace characters. /// is . - /// exists on the remote host, and is not a regular file. + /// exists on the remote host, and is not a regular file, or the server sent an invalid or unexpected SCP record. /// The secure copy execution request was rejected by the server. /// Client is not connected. public void Download(string filename, Stream destination) @@ -600,19 +635,19 @@ namespace Renci.SshNet var message = ReadString(input); var match = FileInfoRegex.Match(message); - if (match.Success) + if (match.Success && + long.TryParse(match.Result("${length}"), NumberStyles.None, CultureInfo.InvariantCulture, out var length)) { + var fileName = match.Result("${filename}"); + // Read file SendSuccessConfirmation(channel); // Send reply - var length = long.Parse(match.Result("${length}"), CultureInfo.InvariantCulture); - var fileName = match.Result("${filename}"); - - InternalDownload(channel, input, destination, fileName, length); + DownloadFileContent(channel, input, destination, fileName, length); } else { - SendErrorConfirmation(channel, string.Format("\"{0}\" is not valid protocol message.", message)); + SendInvalidRecordErrorAndThrow(channel); } } } @@ -746,6 +781,34 @@ namespace Renci.SshNet channel.SendData(ConnectionInfo.Encoding.GetBytes(command)); } + [DoesNotReturn] + private void SendInvalidRecordErrorAndThrow(IChannel channel) + { + SendErrorConfirmation(channel, "Invalid or unexpected SCP record."); + + throw new ScpException("The server sent an invalid or unexpected SCP record."); + } + + /// + /// Converts a number of seconds since the Unix epoch, as sent in a T record, to a . + /// + /// The number of seconds since the Unix epoch. + /// The time, in UTC, if can be represented. + /// if can be represented as a ; otherwise, . + private static bool TryFromUnixTimeSeconds(long seconds, out DateTime dateTime) + { + try + { + dateTime = DateTimeOffset.FromUnixTimeSeconds(seconds).UtcDateTime; + return true; + } + catch (ArgumentOutOfRangeException) + { + dateTime = default; + return false; + } + } + /// /// Read a LF-terminated string from the . /// @@ -909,7 +972,11 @@ namespace Renci.SshNet } } - private void InternalDownload(IChannel channel, Stream input, Stream output, string filename, long length) + /// + /// Downloads the content announced by a C record into a stream, and exchanges the confirmations + /// that follow it. + /// + private void DownloadFileContent(IChannel channel, Stream input, Stream output, string filename, long length) { var buffer = new byte[Math.Min(length, BufferSize)]; var needToRead = length; @@ -942,12 +1009,42 @@ namespace Renci.SshNet CheckReturnCode(input); } - private void InternalDownload(IChannelSession channel, Stream input, FileSystemInfo fileSystemInfo) + /// + /// Downloads the content announced by a C record into a local file, and applies the times. + /// + private void DownloadFileContent( + IChannelSession channel, + Stream input, + FileInfo fileInfo, + string fileName, + long length, + DateTime modifiedUtc, + DateTime accessedUtc) { - var modifiedTime = DateTime.Now; - var accessedTime = DateTime.Now; + Debug.Assert(modifiedUtc.Kind == DateTimeKind.Utc); + Debug.Assert(accessedUtc.Kind == DateTimeKind.Utc); - var startDirectoryFullName = fileSystemInfo.FullName; + using (var output = fileInfo.Open(FileMode.Create, FileAccess.Write)) + { + DownloadFileContent(channel, input, output, fileName, length); + } + + fileInfo.LastAccessTimeUtc = accessedUtc; + fileInfo.LastWriteTimeUtc = modifiedUtc; + } + + /// + /// Downloads the records and content of a directory tree (scp -prf) into a local directory. + /// + /// The channel to perform the download in. + /// A from which the server's records and content are read. + /// The local directory to download into. + private void DownloadDirectoryContent(IChannelSession channel, Stream input, DirectoryInfo directoryInfo) + { + var modifiedUtc = DateTime.UtcNow; + var accessedUtc = modifiedUtc; + + var startDirectoryFullName = directoryInfo.FullName; var currentDirectoryFullName = startDirectoryFullName; var directoryCounter = 0; @@ -991,13 +1088,13 @@ namespace Renci.SshNet EnsureValidLocalName(filename); newDirectoryInfo = Directory.CreateDirectory(Path.Combine(currentDirectoryFullName, filename)); - newDirectoryInfo.LastAccessTime = accessedTime; - newDirectoryInfo.LastWriteTime = modifiedTime; + newDirectoryInfo.LastAccessTimeUtc = accessedUtc; + newDirectoryInfo.LastWriteTimeUtc = modifiedUtc; } else { // Don't create directory for first level - newDirectoryInfo = (DirectoryInfo)fileSystemInfo; + newDirectoryInfo = directoryInfo; } directoryCounter++; @@ -1007,30 +1104,21 @@ namespace Renci.SshNet } match = FileInfoRegex.Match(message); - if (match.Success) + if (match.Success && + long.TryParse(match.Result("${length}"), NumberStyles.None, CultureInfo.InvariantCulture, out var length)) { + var fileName = match.Result("${filename}"); + // Read file SendSuccessConfirmation(channel); // Send reply - var length = long.Parse(match.Result("${length}"), CultureInfo.InvariantCulture); - var fileName = match.Result("${filename}"); + // The server-supplied name is combined into a local path; ensure it + // cannot escape the destination directory. + EnsureValidLocalName(fileName); - 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); + var fileInfo = new FileInfo(Path.Combine(currentDirectoryFullName, fileName)); - fileInfo = new FileInfo(Path.Combine(currentDirectoryFullName, fileName)); - } - - using (var output = fileInfo.Open(FileMode.Create, FileAccess.Write)) - { - InternalDownload(channel, input, output, fileName, length); - } - - fileInfo.LastAccessTime = accessedTime; - fileInfo.LastWriteTime = modifiedTime; + DownloadFileContent(channel, input, fileInfo, fileName, length, modifiedUtc, accessedUtc); if (directoryCounter == 0) { @@ -1041,21 +1129,17 @@ namespace Renci.SshNet } match = TimestampRegex.Match(message); - if (match.Success) + if (match.Success && + long.TryParse(match.Result("${mtime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var mtime) && + long.TryParse(match.Result("${atime}"), NumberStyles.None, CultureInfo.InvariantCulture, out var atime) && + TryFromUnixTimeSeconds(mtime, out modifiedUtc) && + TryFromUnixTimeSeconds(atime, out accessedUtc)) { - // Read timestamp - SendSuccessConfirmation(channel); // Send reply - - var mtime = long.Parse(match.Result("${mtime}"), CultureInfo.InvariantCulture); - var atime = long.Parse(match.Result("${atime}"), CultureInfo.InvariantCulture); - - var zeroTime = DateTime.UnixEpoch; - modifiedTime = zeroTime.AddSeconds(mtime); - accessedTime = zeroTime.AddSeconds(atime); + SendSuccessConfirmation(channel); continue; } - SendErrorConfirmation(channel, string.Format("\"{0}\" is not valid protocol message.", message)); + SendInvalidRecordErrorAndThrow(channel); } } } diff --git a/test/Renci.SshNet.IntegrationTests/ScpTests.cs b/test/Renci.SshNet.IntegrationTests/ScpTests.cs index 971a624a..5e6704d5 100644 --- a/test/Renci.SshNet.IntegrationTests/ScpTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ScpTests.cs @@ -2070,6 +2070,55 @@ namespace Renci.SshNet.IntegrationTests } } + /// + /// A file whose name contains the text of a D record must be downloaded as a file, with its + /// content written verbatim rather than interpreted as SCP records. + /// + [TestMethod] + public void Scp_Download_DirectoryInfo_FileNameContainingRecordText_IsDownloadedAsFile() + { + const string FileName = "zD0755 0 x"; + const string Content = "E\nC0644 8 injected.txt\ninjected"; + + var remoteDirectory = "/tmp/sshnet-scp-records-" + Guid.NewGuid().ToString("N"); + + using (var client = new SftpClient(_connectionInfoFactory.Create())) + { + client.Connect(); + client.CreateDirectory(remoteDirectory); + client.WriteAllText(remoteDirectory + "/" + FileName, Content); + } + + var localRoot = Path.Combine(Path.GetTempPath(), "sshnet-scp-records-" + Guid.NewGuid().ToString("N")); + var destination = Directory.CreateDirectory(Path.Combine(localRoot, "download")); + + try + { + using (var client = new ScpClient(_connectionInfoFactory.Create(), _remotePathTransformation)) + { + client.Connect(); + client.Download(remoteDirectory, destination); + } + + var filePath = Path.Combine(destination.FullName, FileName); + + CollectionAssert.AreEquivalent( + new[] { destination.FullName, filePath }, + Directory.GetFileSystemEntries(localRoot, "*", SearchOption.AllDirectories)); + Assert.AreEqual(Content, File.ReadAllText(filePath)); + } + finally + { + using (var client = new SshClient(_connectionInfoFactory.Create())) + { + client.Connect(); + _ = client.RunCommand("rm -rf '" + remoteDirectory + "'"); + } + + Directory.Delete(localRoot, recursive: true); + } + } + /// /// 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, diff --git a/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs new file mode 100644 index 00000000..dd348445 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs @@ -0,0 +1,194 @@ +using System; +using System.IO; +using System.Linq; +using System.Text; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// Verifies how downloads handle control records: names that contain record-like text, records + /// that are malformed or unknown, and records that are valid but not expected at that point. The + /// single-file downloads accept only the records that a server sends for a single file: for + /// a C record, and for + /// an optional T record followed by a C record. + /// + [TestClass] + public class ScpClientTest_Download_InvalidRecord : ScpClientTranscriptTestBase + { + private string Target + { + get + { + return Path.Combine(Destination, "target"); + } + } + + [TestMethod] + public void FileNameContainingDirectoryRecord_ShouldBeDownloadedAsFile() + { + // An honest server sends a file whose (legitimate) name contains the text of a D + // record. Its content must be written to that file, not interpreted as records. + const string Name = "zD0755 0 x"; + const string Content = "E\nC0644 8 injected.txt\ninjected"; + + var transcript = new ScpServerTranscript() + .Line("D0755 0 remote") + .File(Name, Content) + .Line("E"); + + var result = transcript.Run( + client => client.Download("/remote", new DirectoryInfo(Destination)), + closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { Name }, ListEntries(Destination)); + Assert.AreEqual(Content, File.ReadAllText(Path.Combine(Destination, Name))); + } + + [TestMethod] + [DataRow("Xjunk")] + [DataRow("garbageC0644 1 f")] + [DataRow("C0644 99999999999999999999 f")] + [DataRow("C0644 +1 f")] + [DataRow("C0899 1 f")] + [DataRow("T99999999999999999999 0 1 0")] + [DataRow("T999999999999 0 1 0")] + [DataRow("E trailing")] + [DataRow("C0644 1 f")] + [DataRow("C064 1 f")] + [DataRow("C0644 1 ")] + [DataRow("D0755 0")] + [DataRow("C0644 \u0663 f")] + [DataRow("C\u0660644 1 f")] + [DataRow("T1 0 2")] + [DataRow("T\u0661 0 2 0")] + public void DownloadDirectoryInfo_InvalidRecord_ShouldThrowScpException(string line) + { + var transcript = new ScpServerTranscript() + .Line("D0755 0 remote") + .Line(line) + .File("after.txt", "abc") + .Line("E"); + + var result = transcript.Run( + client => client.Download("/remote", new DirectoryInfo(Destination)), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 2); + AssertNothingOutsideDestination(); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + + [TestMethod] + [DataRow("Xjunk")] + [DataRow("C0644 99999999999999999999 f")] + [DataRow("T999999999999 0 1 0")] + [DataRow("D0755 0 dir")] + [DataRow("E")] + public void DownloadFileInfo_RecordOtherThanTimesOrFile_ShouldThrowScpException(string line) + { + var transcript = new ScpServerTranscript() + .Line(line) + .File("f", "abc"); + + var result = transcript.Run( + client => client.Download("/remote/f", new FileInfo(Target)), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 1); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + + [TestMethod] + [DataRow("T1 0 2 0")] + [DataRow("D0755 0 dir")] + [DataRow("E")] + [DataRow("Xjunk")] + public void DownloadFileInfo_TimesFollowedByRecordOtherThanFile_ShouldThrowScpException(string line) + { + // Only one T record is allowed, and it must be followed by the C record. + var transcript = new ScpServerTranscript() + .Line("T1700000000 0 1700000000 0") + .Line(line) + .File("f", "abc"); + + var result = transcript.Run( + client => client.Download("/remote/f", new FileInfo(Target)), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 2); + Assert.IsEmpty(Directory.GetFileSystemEntries(Destination)); + } + + [TestMethod] + public void DownloadFileInfo_TimesThenFile_ShouldWriteToTargetIgnoringServerName() + { + var transcript = new ScpServerTranscript() + .Line("T1700000000 0 1700000000 0") + .File("server-name", "content"); + + var result = transcript.Run( + client => client.Download("/remote/f", new FileInfo(Target)), + closeChannel: true); + + Assert.IsTrue(result.Completed); + Assert.IsNull(result.Exception, result.Exception?.ToString()); + CollectionAssert.AreEquivalent(new[] { "target" }, ListEntries(Destination)); + Assert.AreEqual("content", File.ReadAllText(Target)); + Assert.AreEqual(DateTime.UnixEpoch.AddSeconds(1700000000), File.GetLastWriteTimeUtc(Target)); + } + + [TestMethod] + [DataRow("Xjunk")] + [DataRow("garbageC0644 1 f")] + [DataRow("C0644 99999999999999999999 f")] + [DataRow("C0644 9223372036854775808 f")] + [DataRow("D0755 0 dir")] + [DataRow("E")] + [DataRow("T1 0 2 0")] + public void DownloadStream_RecordOtherThanFile_ShouldThrowScpException(string line) + { + var transcript = new ScpServerTranscript() + .Line(line) + .File("f", "abc"); + + using (var destination = new MemoryStream()) + { + var result = transcript.Run( + client => client.Download("/remote/f", destination), + closeChannel: true); + + AssertRejected(result, expectedSuccessStatuses: 1); + Assert.AreEqual(0, destination.Length); + } + } + + private static void AssertRejected(ScpTranscriptResult result, int expectedSuccessStatuses) + { + Assert.IsTrue(result.Completed); + Assert.IsInstanceOfType(result.Exception); + Assert.Contains("invalid or unexpected SCP record", result.Exception.Message, StringComparison.Ordinal); + + // The rejected record is not acknowledged: the only success status bytes are the one sent + // after the exec request and one for each record accepted before it. + Assert.AreEqual( + expectedSuccessStatuses, + result.SentData.Count(data => data.Length == 1 && data[0] == 0), + "Unexpected number of success status bytes; was the rejected record acknowledged?"); + + // The error is also reported to the server: a 0x01 status byte followed by a message. + Assert.IsTrue( + result.SentData.Any(data => data.Length == 1 && data[0] == 1), + "No error status was sent to the server."); + Assert.IsTrue( + result.SentData.Any(data => string.Equals(Encoding.UTF8.GetString(data), "Invalid or unexpected SCP record.\n", StringComparison.Ordinal)), + "No error message was sent to the server."); + } + } +}