diff mbox

.config for iptables icmp rule delete failure

Message ID 20170508202236.GC9660@breakpoint.cc
State Not Applicable
Delegated to: Pablo Neira
Headers show

Commit Message

Florian Westphal May 8, 2017, 8:22 p.m. UTC 
Richard Guy Briggs <rgb@redhat.com> wrote:

[ CC'ing Willem ]

> (Summary of IRC conversation for background...)
> Paul Moore and I hit what appears to be a bug since f25's 4.10.11 and
> upstream's 4.11-rc3 that would fail to locate on deletion an icmp rule
> in iptables.  Paul narrowed it down to the icmp option.
> Here's our issue where it came up: 
> 	https://github.com/linux-audit/audit-testsuite/pull/43#issuecomment-296831880
> 
> The test case is:
> # iptables -t mangle -I INPUT -i lo -p icmp --icmp-type 1 -j MARK --set-mark 0xdeadbeef
> # iptables -t mangle -D INPUT -i lo -p icmp --icmp-type 1 -j MARK --set-mark 0xdeadbeef
> The error we're getting is "iptables: No chain/target/match by that name."

This is a iptables (yes, userspace) bug exposed with
f77bc5b23fb1af51fc0faa8a479dea8969eb5079
(iptables: use match, target and data copy_to_user helpers)

icmp is 4 byte in size, but for some silly(?) reason userspace size
in iptables userland is set as XT_ALIGN(sizeof(ipt_icmp), so userspace
thinks its 8.

In 4.10, kernel copied the full kernel blob to userspace, and since
its allocated with kz/vzalloc the 4 padding bytes are 0.

libiptc uses malloc, so in case that contains garbage bytes the
memcmp() used to figure out if we found the correct rule in libiptc
during -D mode returns false because it chokes on the extra padding
after struct ipt_icmp match :-/

Simples fix is to use calloc/memset to 0 in libiptc, but we can't
go with userspace-only fix ...

So we have to fix this in the kernel and have xt_data_to_user()
zero out any padding as well.

Willem, if you don't have time to fix this let me know and i'll
try to work on this tomorrow.

For testing, this iptables patch makes things barf reliably regardless what
malloc/libc is doing:

iptables -A INPUT -p icmp --icmp-type 1 &&
iptables -D INPUT -p icmp --icmp-type 1

Works on 4.10, but not >= 4.11 (assumes x86_64 kernel+userland).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Willem de Bruijn May 9, 2017, 2:22 a.m. UTC | #1 
On Mon, May 8, 2017 at 4:22 PM, Florian Westphal <fw@strlen.de> wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
>
> [ CC'ing Willem ]
>
>> (Summary of IRC conversation for background...)
>> Paul Moore and I hit what appears to be a bug since f25's 4.10.11 and
>> upstream's 4.11-rc3 that would fail to locate on deletion an icmp rule
>> in iptables.  Paul narrowed it down to the icmp option.
>> Here's our issue where it came up:
>>       https://github.com/linux-audit/audit-testsuite/pull/43#issuecomment-296831880
>>
>> The test case is:
>> # iptables -t mangle -I INPUT -i lo -p icmp --icmp-type 1 -j MARK --set-mark 0xdeadbeef
>> # iptables -t mangle -D INPUT -i lo -p icmp --icmp-type 1 -j MARK --set-mark 0xdeadbeef
>> The error we're getting is "iptables: No chain/target/match by that name."
>
> This is a iptables (yes, userspace) bug exposed with
> f77bc5b23fb1af51fc0faa8a479dea8969eb5079
> (iptables: use match, target and data copy_to_user helpers)
>
> icmp is 4 byte in size, but for some silly(?) reason userspace size
> in iptables userland is set as XT_ALIGN(sizeof(ipt_icmp), so userspace
> thinks its 8.
>
> In 4.10, kernel copied the full kernel blob to userspace, and since
> its allocated with kz/vzalloc the 4 padding bytes are 0.
>
> libiptc uses malloc, so in case that contains garbage bytes the
> memcmp() used to figure out if we found the correct rule in libiptc
> during -D mode returns false because it chokes on the extra padding
> after struct ipt_icmp match :-/
>
> Simples fix is to use calloc/memset to 0 in libiptc, but we can't
> go with userspace-only fix ...
>
> So we have to fix this in the kernel and have xt_data_to_user()
> zero out any padding as well.
>
> Willem, if you don't have time to fix this let me know and i'll
> try to work on this tomorrow.

Thanks, Florian. Also for the detailed context. I'm having a look.
The following might be sufficient. It fixes the given example for me.

@@ -288,6 +288,10 @@ int xt_data_to_user(void __user *dst, const void *src,
        usersize = usersize ? : size;
        if (copy_to_user(dst, src, usersize))
                return -EFAULT;
+       size = XT_ALIGN(size);
        if (usersize != size && clear_user(dst + usersize, size - usersize))
                return -EFAULT;
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal May 9, 2017, 2:32 a.m. UTC | #2 
Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:

> Thanks, Florian. Also for the detailed context. I'm having a look.
> The following might be sufficient. It fixes the given example for me.
> 
> @@ -288,6 +288,10 @@ int xt_data_to_user(void __user *dst, const void *src,
>         usersize = usersize ? : size;
>         if (copy_to_user(dst, src, usersize))
>                 return -EFAULT;
> +       size = XT_ALIGN(size);
>         if (usersize != size && clear_user(dst + usersize, size - usersize))
>                 return -EFAULT;

Looks good, but I think this needs a tweak for compat case (use of
COMPAT_XT_ALIGN()).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Willem de Bruijn May 9, 2017, 4:18 a.m. UTC | #3 
On Mon, May 8, 2017 at 10:22 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Mon, May 8, 2017 at 4:22 PM, Florian Westphal <fw@strlen.de> wrote:
>> Richard Guy Briggs <rgb@redhat.com> wrote:
>>
>> [ CC'ing Willem ]
>>
>>> (Summary of IRC conversation for background...)
>>> Paul Moore and I hit what appears to be a bug since f25's 4.10.11 and
>>> upstream's 4.11-rc3 that would fail to locate on deletion an icmp rule
>>> in iptables.  Paul narrowed it down to the icmp option.
>>> Here's our issue where it came up:
>>>       https://github.com/linux-audit/audit-testsuite/pull/43#issuecomment-296831880
>>>
>>> The test case is:
>>> # iptables -t mangle -I INPUT -i lo -p icmp --icmp-type 1 -j MARK --set-mark 0xdeadbeef
>>> # iptables -t mangle -D INPUT -i lo -p icmp --icmp-type 1 -j MARK --set-mark 0xdeadbeef
>>> The error we're getting is "iptables: No chain/target/match by that name."
>>
>> This is a iptables (yes, userspace) bug exposed with
>> f77bc5b23fb1af51fc0faa8a479dea8969eb5079
>> (iptables: use match, target and data copy_to_user helpers)
>>
>> icmp is 4 byte in size, but for some silly(?) reason userspace size
>> in iptables userland is set as XT_ALIGN(sizeof(ipt_icmp), so userspace
>> thinks its 8.
>>
>> In 4.10, kernel copied the full kernel blob to userspace, and since
>> its allocated with kz/vzalloc the 4 padding bytes are 0.
>>
>> libiptc uses malloc, so in case that contains garbage bytes the
>> memcmp() used to figure out if we found the correct rule in libiptc
>> during -D mode returns false because it chokes on the extra padding
>> after struct ipt_icmp match :-/
>>
>> Simples fix is to use calloc/memset to 0 in libiptc, but we can't
>> go with userspace-only fix ...
>>
>> So we have to fix this in the kernel and have xt_data_to_user()
>> zero out any padding as well.
>>
>> Willem, if you don't have time to fix this let me know and i'll
>> try to work on this tomorrow.
>
> Thanks, Florian. Also for the detailed context. I'm having a look.
> The following might be sufficient. It fixes the given example for me.
>
> @@ -288,6 +288,10 @@ int xt_data_to_user(void __user *dst, const void *src,
>         usersize = usersize ? : size;
>         if (copy_to_user(dst, src, usersize))
>                 return -EFAULT;
> +       size = XT_ALIGN(size);
>         if (usersize != size && clear_user(dst + usersize, size - usersize))
>                 return -EFAULT;

This works for me on 64-bit, where __alignof__(struct _xt_align) is 8.
In a 32-bit environment it is 4. I will need to revise it to work correctly
for compat. See also COMPAT_XT_ALIGN.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 2c66d04..9497a5e 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -1286,6 +1286,7 @@  alloc_handle(const char *tablename, unsigned int size, unsigned int num_rules)
        if (!h->entries)
                goto out_free_handle;
 
+       memset(h->entries, 0x42, sizeof(STRUCT_GET_ENTRIES) + size);
        strcpy(h->entries->name, tablename);
        h->entries->size = size;