Vulnerable tmpdir handling

[orlitzky]

Starting around https://github.com/pytest-dev/pytest/blob/main/src/_pytest/tmpdir.py#L156, there are some heuristics to make sure that the tmpdir is safe:

            temproot = Path(from_env or tempfile.gettempdir()).resolve()
            user = get_user() or "unknown"
            # use a sub-directory in the temproot to speed-up                                                                                                  
            # make_numbered_dir() call                                                                                                                         
            rootdir = temproot.joinpath(f"pytest-of-{user}")
            try:
                rootdir.mkdir(mode=0o700, exist_ok=True)
            except OSError:
                # getuser() likely returned illegal characters for the platform, use unknown back off mechanism                                                
                rootdir = temproot.joinpath("pytest-of-unknown")
                rootdir.mkdir(mode=0o700, exist_ok=True)
            # Because we use exist_ok=True with a predictable name, make sure                                                                                  
            # we are the owners, to prevent any funny business (on unix, where                                                                                 
            # temproot is usually shared).                                                                                                                     
            # Also, to keep things private, fixup any world-readable temp                                                                                      
            # rootdir's permissions. Historically 0o755 was used, so we can't                                                                                  
            # just error out on this, at least for a while.                                                                                                    
            uid = get_user_id()
            if uid is not None:
                rootdir_stat = rootdir.stat()
                if rootdir_stat.st_uid != uid:
                    raise OSError(
                        f"The temporary directory {rootdir} is not owned by the current user. "
                        "Fix this and try again."
                    )

This is vulnerable to symlink attacks, and probably TOCTOU attacks (see e.g. https://owasp.org/www-community/vulnerabilities/Insecure_Temporary_File). The linux kernel has protections for some of these in the fs.protected_hardlinks and fs.protected_symlinks sysctls, but they violate POSIX and are not enabled by default (though most distros enable them). In any case, they are linux-only.

For a proof of concept, create a symlink as some other user from /tmp/pytest-of-youruser to a directory you own (this will require fs.protected_symlinks to be disabled). Run pytest as yourself, and it will start writing junk to the directory that the attacker chose. It will also change the permissions on the directory he chose.

Obviously whoever wrote this was aware of these issues, but maybe not of how hard they can be to fix completely. This can be made a little bit better by telling stat() not to follow symlinks, but I would instead recommend getting a random base directory in a secure way (using the python tempdir functions) rather than using a predictable name.
There are just too many ways it can go wrong.

[RonnyPfannschmidt]

The predictable base is a intentional feature

Thanks for the heads up on the feature

[orlitzky]

If the predictable base is just for convenience (to make them easy to find/identify), it might be acceptable to pass it as a prefix= to the python tempdir functions. That would result in a directory like pytest-for-<user>.XXXXXX, where XXXXXX is a random number.

[RonnyPfannschmidt]

we use the pytest-for-$USER as base-tmp for more, have to decide what to do with it as its practically a breaking change in ux

my initial impression is that id prefer to harden obtaining that directory - but i'm unfamiliar with the attack surface atm

[orlitzky]

Looking at the list of weird things that mkstemp() does is a good place to start, but it really depends on how pedantic you want to be. The fundamental issue is that it's not safe to do anything in a directory that is writable by someone you don't trust completely. Usually that's only yourself and root. Right away this raises some questions:

• Do we assume that if PYTEST_DEBUG_TEMPROOT is taken from the environment, that it's completely safe (i.e. that it meets those writability requirements)?
• If we use the default TMPDIR on UNIX, what assumptions do we want to make on its permissions? Usually /tmp is world writable, which is already bad, but maybe e.g. some other users are allowed to bind-mount directories there?

In the "normal" case where we're using /tmp and it's world-writable but there's no other funny business, you have symlinks, hardlinks, and TOCTOU to worry about. Symlinks can be avoided with lstat(), but there's a risk that the path you're lstat'ing will be replaced between the time that you lstat it and the time that you use it. There's also a chance that some directory higher up is a symlink; lstat() only checks the very last path component. Hardlinks to directories aren't allowed on linux, but that's actually not part of the POSIX standard. If you want to be safe on systems where hardlinks to directories are allowed, you have all of the same issues you do with symlinks, but now with hardlinks. Actually it's worse because "is this path a hardlink?" is a meaningless question; you have to do a stat() and count the number of hardlinks, but that number can change behind your back -- there's no way to do it atomically and fail. (Bind mounts are basically hard links to directories in this context.)

You can make it very hard to exploit by doing a lot of checks, but it will never be 100% safe from every contrived scenario. Even using the python library functions to obtain a directory "securely" is probably vulnerable to edge cases. But the benefit of the latter would be that it is no longer your problem :)

[orlitzky]

Oh, and suppose we manage to make the permissions/ownership check bullet-proof. Someone else can create pytest-for-orlitzky when the machine reboots or when /tmp is cleaned. The next time I try to run pytest, it will raise an OSError telling me to fix the ownership issue. But I can't fix it! If I could, the permissions check wouldn't be worth anything.

[RonnyPfannschmidt]

Let's keep in mind that pytest is not commonly a service on a multi user system but rather a test tool you run in isolated ci and/or from the primary user

We don't have the same attack exposure as a unix service on a multi user system

[RonnyPfannschmidt]

That being said we should manage the attack surface and/or ensure user are aware of hardening steps to take