Bug 73878

That's true. Would any attacks remain possible if the code did this:

if e.errno == errno.EEXIST:
    # Already exists - set 700 permissions again.
    import stat
    perms700 = stat.S_IRUSR|stat.S_IWUSR|stat.S_IXUSR
    uid, gid = os.getuid(), os.getgid()
    if os.path.islink(fallback):
        os.lchown(fallback, uid, gid)
        os.lchmod(fallback, perms700)
    os.chown(fallback, uid, gid
    os.chmod(fallback, perms700)

That ensures that the symlink as well as the directory is accessible only to the user who is running this, so it should be impossible for another user to replace it. Setting this on the symlink before the directory prevents pointing it to a different directory after the code thinks the directory is OK.

I should probably also check that the fallback *is* a directory, but I don't think that's a security issue.

I wonder if instead of resetting the permissions/owner, it might be better to check them first and try to recreate the directory completely if they are wrong?

Thanks!

I'd be wary of deleting and recreating the directory in case something is legitimately using it but the permissions have got broken. But can you see an attack that that would prevent but resetting the permissions wouldn't?

The code in comment 1 is racy. To win the race attacker has to:

1) Create /tmp/pyxdg-runtime-dir-fallback-victim as a regular directory.

2) Between the call to os.path.islink (which returns false) and the call to os.chown replace /tmp/pyxdg-runtime-dir-fallback-victim with a symlink to a victim's. file.

Oh, and os.lchmod() doesn't exist on Linux. :-)

I'm not sure it would be practical to exploit a hole like that, but I think we can avoid it by using a stat call to collect all the info we need in one go (so long as stat itself is atomic).

How about this:

if e.errno == errno.EEXIST:
    st = os.lstat(fallback)
    if not stat.S_ISDIR(st.st_mode):
        # If it's not a directory, remake it
        # This catches it being a symlink
        os.unlink(fallback)
        os.mkdir(fallback, 0o700)

    else:
        uid = os.getuid()
        if st.st_uid != uid:
            # gid doesn't matter, because we set user-only access
            os.chown(fallback, uid, -1)
        if st.st_mode & (stat.S_IRWXG | stat.S_IRWXO)
            os.chmod(stat.S_IRWXU)

This would mean that you couldn't intentionally use a symlink for this directory, but since it's created specifically by and for PyXDG, that doesn't seem a big issue.

No, that's still susceptible to the same race condition, isn't it. I'll think some more about it.

OK, trying again, and replacing more of the function:

create = False
try:
    st = os.lstat(fallback)
except FileNotFoundError:
    create = True
else:
    if not stat.S_ISDIR(st.st_mode):
        os.unlink(fallback)
        create = True
    elif (st.st_uid != uid) \
      or (st.st_mode & (stat.S_IRWXG | stat.S_IRWXO):
        os.rmdir(fallback)
        create = True


if create:
    os.mkdir(fallback, 0o700)

This is subject to denial of service attacks, if something exists we can't remove, or if there is no file when lstat is called, but one is made before mkdir is called. I don't see any way around that given that we want the directory to be a stable location.

Is there any way to make the code above succeed but do something unexpected?

From the security perspective, the code in comment 7 looks good to me (except for the already-mentioned possiblity of DoS).

The code will have to be adapted to work with older Python versions: there's no FileNotFoundError in Python < 3.3.

Minor style nitpick: "(stat.S_IRWXG | stat.S_IRWXO)" makes my head hurt; I'd write "0o077" instead.

If you want to avoid DoS, here's a way to do it:
1) Create temporary directory with mkdtemp(). Store the directory name somewhere in $HOME, say $XDG_DATA_HOME/pyxdg/runtime-dir.
2) Next time the function is called, use the directory stored in $XDG_DATA_HOME/pyxdg/runtime-dir if possible (i.e. when it passes the same security checks as in comment 7). If it's not possible, goto 1.

Fix committed: https://github.com/takluyver/pyxdg/commit/bd999c1c3fe7ee5f30ede2cf704cf03e400347b4

I'm inclined to keep the predictable location of the fallback directory for now, rather than preventing DoS attacks, but I'm open to more discussion of that.

Now I need to try to get into my fd.o repo. Hopefully I've got that SSH key around somewhere...