netdev - Re: [PATCH iptables 3/3] libxt_hashlimit: iptables-restore does not work as expected with xt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <577D859F.2000003@akamai.com>
Date:	Wed, 6 Jul 2016 18:26:39 -0400
From:	Vishwanath Pai <vpai@...mai.com>
To:	Pablo Neira Ayuso <pablo@...filter.org>
Cc:	kaber@...sh.net, kadlec@...ckhole.kfki.hu,
	netfilter-devel@...r.kernel.org, coreteam@...filter.org,
	johunt@...mai.com, netdev@...r.kernel.org, pai.vishwain@...il.com
Subject: Re: [PATCH iptables 3/3] libxt_hashlimit: iptables-restore does not
 work as expected with xt_hashlimit

On 07/05/2016 04:13 PM, Vishwanath Pai wrote:
> On 06/25/2016 05:39 AM, Pablo Neira Ayuso wrote:
>> I see, but I'm not convinced about this /proc rename feature.
>>
>> I think the main point of this, as well as other entries in bugzilla
>> related to this, is ability to update an existing hashlimit state.
>>
>> So, I'm not proposing to rename --enhanced-procfs to something else,
>> I think that a different approach consisting on adding a new option
>> like --hashlimit-update that will update the internal state of an
>> existing hashlimit object is just fine for your usecase, right?
>>
>>>> Other than that, we are doing exactly what you said, but creating a new
>>>> entry in the hashtable instead of updating it. The previous entry will
>>>> automatically be removed when the old rule is flushed/deleted.
>> What I'm missing is why we need this /proc rename at all.
> 
> The reason we need the procfs rename feature is because it is broken at
> the moment. Let us assume someone adds two rules with the same name
> (intentionally or otherwise). We cannot prevent them from doing this or
> error out when someone does this because all of this is done in
> hashlimit_mt_check which is called for every iptables rule change, even
> an entirely different rule. I'll demonstrate two scenarios here. I have
> put debug printk statements which prints everytime hashlimit_mt_check is
> called.
> 
> 1) Add two rules with the same name but in different chains
> 
> $ iptables -A chain1 -m hashlimit --hashlimit-above 200/sec \
>   --hashlimit-mode srcip --hashlimit-name hashlimit1 -j DROP
> 
> $ dmesg -c
> [  103.965578] hashlimit_mt_check for rule hashlimit1
> 
> 
> $ iptables -A chain2 -m hashlimit --hashlimit-above 300/sec \
>    --hashlimit-mode srcip --hashlimit-name hashlimit1 -j DROP
> 
> $ dmesg -c
> [  114.613758] hashlimit_mt_check for rule hashlimit1
> [  114.621360] hashlimit_mt_check for rule hashlimit1
> [  114.627411] hashlimit_mt_destroy on hashlimit1
> 
> 2) Replace an iptables rule with iptables-restore
> 
> $ iptables -A chain1 -m hashlimit --hashlimit-above 200/sec \
>   --hashlimit-mode srcip --hashlimit-name hashlimit1 -j DROP
> 
> $ iptables-save > /tmp/hashlimit
> 
> $ vi /tmp/hashlimit (edit 200/s to 300/s)
> 
> $ iptables-restore < /tmp/hashlimit
> 
> $ dmesg -c
> [ 1585.411093] hashlimit_mt_check for rule hashlimit1
> [ 1585.418948] hashlimit_mt_destroy on hashlimit1
> 
> In case 1 there exists two rules with the same name but we cannot have
> procfs files for both of the rules since they have to exist in the same
> directory. In case 2 there will be only one rule but there is a small
> window where two rules with same name exist. We cannot differentiate
> this from case 1. In both the cases we get the call for
> hashlimit_mt_check for the new rule before the old rule is deleted.
> 
> Without the rename feature I do not know how to correctly handle the
> scenario where two rules with different parameters but the same name exist.
> 
> I believe the rest of the patch handles the --hashlimit-update feature
> you mentioned, but instead of updating an existing object it creates a
> new one and the old object is deleted by the call to destroy. The
> hashtable match function is modified to include all parameters of the
> object and not just the name so that we can reuse objects that have the
> exact same features.
> 

There is also another option of removing the procfs files altogether in
revision 2. The files were only used for debugging and any users relying
on the file being present will continue to see it as long as they are in
revision 1.

If we do need these files, we can create another option in rev2 which
explicitly enables procfs but leave them disabled by default. And we can
retain the existing behavior assuming whoever is using it knows its
caveats. Please suggest.

Thanks,
Vishwanath