TOR-005 pen-torproject#5: sbws - Arbitrary File read/write via symlinks due to Time-of-check-to-time-of-use

Technical Description

The cleanup command compresses all files in the datadir with the file extension .txt. For this purpose, the generator function _get_files_mtime_older_than is defined, which returns the filename of files that are not older than a specific time. Finally, the _compress_files function is invoked to compress these files.

Within the _compress_files function, each file is iterated in the folder, filtered by the function _get_files_mtime_older_than. Then, the file is opened and copied into a gzip archive. However, there are two different vulnerabilities hidden in this code.

Attackers can create a symlink with the name of the archive before the call with gzip.open(out_fname, "wt") which causes the symlink's target to be overwritten with the contents of the gzip file leading to an arbitrary file write. In the second case, attackers can bypass symlink protection in the _get_files_mtime_older_than function. This function calls the os.stat function with the parameter follow_symlinks=False. However, this can be ignored since only the file name and nothing else is returned after calling os.stat(Time-of-check). After this call, attackers replace the filenames with a symlink immediately after (Time-of-use). As a result, the content of the symlink target will be written to the gzip archive, leading to an arbitrary file read. sbws/core/cleanup.py

def _clean_result_files(args, conf):
    datadir = conf.getpath("paths", "datadir")
    [...]
    files_to_compress = _get_files_mtime_older_than(
        datadir, compress_after_days, [".txt"]
    )
    _compress_files(datadir, files_to_compress, dry_run=args.dry_run)

def _get_files_mtime_older_than(dname, days_delta, extensions):
    [...]
    for root, dirs, files in os.walk(dname):
        for f in files:          
            # using file modification time instead of parsing the name
            # of the file.
            filedt = unixts_to_dt_obj(
                os.stat(fname, follow_symlinks=False).st_mtime
            )
            if filedt < oldest_day:
                yield fname

def _compress_files(dname, files, dry_run=True):
    with DirectoryLock(dname):
        for fname in files:
            [...]
            with open(fname, "rt") as in_fd:
                out_fname = fname + ".gz"
                with gzip.open(out_fname, "wt") as out_fd:
                    shutil.copyfileobj(in_fd, out_fd)

Impact

A low-privileged user with write access to the datadir of the sbws user can read and write arbitrary files with symlinks if the cleanup command is invoked.

Recommendation

It is recommended to restrict access to the datadir to the sbws user only. Furthermore, it should be checked whether a file is not a symlink before acting as in the case of gzip.open. Moreover, the _get_files_mtime_older_than function should return file descriptors instead of filenames to prevent a Time-of-check-to-time-of-use attack.

Type

CWE-61: UNIX Symbolic Link (Symlink) Following

Update:

The commit verifying that the file is not a symlink needs to be revised. There is still a Time-of-check-to-time-of-use window between os.path.islink (time-of-check) and os.remove (time-of-use). However, os.remove only deletes the symlink and not the target of the symlink, causing a symlink attack to be useless.