(Bugfix) lib/btrfs.c: Remove hardcoded Paths and increase portability

[silverhadch]

This Bugfixes NixOS/nixpkgs#483407 and increases portability in general and removes an unclean hack.

[Conan-Kudo]

Is this safe? We should be pulling the binary via PATH rather than using env, and there might be a good reason for the path lockdown on the binaries...

[silverhadch]

Is this safe? We should be pulling the binary via PATH rather than using env, and there might be a good reason for the path lockdown on the binaries...

You need root priviledges in the first place to even get to this Code Path tho?

[silverhadch]

Is this safe? We should be pulling the binary via PATH rather than using env, and there might be a good reason for the path lockdown on the binaries...

The env source code just does
execvp (program, &argv[optind]);
eitherway. Its like asking PATH without doing the reading yourself. And if somebody comprimises your Systems /usr/bin coreutils binaries then you have a way bigger problem on hand then calling btrfs progs binarys via env.
And also cleans up the enviroment to make PATH Trickery harder with

if (ignore_environment)
 environ = dummy_environ;

So I would argue it might actually be safer then to iterate through a list of hardcoded Paths. Imagine an Attacker places a fake binary into one of the Path places before the actual real btrfs binary was iterated through. env basically does the same logic but more predictable and in some scenarios even safer.

[alejandro-colomar]

I've done a light review of the C code (mostly, style), but I prefer if @hallyn reviews the core idea.

[hallyn]

My main concern: /usr/bin/env will use PATH. glibc will sanitize PATH for setuid-root binaries, but musl will not.

Google/gemini tells me the right way to handle this in nix would be using a nix derivation to set the path, and rebuild every time btrfs is upgraded.

I don't think this patch is safe, especially in the nix world.

[silverhadch]

My main concern: /usr/bin/env will use PATH. glibc will sanitize PATH for setuid-root binaries, but musl will not.

Google/gemini tells me the right way to handle this in nix would be using a nix derivation to set the path, and rebuild every time btrfs is upgraded.

I don't think this patch is safe, especially in the nix world.

The idea that /usr/bin/env is inherently “unsafe” is mostly a misunderstanding of what it actually does and where the real trust boundary lies. env does not introduce any new behavior — it simply calls execvp(), which performs a PATH lookup and then execve(). Using /usr/bin/env foo is therefore functionally equivalent to invoking foo via execvp() in C. If PATH is compromised, that problem already exists regardless of whether env is involved.

On NixOS in particular, /usr/bin/env exists precisely because the system is not FHS-compliant. It provides a minimal compatibility layer so upstream software does not need to be rewritten to hardcode store paths. Replacing this with hardcoded paths in a derivation means patching and rebuilding every time a dependency changes, which quickly becomes a nightmare.

There is also an important practical issue here: shadow on NixOS is not actively updated, and currently lags behind upstream. If we rely on substituting paths via the nix macros, we risk pulling in older, hardcoded versions of tools like btrfs-progs. Using outdated filesystem tooling is not just inconvenient — it’s actively dangerous. Filesystem utilities are tightly coupled to kernel and on-disk format behavior, and running old userspace against newer kernels is a well-known source of corruption and data loss.

It’s also important to distinguish between setuid binaries and binaries that are simply executed as root. useradd, which is the only binary consuming this function, is not setuid. In that case there is no privilege transition happening at exec time, so PATH lookup does not introduce a new security boundary and only roots enviroment matters. If an attacker can influence root’s PATH, then they can just as easily place a fake sudo, useradd, or btrfs binary earlier in PATH. At that point the system is already compromised.

On NixOS, PATH is explicitly constructed and /usr/bin is mostly empty, with /usr/bin/env existing specifically to bridge non-FHS software. Using env here does not weaken security; it improves portability and avoids fragile downstream patching.

In short: there is no meaningful security gain in hardcoding paths here, but there is a very real maintenance and safety cost. If roots PATH is attacker-controlled, the system is already lost — removing /usr/bin/env does not change that.

