linux-kernel - Re: [RFC/PATCH 2/3] netfilter: ctnetlink: Fix regression in CTA

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]

Date:   Wed, 18 Jan 2017 11:02:10 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Kevin Cernekee <cernekee@...omium.org>
Cc:     pablo@...filter.org, netfilter-devel@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [RFC/PATCH 2/3] netfilter: ctnetlink: Fix regression in
 CTA_STATUS processing

Hi,

On Mon, Jan 16, 2017 at 9:14 PM, Kevin Cernekee <cernekee@...omium.org> wrote:
> The libnetfilter_conntrack userland library always sets IPS_CONFIRMED
> when building a CTA_STATUS attribute.  If this toggles the bit from
> 0->1, Linux 4.4+ will reject it and this will cause any NFQA_EXP
> attribute in the packet to be ignored.  This breaks conntrackd's
> userland helpers because they operate on unconfirmed connections.

I prefer the wording that you changed it to between this patch and the
next one on the Chromium OS review system:

https://chromium-review.googlesource.com/c/428493/1..2//COMMIT_MSG

It makes it more obvious that the function has always considered some
of these things to be "errors" but that the errors simply affect
things so much in the past (before commit b7bd1809e078 ("netfilter:
nfnetlink_queue: get rid of nfnetlink_queue_ct.c")).

> Instead of returning -EBUSY if the user program asks to modify an
> unchangeable bit, simply ignore the change.
>
> Also, fix the logic so that user programs are allowed to clear
> the bits that they are allowed to change.

This is a general bugfix to the code.  If folks are using netfilter
then it seems like this ought to be backported even before 4.4.

> Signed-off-by: Kevin Cernekee <cernekee@...omium.org>
> ---
>  include/uapi/linux/netfilter/nf_conntrack_common.h |  4 ++++
>  net/netfilter/nf_conntrack_netlink.c               | 10 ++++------
>  2 files changed, 8 insertions(+), 6 deletions(-)

This bugfix is fairly easy to understand.  Assuming you agree that
it's OK to silently ignore these errors (and behave more like things
worked before kernel 4.4), then it's a clear win.

Unless someone has a better suggestion, I'd say:

Reviewed-by: Douglas Anderson <dianders@...omium.org>