From eba6e8210e119e9cc30f5458ce82db365aa415f2 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Thu, 8 Oct 2026 08:53:06 +0000 Subject: [PATCH] Tighten SCP record parsing and handling The C, D and T record patterns were not anchored and D was tried before C, so a file named, for example, "zD0755 0 x" was treated as a directory and its content read as SCP records, letting whoever controls a file on the server choose the names and contents of files written locally. The patterns now match the whole line. An invalid or unexpected record is reported to the server and thrown as ScpException; previously an unknown record was reported and ignored, and Download(string, Stream) then returned normally with an empty stream. Download(string, FileInfo) no longer shares the directory loop. It accepts an optional T record followed by a C record, so a D or E record can no longer cause an InvalidCastException or let later C records overwrite the file. The directory loop moves to DownloadDirectoryContent and the content copy is renamed DownloadFileContent. --- src/Renci.SshNet/ScpClient.cs | 186 ++++++++++++----- .../Renci.SshNet.IntegrationTests/ScpTests.cs | 49 +++++ .../ScpClientTest_Download_InvalidRecord.cs | 194 ++++++++++++++++++ 3 files changed, 378 insertions(+), 51 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/ScpClientTest_Download_InvalidRecord.cs 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."); + } + } +}