mrash / fwknop

Single Packet Authorization > Port Knocking
http://www.cipherdyne.org/fwknop/
GNU General Public License v2.0
1.12k stars 232 forks source link

Expected expire time in check_firewall_rules() becomes out of sync with active rules #270

Open tofurky opened 6 years ago

tofurky commented 6 years ago

Using nfq/iptables with fwknopd 2.6.9 (I also tested git master) on Debian 9.0, I noticed an odd pattern in some Munin graphs regarding the number of fork()'s per second on one of my servers. I eventually traced it to fwknopd.

The cause is that check_firewall_rules() was executing the iptables check with an interval based upon an already-expired rule, not based on remaining active rules. This leads to iptables being executed every CONF_NFQ_LOOP_SLEEP (twice per second) until the longer-lived rule expires.

I've come up with a simple, repeatable sequence that causes the issue:

1.) Send valid SPA with a long expiration - 65535 seconds. 2.) Send valid SPA with a short expiration - 1 second.

It doesn't matter if the smaller of the two expirations is 1s, or 300s, as long as it expires before the first. After the shorter rule expires, iptables starts to be executed twice per second.

This, while still functional, seems inefficient and perhaps unintended. I'm not sure what the best way to go about fixing this would be - perhaps recalculating the iptables check interval (ch[i].next_expire ?) at the same time a rule expires and is removed.

Here is the code which is improperly calculating the expected expire time:

https://github.com/mrash/fwknop/blob/master/server/fw_util_iptables.c#L1832-L1867