[Pkg-netfilter-devel] Bug#816087: iptables is racy by default when used in scripts

[Ron]

On Sat, Sep 24, 2016 at 04:12:57PM +0100, Ben Hutchings wrote:
> Control: tag -1 upstream
> 
> On Sat, 27 Feb 2016 21:06:57 +1030 Ron <ron at debian.org> wrote:
> > Package: iptables
> > Version: 1.4.21-2+b1
> > Severity: important
> > 
> > Hi,
> > 
> > So, somewhere between Wheezy and Jessie, iptables starting using locking
> > to avoid racy updates to the kernel state, which means the command line
> > tool will now sometimes fail with:
> > 
> >  "Another app is currently holding the xtables lock."
> > 
> > Which is good, except that means the command line tool itself has now
> > introduced race conditions which scripts calling it repeatedly can lose
> > unless they explicitly pass the -w option to wait for the lock.
> > 
> > The problem seems to be that iptables itself will return before the
> > xtables lock has been released, so a script calling it multiple times
> > is prone to fail somewhere in the middle of what it is doing ...
> 
> This sounds hard to believe as the kernel should remove the lock
> synchronously as the process exits.  If it doesn't then I suppose
> there's a kernel bug here as well. :-/

Yes, that part of it was surprising to me as well (and mostly why I
filed the bug, since I understand the original problem that people
were trying to fix with this change).

It's been a while since I looked at this, but I'm pretty sure that
I was able to reproduce it by running the failing script manually
from the command line (which is how I saw the error message reported
and confirmed that adding -w did fix it) - and that was just a fairly
trivial shell script which repeatedly called iptables sequentially
to set up the firewall rules.

So unless something got triggered elsewhere which also took that
lock separately, nothing should have been running in parallel at
the time.  [ed. Oh ...  except ...  see below ...]


> However, it's entirely possible that multiple processes may run
> iptables in parallel at boot time; there's an example given here:
> https://utcc.utoronto.ca/~cks/space/blog/linux/IptablesUseWOption
> 
> [...] 
> > I'm inclined to think -w should actually be the default,
> 
> I agree with this.
> 
> [...]
> > But I do think this needs to be
> > either far more widely advertised as an incompatible and dangerous
> > change, or mitigated in some better way that doesn't make things
> > which were working in Wheezy gain a new and subtle failure mode
> > for Jessie and later releases.
> 
> A race condition already existed, and the new behaviour (exit with
> error message) seems better than the previous behaviour (quiet
> overwrite), not a regression in iptables.

Indeed, and I assume the parallel boot case was how the original race
was encountered.  It's obviously important to fix that, but I thought
what I'd hit here looked like a separate bug to that ...

I didn't dig deeper to see if it was an implementation bug, or if
there was some Good Reason that couldn't be completely synchronous
with the exit of the iptables process.  It was kind of a "rush to
fix newly broken firewalls asap" thing, which I squeezed firing off
a report about it into somewhere.


That said though, I think I need to have another look at exactly
_where_ this was failing in the script ...  since looking at it
again now, I can see one place where calls to iptables may have
sometimes occurred in parallel ...

There's one place where the output of iptables -L is piped into
a loop that calls iptables to flush the existing chains before
applying the new rules.  If that's really the (only) place where
this was failing, then this probably is just the parallel call
problem, and the 'race' I saw was whether the input to the pipe
exited before the first command in the loop tried to take the
lock ...

If that's true, then the world all makes proper sense again.
I'll try to confirm that one way or the other soon and follow
up here with the results.


> However, the switch to systemd as default init system in jessie
> presumably makes the race condition more likely, so that there *is* a
> regression in Debian GNU/Linux.  That makes it more important to fix
> the race properly, i.e. iptables should wait by default.  That also
> means there needs to be a way to disable waiting, which would need to
> be discussed upstream first.

I would have thought that waiting by default would probably be the
more 'backward compatible' option - though that said, if I'm correct
about the problem I saw in the script above, then defaulting to
waiting could potentially deadlock (empirically it hasn't so far with
-w added manually, but if both -L and -F take the same lock, then that
loop actually needs to be rewritten to be properly safe now ...)


So maybe the current behaviour _is_ the 'safest' change, and really
the only way to fix this is to audit everything calling iptables now :/
Either option seems to have its own flavour of eww ...

  Ron