lsetstat@openssh.com with PERMISSIONS answers SSH_FX_OK and discards the request on platforms without lchmod

[kmoneil]

The SFTP server answers SSH_FX_OK to an lsetstat@openssh.com request carrying
SSH_FILEXFER_ATTR_PERMISSIONS and changes neither the link's mode nor the target's, on any
platform where os.chmod does not support follow_symlinks=False (Linux, notably — there is no
lchmod, and fchmodat(AT_SYMLINK_NOFOLLOW) answers ENOTSUP).

A client that asked not to follow the link is told the permission change happened. It did not, and
nothing in the response distinguishes that from success.

This is narrow: it is one flag, not the extension. ACMODTIME over the same request works
correctly — the link's own timestamp really changes — so lsetstat itself is fine.

Reproducer

asyncssh client against asyncssh server, one process, no third-party code.

import asyncio, os, stat, tempfile
from pathlib import Path

import asyncssh


class NoAuthServer(asyncssh.SSHServer):
    def begin_auth(self, username):
        return False


def mode_of(path, *, follow):
    return format(stat.S_IMODE((os.stat if follow else os.lstat)(path).st_mode), "04o")


async def main():
    root = Path(tempfile.mkdtemp())
    target, link = root / "target", root / "link"
    target.write_bytes(b"payload")
    os.chmod(target, 0o600)
    os.symlink(target, link)

    server = await asyncssh.create_server(
        NoAuthServer, "127.0.0.1", 0,
        server_host_keys=[asyncssh.generate_private_key("ssh-ed25519")],
        sftp_factory=True,
    )
    port = server.sockets[0].getsockname()[1]

    async with asyncssh.connect(
        "127.0.0.1", port, username="probe", known_hosts=None
    ) as conn, conn.start_sftp_client() as sftp:
        print(f"before: link {mode_of(link, follow=False)}  target {mode_of(target, follow=True)}")
        await sftp.chmod(str(link), 0o640, follow_symlinks=False)
        print("server answered SSH_FX_OK (no exception raised)")
        print(f"after:  link {mode_of(link, follow=False)}  target {mode_of(target, follow=True)}")

        # The control: the same request shape with ACMODTIME instead, which works.
        before = os.lstat(link).st_mtime_ns, os.stat(target).st_mtime_ns
        await sftp.utime(str(link), (1_000_000_000, 1_000_000_000), follow_symlinks=False)
        print(f"link mtime changed:   {os.lstat(link).st_mtime_ns != before[0]}")
        print(f"target mtime changed: {os.stat(target).st_mtime_ns != before[1]}")

    server.close()
    await server.wait_closed()


asyncio.run(main())

Output, asyncssh 2.24.0, CPython 3.13.15, Linux 6.12 aarch64:

before: link 0777  target 0600
server answered SSH_FX_OK (no exception raised)
after:  link 0777  target 0600
link mtime changed:   True
target mtime changed: False

(0777 is what a Linux symlink always reports; the point is that neither it nor the target moved.)

Where it comes from

SFTPServer.lsetstat calls _setstat(..., follow_symlinks=False), and that function swallows the
exception CPython raises — asyncssh/sftp.py:632-637 in 2.24.0, unchanged on develop at
a787030:

    if attrs.permissions is not None:
        try:
            os.chmod(path, stat.S_IMODE(attrs.permissions),
                     follow_symlinks=follow_symlinks)
        except NotImplementedError: # pragma: no cover
            pass

On Linux, os.chmod not in os.supports_follow_symlinks, so that call raises
NotImplementedError: chmod: follow_symlinks unavailable on this platform and the request is
discarded. _process_lsetstat then returns SSH_FX_OK, because as far as it can tell the handler
succeeded.

The two neighbouring clauses (os.chown, os.utime) have the same except NotImplementedError: pass, but both of those are in os.supports_follow_symlinks on Linux, so they never fire —
which is why ACMODTIME works and only PERMISSIONS is dropped.

