Re: [PATCH] security: use secure_getenv() to prevent env-var privilege escalation

[Florian Westphal <fw@strlen.de>]

Alan Ross <alan@sleuthco.ai> wrote:
>   The gap is when iptables is run with file capabilities rather than via
> sudo:
> 
>   setcap cap_net_admin+ep /usr/sbin/iptables
> 
>   In that case the kernel sets AT_SECURE, the linker correctly strips
> LD_PRELOAD, but getenv("XTABLES_LIBDIR") still
>   returns the attacker-controlled value and gets passed to dlopen().
> secure_getenv() closes that specific gap.
> 
> >>  iptables requires CAP_NET_ADMIN to work and it was never designed to
> work with setuid-to-root.
> 
>   Understood. The capability-elevated case above is the primary scenario —
> some container runtimes and minimal
>   distributions grant cap_net_admin via setcap rather than running through
> sudo, and that's where the env-controlled
>   dlopen() becomes reachable.

ARGH!

cd ~/git/iproute2
git grep getenv | wc -l
36

>   That said, I recognize this is defense-in-depth rather than a critical
> fix. secure_getenv() is a strict behavioral
>   superset of getenv() for unprivileged execution (returns the same value
> when euid==uid), so the patch has no impact on
>    normal usage. The precedent is util-linux (su, mount) and sudo, which
> made the same change for similar env-controlled
>    paths.
>
>   If the consensus is that capability-elevated iptables is not a supported
> configuration, I understand. Happy to drop
>   the patch or adjust scope.

If there are distros that are dumb enough to setuid-to-0/setcap random
binaries then we should cope with this.

Would you have the cycles to go through all of nf software to make this
change?  nftables, ipet, conntrack, ulogd etc would all need this
change.  And non-netfilter software too, iproute2 tool has 36 getenv
calls.

As for this patch, I think it just needs a rework of the commit message
to explain that this is about existing distros/containers that setcap the
binary.

Any reason for the wrapper to not do

static inline const char *secure_getenv(const char *name)
{
        unsigned long x = getauxval(AT_SECURE);

	return x == 0 ? getenv(name) : NULL
}

?

It probably doesn't matter too much given glibc 2.17 is ancient, but
still, I'm curious.

Another option is to alter ef7781eb1437a2d6fd37eb3567c599e3ea682b96
("libxtables: exit if called by setuid executeable")
to enforce non-capability binary and then followup in nftables and
others.