[stoeckmann]

It’s also important to distinguish between setuid binaries and binaries that are simply executed as root. useradd, which is the only binary consuming this function, is not setuid.

There is also userdel next to useradd which reaches that function.

Also, keep in mind that these tools can be setuid through ./configure --enable-account-tools-setuid.

[silverhadch]

It’s also important to distinguish between setuid binaries and binaries that are simply executed as root. useradd, which is the only binary consuming this function, is not setuid.

There is also userdel next to useradd which reaches that function.

Also, keep in mind that these tools can be setuid through ./configure --enable-account-tools-setuid.

Its off by default and there is probably not even a single distro turning on setuid on porpuse if not needed tho?

[stoeckmann]

Its off by default and there is probably not even a single distro turning on setuid on porpuse if not needed tho?

I hope so, but as long as shadow offers this configure option (and I would like to see it gone), these programs can be setuid and should be treated as setuid binaries.

[silverhadch]

Its off by default and there is probably not even a single distro turning on setuid on porpuse if not needed tho?

I hope so, but as long as shadow offers this configure option (and I would like to see it gone), these programs can be setuid and should be treated as setuid binaries.

The fact that an suid binary can call btrfs commands in the first place is concerning. The option should defenitly be removed.

[alejandro-colomar]

Its off by default and there is probably not even a single distro turning on setuid on porpuse if not needed tho?

I hope so, but as long as shadow offers this configure option (and I would like to see it gone)

Feel free to send a patch removing it. :)

[silverhadch]

Its off by default and there is probably not even a single distro turning on setuid on porpuse if not needed tho?

I hope so, but as long as shadow offers this configure option (and I would like to see it gone)

Feel free to send a patch removing it. :)

I could do it.

[alejandro-colomar]

Thanks! The style is good; now it's only a matter of whether this is safe or not. A patch for removing --enable-account-tools-setuid might help.

[silverhadch]

Thanks! The style is good; now it's only a matter of whether this is safe or not. A patch for removing --enable-account-tools-setuid might help.

Opened now.

[stoeckmann]

* Replace `execv` calls with `execl` if the path is absolute anyway. This makes it very obvious that `PATH` won't be used. This includes replacing `run_command` in `lib/nscd.c` and `lib/sssd.c` with an `execl` version

@silverhadch, do these absolute calls in lib/nscd.c and lib/sssd.c even work for NixOS? They are hardcoded to /usr/sbin/nscd and /usr/sbin/sss_cache.

[silverhadch]

* Replace `execv` calls with `execl` if the path is absolute anyway. This makes it very obvious that `PATH` won't be used. This includes replacing `run_command` in `lib/nscd.c` and `lib/sssd.c` with an `execl` version

@silverhadch, do these absolute calls in lib/nscd.c and lib/sssd.c even work for NixOS? They are hardcoded to /usr/sbin/nscd and /usr/sbin/sss_cache.

They are replaced with the Nix Macros.

[stoeckmann]

Wouldn't it be a sane and simple solution to strip this PATH handling in lib/btrfs.c entirely and have a hard-coded path as in lib/nscd.c or lib/sssd.c? Even better, replace these constant strings with preprocessor definitions in all these files, e.g. BTRFS_PROG, NSCD_PROG, SSS_CACHE_PROG which are only set if not already defined.

Then, this very issue could be resolved by NixOS by placing a custom shell script into the installation directory of shadow and configure shadow with BTRFS_PROG=/path/to/script, which does nothing more than calling the real btrfs location with $@ arguments.

Would that be an acceptable approach?

[silverhadch]

Wouldn't it be a sane and simple solution to strip this PATH handling in lib/btrfs.c entirely and have a hard-coded path as in lib/nscd.c or lib/sssd.c? Even better, replace these constant strings with preprocessor definitions in all these files, e.g. BTRFS_PROG, NSCD_PROG, SSS_CACHE_PROG which are only set if not already defined.