SSH_FILEXFER_ATTR_UIDGID is untested here: an unprivileged reproducer can only chown a file to
the uid it already has, so "nothing moved" is the expected result either way.

Why it seems worth fixing rather than documenting

A caller passing follow_symlinks=False is making a security decision — do not act on whatever
this link points at. Three outcomes are possible and SSH_FX_OK names none of them: the link's
mode changed, the target's changed (the link was followed, which is the thing being refused), or
nothing changed. There is no way to tell them apart without statting both files afterwards.

OpenSSH's sftp-server refuses the same request on the same kernel, with SSH_FX_FAILURE. So two
servers advertising the same extension give opposite answers, and the one that says yes is the one
that did nothing.

Suggested fix

SSH_FX_OP_UNSUPPORTED is what the protocol has for this, and asyncssh already produces it —
SFTPServerHandler._process_packet maps NotImplementedError to FX_OP_UNSUPPORTED
(sftp.py:6058). The swallow one layer down is the only thing preventing it.

Removing just that try/except (nothing else) gives the client:

SFTPOpUnsupported  code=8  reason='Operation not supported: lsetstat'

Whether the other two clauses should also stop swallowing is a judgement call — on Linux they are
unreachable, but the same silent-success shape is available to them on a platform where those
os.* calls lack follow_symlinks support.

Worth noting that the # pragma: no cover on the clause means no test exercises it, which is
presumably why this has gone unnoticed.

Versions

• asyncssh 2.24.0 (latest release), and develop at a787030 is unchanged
• CPython 3.13.15
• Linux 6.12.76 aarch64

[ronf]

Thanks for the report!

One challenge here is that _setstat() can be called with multiple attributes set. So, some changes might be successful and others might fail, and that failure could prevent some of the remaining items from being successfully changed.

One option would be to swallow the error at first to try and attempt all of the requested changed, but then report an error at the end if there were any failures. It might be possible to adjust the details string to list which attributes couldn't be changed in the result that's returned here. The caller would need to understand that attributes reported in the error weren't changed, but the attributes in the request which were not listed in the error were changed. What do you think of this approach?

There would still be cases where other OS errors are returned when setting certain attributes, and that would be something we could detect up front. There's a question here about whether to make this general or not, though, as we can't report back more than one kind of error in the result. So, I'm thinking we maybe only do this for NotImplementedError and the AttributeError that shows up on Windows when some "os" functions like chmod() are completely missing.

In the case of other OS errors, it would continue to behave as it does now with an unknown set of changes which might have succeeded, and others which were not tried even though they could have succeeded, depending on the order in _setstat(). However, this would at least do better in the case of OS-related unsupported operations.

For symlinks in particular, MacOS does support changing permissions on symlinks, honoring follow_symlinks to decide which file permissions are changed. It looks like Windows supports follow_symlinks for chmod() as well according to os.supports_follow_symlinks, but in a quick test I could only get os.chmod() to work on regular files, not links, and even setting follow_symlinks to True didn't seem to get it to change permissions on the file the link pointed at. So, while it doesn't give an error when you attempt to use it, there's very little you can do with chmod on Windows.

[kmoneil]

Thanks for taking a look.

You're right that removing the try/except on its own is wrong. chmod runs before utime in _setstat, so a request carrying both PERMISSIONS and ACMODTIME would start failing on Linux where the timestamp half works today. I missed that.

Attempt everything and report at the end works for me. The part that matters is the direction it fails in. Right now the caller is told the change happened and has no reason to go check. With an error at the end they know to look, even if the message doesn't say exactly what applied.

One thought on the scope. NotImplementedError and the Windows AttributeError are both things you can check before touching anything (os.chmod in os.supports_follow_symlinks, hasattr(os, "chown")). Checking up front lets you refuse the request having changed nothing, so there's no partial state and nothing to list in the details string. Collecting and reporting at the end seems more useful for the OS errors you're leaving alone, since those you can't know in advance.

Either way is fine by me. Not answering OK is the part I care about.

On the details string, I'd read it in a log but wouldn't write a client that parses it. Fine as long as it isn't the only signal.