CERBERUS/kitty
1
0
Fork 0
mirror of https://github.com/kovidgoyal/kitty.git synced 2026-10-12 00:38:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

file transfer protocol: harden against malicious clients planting symlinks

Browse source 
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...
This commit is contained in:
Kovid Goyal 2026-09-01 22:23:24 +05:30
parent 03fbdad855
commit ede47d2f93
No known key found for this signature in database
GPG key ID: 06BC317B515ACE7C
2 changed files with 125 additions and 4 deletions
Download patch file Download diff file Expand all files Collapse all files

23
kitty/file_transmission.py
View file

@ -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)

106
kitty_tests/file_transmission.py
View file

@ -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):