Then, this very issue could be resolved by NixOS by placing a custom shell script into the installation directory of shadow and configure shadow with BTRFS_PROG=/path/to/script, which does nothing more than calling the real btrfs location with $@ arguments.

Would that be an acceptable approach?

Yesnt. If every Distro where to define their options then yes but currently its a mix of basically supporting FHS only Distros. If the appraoch were to change to like sssd and nscd then Its a simple nix macro and for other distros in their package build a variable. But currently it tries to be portable across everything. So this PR when useradd and userdel no longer are capable of being setuid is and we want to go the work everywhere route or go the nscd route and tell distros about it. Or maybe both? Have an set Path Macro and if empty try the looping through the hard coded path and as a last resort use env.

[hallyn]

* Replace `execv` calls with `execl` if the path is absolute anyway. This makes it very obvious that `PATH` won't be used. This includes replacing `run_command` in `lib/nscd.c` and `lib/sssd.c` with an `execl` version

@silverhadch, do these absolute calls in lib/nscd.c and lib/sssd.c even work for NixOS? They are hardcoded to /usr/sbin/nscd and /usr/sbin/sss_cache.

They are replaced with the Nix Macros.

Where is that done? In a nix-specific patch?

I do think some autoconfigure magic which sets an array of possible
paths to try (only one in the case of nscd.c and sssd.c, but more for
btrfs, for the existing case), which is set to the nix path for nix,
with a small helper function to walk that array of paths to find
one that exists and works, would be good, and might let nix drop a
patch.

Dropping setuid does remove my main objection to using env.

[silverhadch]

* Replace `execv` calls with `execl` if the path is absolute anyway. This makes it very obvious that `PATH` won't be used. This includes replacing `run_command` in `lib/nscd.c` and `lib/sssd.c` with an `execl` version

@silverhadch, do these absolute calls in lib/nscd.c and lib/sssd.c even work for NixOS? They are hardcoded to /usr/sbin/nscd and /usr/sbin/sss_cache.

They are replaced with the Nix Macros.

Where is that done? In a nix-specific patch?

I do think some autoconfigure magic which sets an array of possible
paths to try (only one in the case of nscd.c and sssd.c, but more for
btrfs, for the existing case), which is set to the nix path for nix,
with a small helper function to walk that array of paths to find
one that exists and works, would be good, and might let nix drop a
patch.

Dropping setuid does remove my main objection to using env.

Yes — the Nix package does a string replacement on the path and injects a pkg macro that resolves to the current Nix store path of the tools, adding them as a dependency. The problem is that here we don’t have a single path, but a list of paths.

In theory, we could just add /run/current-system/sw/bin/btrfs to the list and it would work on Nix, but that’s incredibly unclean. Instead, I wanted to go for the cleanest solution that works across all distros, including NixOS, by resolving the binary via the environment (i.e. using $PATH).

If you resolve via $PATH in a normal binary, only the user environment matters — and since root access is required anyway, that’s fine. If the root environment is compromised, things are already broken, so there’s no additional security downside compared to hardcoding paths.

However, if the binary is setuid, then the user’s environment gets elevated instead of root’s, which is a very bad idea.

So the options are basically:

1. Hardcode paths (nscd-style), which is ugly but safe.

2. Add /run/... for NixOS, making the code even messier.

3. Or drop setuid support for the binaries using this function, in which case the clean $PATH-based approach becomes just as safe as the hardcoded one.

That last option is the cleanest overall.

[silverhadch]

@alejandro-colomar @hallyn
Since setuid for useradd and userdel are gone, are there any blockers left for the env approach?
I want to write a NixOS Module for the new Option soon so I can enable it on my machine declaratively.

[alejandro-colomar]

@alejandro-colomar @hallyn Since setuid for useradd and userdel are gone, are there any blockers left for the env approach? I want to write a NixOS Module for the new Option soon so I can enable it on my machine declaratively.

This is a bit scary to me, so I prefer if @hallyn reviews this.

I don't see anything obviously wrong, FWIW.