Merge pull request #1685 from twisted/10293-conch-sftp-close-invalid-…

…handle

Author: exarkun
Reviewer: wsanchez
Fixes: ticket:10293

Handle close of an invalid handle in Conch's SFTP server by sending
an error response instead of raising an unhandled AttributeError and
never responding to the request.

src/twisted/conch/newsfragments/10293.bugfix

@@ -0,0 +1 @@
+twisted.conch.ssh.filetransfer.FileTransferServer will now return an ENOENT error status if an SFTP client tries to close an unrecognized file handle.

src/twisted/conch/ssh/filetransfer.py

@@ -5,6 +5,7 @@
 
 
 import errno
+import os
 import struct
 import warnings
 from typing import Dict
@@ -35,28 +36,54 @@ def sendPacket(self, kind, data):
 
     def dataReceived(self, data):
         self.buf += data
-        while len(self.buf) > 5:
-            length, kind = struct.unpack("!LB", self.buf[:5])
+
+        # Continue processing the input buffer as long as there is a chance it
+        # could contain a complete request.  The "General Packet Format"
+        # (format all requests follow) is a 4 byte length prefix, a 1 byte
+        # type field, and a 4 byte request id.  If we have fewer than 4 + 1 +
+        # 4 == 9 bytes we cannot possibly have a complete request.
+        while len(self.buf) >= 9:
+            header = self.buf[:9]
+            length, kind, reqId = struct.unpack("!LBL", header)
+            # From draft-ietf-secsh-filexfer-13 (the draft we implement):
+            #
+            #   The `length' is the length of the data area [including the
+            #   kind byte], and does not include the `length' field itself.
+            #
+            # If the input buffer doesn't have enough bytes to satisfy the
+            # full length then we cannot process it now.  Wait until we have
+            # more bytes.
             if len(self.buf) < 4 + length:
                 return
+
+            # We parsed the request id out of the input buffer above but the
+            # interface to the `packet_TYPE` methods involves passing them a
+            # data buffer which still includes the request id ... So leave
+            # those bytes in the `data` we slice off here.
             data, self.buf = self.buf[5 : 4 + length], self.buf[4 + length :]
+
             packetType = self.packetTypes.get(kind, None)
             if not packetType:
                 self._log.info("no packet type for {kind}", kind=kind)
                 continue
+
             f = getattr(self, f"packet_{packetType}", None)
             if not f:
                 self._log.info(
                     "not implemented: {packetType} data={data!r}",
                     packetType=packetType,
                     data=data[4:],
                 )
-                (reqId,) = struct.unpack("!L", data[:4])
                 self._sendStatus(
                     reqId, FX_OP_UNSUPPORTED, f"don't understand {packetType}"
                 )
                 # XXX not implemented
                 continue
+            self._log.info(
+                "dispatching: {packetType} requestId={reqId}",
+                packetType=packetType,
+                reqId=reqId,
+            )
             try:
                 f(data)
             except Exception:
@@ -178,6 +205,11 @@ def packet_CLOSE(self, data):
         requestId = data[:4]
         data = data[4:]
         handle, data = getNS(data)
+        self._log.info(
+            "closing: {requestId!r} {handle!r}",
+            requestId=requestId,
+            handle=handle,
+        )
         assert data == b"", f"still have data in CLOSE: {data!r}"
         if handle in self.openFiles:
             fileObj = self.openFiles[handle]
@@ -190,7 +222,10 @@ def packet_CLOSE(self, data):
             d.addCallback(self._cbClose, handle, requestId, 1)
             d.addErrback(self._ebStatus, requestId, b"close failed")
         else:
-            self._ebClose(failure.Failure(KeyError()), requestId)
+            code = errno.ENOENT
+            text = os.strerror(code)
+            err = OSError(code, text)
+            self._ebStatus(failure.Failure(err), requestId)
 
     def _cbClose(self, result, handle, requestId, isDir=0):
         if isDir:

src/twisted/conch/test/test_filetransfer.py

@@ -12,12 +12,15 @@
 import struct
 from unittest import skipIf
 
+from hamcrest import assert_that, equal_to
+
 from twisted.internet import defer
 from twisted.internet.error import ConnectionLost
 from twisted.protocols import loopback
 from twisted.python import components
 from twisted.python.compat import _PY37PLUS
 from twisted.python.filepath import FilePath
+from twisted.test.proto_helpers import StringTransport
 from twisted.trial.unittest import TestCase
 
 try:
@@ -852,6 +855,68 @@ def test_constantsAgainstSpec(self):
             self.assertEqual(v, getattr(filetransfer, k))
 
 
+@skipIf(not cryptography, "Cannot run without cryptography")
+class RawPacketDataServerTests(TestCase):
+    """
+    Tests for L{filetransfer.FileTransferServer} which explicitly craft
+    certain less common situations to exercise their handling.
+    """
+
+    def setUp(self):
+        self.fts = filetransfer.FileTransferServer(avatar=TestAvatar())
+
+    def test_closeInvalidHandle(self):
+        """
+        A close request with an unknown handle receives an FX_NO_SUCH_FILE error
+        response.
+        """
+        transport = StringTransport()
+        self.fts.makeConnection(transport)
+
+        # any four bytes
+        requestId = b"1234"
+        # The handle to close, arbitrary bytes.
+        handle = b"invalid handle"
+
+        # Construct a message packet
+        # https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-4
+        close = common.NS(
+            # Packet type - SSH_FXP_CLOSE
+            # https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-4.3
+            bytes([4])
+            + requestId
+            + common.NS(handle)
+        )
+
+        self.fts.dataReceived(close)
+
+        # An SSH_FXP_STATUS message
+        # https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-9.1
+        expected = common.NS(
+            # Packet type SSH_FXP_STATUS
+            # https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-4.3
+            bytes([101])
+            +
+            # The same request id
+            requestId
+            +
+            # A four byte status code.  SSH_FX_NO_SUCH_FILE in this case.
+            bytes([0, 0, 0, 2])
+            +
+            # Error message
+            common.NS(b"No such file or directory")
+            +
+            # error message language tag - conch doesn't send one at all,
+            # though maybe it should
+            common.NS(b"")
+        )
+
+        assert_that(
+            transport.value(),
+            equal_to(expected),
+        )
+
+
 @skipIf(not cryptography, "Cannot run without cryptography")
 class RawPacketDataTests(TestCase):
     """