diff --git a/src/twisted/conch/newsfragments/10293.bugfix b/src/twisted/conch/newsfragments/10293.bugfix new file mode 100644 index 00000000000..ba00280afea --- /dev/null +++ b/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. diff --git a/src/twisted/conch/ssh/filetransfer.py b/src/twisted/conch/ssh/filetransfer.py index 92c3d7a08f5..291f67c9d6c 100644 --- a/src/twisted/conch/ssh/filetransfer.py +++ b/src/twisted/conch/ssh/filetransfer.py @@ -5,6 +5,7 @@ import errno +import os import struct import warnings from typing import Dict @@ -35,15 +36,37 @@ 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( @@ -51,12 +74,16 @@ def dataReceived(self, data): 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: diff --git a/src/twisted/conch/test/test_filetransfer.py b/src/twisted/conch/test/test_filetransfer.py index 44d67c7b072..2a3b10c6632 100644 --- a/src/twisted/conch/test/test_filetransfer.py +++ b/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): """