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

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

[Alan Ross]

Hi netfilter team,

  iptables uses getenv() to read XTABLES_LIBDIR, IPTABLES_LIB_DIR,
  IP6TABLES_LIB_DIR, XTABLES_LOCKFILE, and EBTABLES_SAVE_COUNTER. Since
  iptables runs as root, these become local privilege escalation vectors:

   - XTABLES_LIBDIR controls where extensions are loaded via dlopen().
     A local attacker who can inject this variable forces iptables to
     load arbitrary shared libraries as root (CWE-426, CWE-427).

   - XTABLES_LOCKFILE controls where the lock file is created via
     open(path, O_CREAT, 0600). An attacker can create or clobber
     arbitrary files as root.

  This patch replaces getenv() with secure_getenv() for all 5 variables.
  secure_getenv() returns NULL when AT_SECURE is set by the kernel (for
  setuid, setgid, or capability-elevated binaries), blocking env-var
  injection without affecting normal unprivileged usage.

  A portability shim is included for glibc < 2.17. A test program is
  included at tests/test-secure-getenv.c.

  Patch and full details:
  https://github.com/SleuthCo/iptables/compare/master...security/fix-env-var-privilege-escalation

  Signed-off-by: Alan <alan@sleuthco.ai>

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

[Florian Westphal]

Alan Ross <alan@sleuthco.ai> wrote:
> Hi netfilter team,
> 
>   iptables uses getenv() to read XTABLES_LIBDIR, IPTABLES_LIB_DIR,
>   IP6TABLES_LIB_DIR, XTABLES_LOCKFILE, and EBTABLES_SAVE_COUNTER. Since
>   iptables runs as root, these become local privilege escalation vectors:

If someone can set up your environment they can also set up
LD_PRELOAD and PATH.

>   This patch replaces getenv() with secure_getenv() for all 5 variables.
>   secure_getenv() returns NULL when AT_SECURE is set by the kernel (for
>   setuid, setgid, or capability-elevated binaries), blocking env-var
>   injection without affecting normal unprivileged usage.

iptables requires CAP_NET_ADMIN to work and it was never designed to work
with setuid-to-root.

What kind of scenario/setup needs this patch?

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

[Florian Westphal]

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.

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

[Florian Westphal]

Alan Ross <alan@sleuthco.ai> wrote:
> >> Would you have the cycles to go through all of nf software to make this
> change?
> 
>   Yes, happy to take this on. I'll work through them in order: nftables,
> ipset, conntrack-tools, ulogd. iproute2 I can
>   look at as well, though that's a separate tree/maintainer so I'd send
> those separately.

Sure, iproute2 patches go to netdev@ , not to netfilter-devel.

>  >> I think it just needs a rework of the commit message
> 
>   Will do — v2 will lead with the setcap/container-runtime scenario as the
> motivation.

Thanks!

>  >> Any reason for the wrapper to not do getauxval(AT_SECURE)?
> 
>   No good reason. getauxval is available since glibc 2.16 (one release
> before secure_getenv in 2.17), and since this is
>   all Linux-only code there's no portability concern. Your version is
> cleaner — I'll use that for the fallback.

Alright.

>   In practice the #ifdef HAVE_SECURE_GETENV path will hit on anything
> remotely modern, but agreed the getauxval fallback
>    is simpler than a uid/euid comparison.

Agreed, it should not be hit in practice.

>  >> Another option is to alter ef7781eb1437a ("libxtables: exit if called
> by setuid executable") to enforce non-capability
>    binary
> 
>   That would work as a first step for xtables specifically — extend the
> existing getuid() != geteuid() check to also
>   bail on getauxval(AT_SECURE). The secure_getenv changes would then be
> belt-and-suspenders on top. Want me to include
>   both in v2, or would you prefer the enforcement-first approach across the
> nf suite?

I would prefer enforcement-first, but you are free to followup with
getenv_secure if you want.  I mean, the change isn't wrong, I am just
not sure there isn't anything else that we might be missing.

xtables has a plugin architecture, so we don't know what other
extensions that might get shipped by some distros do.

For nftables, thats less of a concern, BUT I don't know what some of the
libraries we link against do with untrusted input.  Or what will happen
when nft is invoked with stdin/stdout/stderr closed.

So I would prefer to enforce a no-setcap/setuid approach in any case
and followup with secure_getenv later.

> I'll start with the iptables v2 (reworked message + getauxval fallback)
> and then work through nftables and the others
> as follow-up series.

Thank you, much appreciated.