From ede47d2f9305acaceaf2f6a3fe220668b5d0ada8 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Tue, 1 Sep 2026 22:23:24 +0530 Subject: [PATCH] file transfer protocol: harden against malicious clients planting symlinks This is actually not a security boundary for this protocol. Since it is explicitly *not* designed to be proof against malicious clients. However, to avoid more AI generated security reports against it... --- kitty/file_transmission.py | 23 +++++-- kitty_tests/file_transmission.py | 106 +++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 4 deletions(-) diff --git a/kitty/file_transmission.py b/kitty/file_transmission.py index 9450237ec..a6a3b324a 100644 --- a/kitty/file_transmission.py +++ b/kitty/file_transmission.py @@ -393,7 +393,10 @@ class PatchFile: @property def dest_file(self) -> IO[bytes]: if self._dest_file is None: - self._dest_file = tempfile.NamedTemporaryFile(mode='wb', dir=os.path.dirname(os.path.abspath(os.path.realpath(self.path))), delete=False) + # Note: deliberately no realpath() here, the temporary file must be + # created next to the destination itself, not next to whatever a + # symlink at the destination happens to point to. + self._dest_file = tempfile.NamedTemporaryFile(mode='wb', dir=os.path.dirname(os.path.abspath(self.path)), delete=False) return self._dest_file def close(self) -> None: @@ -406,7 +409,7 @@ class PatchFile: self._dest_file.close() p.finish_delta_data() if self.src_file is not None: - os.replace(self.dest_file.name, self.src_file.name) + os.replace(self.dest_file.name, self.path) if self.src_file is not None and not self.src_file.closed: self.src_file.close() @@ -431,7 +434,10 @@ class PatchFile: if self.signature_done: return 0 if self.src_file is None: - self.src_file = open(self.path, 'rb') + # O_NOFOLLOW so that a symlink planted at the destination cannot be + # used to read some other file and leak its signature to the sender + flags = os.O_RDONLY | getattr(os, 'O_CLOEXEC', 0) | getattr(os, 'O_BINARY', 0) | getattr(os, 'O_NOFOLLOW', 0) + self.src_file = open(os.open(self.path, flags), mode='rb', closefd=True) return self.patcher.signature_header(buf) n = self.src_file.readinto(self.block_buffer) if n > 0: @@ -469,6 +475,15 @@ class DestFile: self.failed = False self.bytes_written = 0 + @property + def can_patch_in_place(self) -> bool: + # A differential (rsync) transfer reads the existing file and renames a + # temporary file over it, bypassing the unlink and O_NOFOLLOW defences + # in write_data(). So only allow it when the destination is a plain + # regular file with no other links to it. + es = self.existing_stat + return es is not None and not self.needs_unlink and stat.S_ISREG(es.st_mode) + def signature_iterator(self) -> PatchFile: self.actual_file = PatchFile(self.name, self.existing_stat.st_size if self.existing_stat is not None else 0) return self.actual_file @@ -1028,7 +1043,7 @@ class FileTransmission: sz = df.existing_stat.st_size if df.existing_stat is not None else -1 ttype = ( TransmissionType.rsync - if sz > -1 and df.ttype is TransmissionType.rsync and df.ftype is FileType.regular + if sz > -1 and df.ttype is TransmissionType.rsync and df.ftype is FileType.regular and df.can_patch_in_place else TransmissionType.simple ) self.send_status_response(code=ErrorCode.STARTED, request_id=ar.id, file_id=df.file_id, name=df.name, size=sz, ttype=ttype) diff --git a/kitty_tests/file_transmission.py b/kitty_tests/file_transmission.py index 3cfdee623..a9df9ad89 100644 --- a/kitty_tests/file_transmission.py +++ b/kitty_tests/file_transmission.py @@ -226,6 +226,112 @@ class TestFileTransmission(BaseTest): def test_rsync_roundtrip(self): test_rsync_roundtrip(self) + def test_file_put_differential(self): + from kitty.file_transmission import PatchFile + + def start_receiving(): + ft = FileTransmission() + ft.handle_serialized_command(serialized_cmd(action='send')) + ft.test_responses = [] + return ft + + def started_ttype(ft): + q = [r for r in ft.test_responses if r.get('status') == 'STARTED'] + self.ae(len(q), 1) + return q[0].get('ttype', 'simple') + + def signature_sent(ft): + return b''.join(r['data'] for r in ft.test_responses if r.get('action') == 'data' and r.get('data')) + + base = os.path.join(self.tdir, 'diff') + os.mkdir(base) + + # a differential transfer to a plain regular file works as usual + dest = os.path.join(base, 'f.bin') + before = b'hello world ' * 5000 + after = (b'hello world ' * 4000) + (b'goodbye!' * 2000) + with open(dest, 'wb') as f: + f.write(before) + ft = start_receiving() + ft.handle_serialized_command(serialized_cmd(action='file', file_id='r', name=dest, ftype='regular', ttype='rsync')) + self.ae(started_ttype(ft), 'rsync') + d = Differ() + d.add_signature_data(signature_sent(ft)) + d.finish_signature_data() + src = memoryview(after) + delta = bytearray() + + def read_into(b): + nonlocal src + n = min(len(b), len(src)) + b[:n] = src[:n] + src = src[n:] + return n + + while d.next_op(read_into, delta.extend): + pass + chunks = memoryview(bytes(delta)) + while len(chunks) > 4096: + ft.handle_serialized_command(serialized_cmd(action='data', file_id='r', data=bytes(chunks[:4096]))) + chunks = chunks[4096:] + ft.handle_serialized_command(serialized_cmd(action='end_data', file_id='r', data=bytes(chunks))) + with open(dest, 'rb') as f: + self.ae(f.read(), after) + self.ae(sorted(names_in(base)), ['f.bin']) + + # a sender must not be able to use a differential transfer to read + # through a symlink it plants at the destination in the same transfer + secret_dir = os.path.join(base, 'secret') + os.mkdir(secret_dir) + secret = os.path.join(secret_dir, 'secret.txt') + secret_data = b'super secret data' * 1000 + with open(secret, 'wb') as f: + f.write(secret_data) + sdest = os.path.join(base, 'innocent.bin') + ft = start_receiving() + ft.handle_serialized_command(serialized_cmd(action='file', file_id='s', name=sdest, ftype='symlink')) + ft.handle_serialized_command(serialized_cmd(action='end_data', file_id='s', data='path:' + secret)) + self.assertTrue(os.path.islink(sdest)) + ft.test_responses = [] + ft.handle_serialized_command(serialized_cmd(action='file', file_id='r', name=sdest, ftype='regular', ttype='rsync')) + self.ae(started_ttype(ft), 'simple') + self.ae(signature_sent(ft), b'') + ft.handle_serialized_command(serialized_cmd(action='end_data', file_id='r', data='replacement')) + self.assertFalse(os.path.islink(sdest)) + with open(sdest, 'rb') as f: + self.ae(f.read(), b'replacement') + with open(secret, 'rb') as f: + self.ae(f.read(), secret_data) + self.ae(sorted(names_in(secret_dir)), ['secret.txt']) + + # likewise for a destination with more than one link to it + hdest = os.path.join(base, 'hard.bin') + other = os.path.join(base, 'other.bin') + with open(hdest, 'wb') as f: + f.write(b'original') + os.link(hdest, other) + ft = start_receiving() + ft.handle_serialized_command(serialized_cmd(action='file', file_id='r', name=hdest, ftype='regular', ttype='rsync')) + self.ae(started_ttype(ft), 'simple') + self.ae(signature_sent(ft), b'') + ft.handle_serialized_command(serialized_cmd(action='end_data', file_id='r', data='replacement')) + with open(other, 'rb') as f: + self.ae(f.read(), b'original') + + # and PatchFile itself must never follow a symlink, in case the + # destination is swapped after it is created but before it is read + pf = PatchFile(sdest, len(secret_data)) + os.remove(sdest) + os.symlink(secret, sdest) + with self.assertRaises(OSError): + pf.next_signature_block(memoryview(bytearray(64))) + # its temporary file goes next to the destination, not next to the + # symlink target + tf = pf.dest_file + tf.close() + self.assertPathEqual(os.path.dirname(tf.name), base) + os.remove(tf.name) + def test_file_get(self): # send refusal for quiet in (0, 1, 2):