netdev - Re: [PATCH v2 0/5] strict netlink validation

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] [day] [month] [year] [list]

Message-ID: <f1bd62ff-1013-d6ab-adfa-8af442ede5d5@cumulusnetworks.com>
Date:   Sun, 28 Apr 2019 17:11:26 -0600
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org
Cc:     Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [PATCH v2 0/5] strict netlink validation

On 4/28/19 1:32 PM, Johannes Berg wrote:
> On Fri, 2019-04-26 at 20:28 -0600, David Ahern wrote:
>>
>> I agree with this set and will help moving forward. As I recall it
>> requires follow up patches for each policy to set strict_start_type
>> opting in to the strict checking. With that in place new userspace on
>> old kernels will get a failure trying to configure a feature the old
>> kernel does not recognize.
> 
> Actually, that part you had already handled with nla_parse_strict() (now
> nla_parse_strict_deprecated()) - and I'm not sure we can make this even
> stricter because existing code might be setting future attributes
> already, expecting them to be ignored. This is already fishy of
> applications to expect though, but I'm not sure we really can change
> that? I don't think I'm actually changing something here, but I'm
> certainly open to suggestions - after all, when we actually do get
> around to adding that future attribute it almost certainly will have a
> different type than a (buggy) application would be using now.
> 
> However, what I did already do is that adding strict_start_type to
> policies means that all future attributes added to those policies will
> be handled in a strict fashion, i.e. if you add strict_start_type and
> then add a new u32 attribute >= strict_start_type, that new u32
> attribute will not be permitted to have a size other than 4 octets.
> 

For routes, an unknown attribute should cause the NEW/DEL command to
fail. There is a rare exception - something like RTA_PROTOCOL which is
just a passthrough, but in general the attribute is an important
modifier for the route.

With this change:

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index b298255f6fdb..7325c0265c5b 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -645,6 +645,7 @@ int ip_rt_ioctl(struct net *net, unsigned int cmd,
struct rtentry *rt)
 }

 const struct nla_policy rtm_ipv4_policy[RTA_MAX + 1] = {
+       [RTA_UNSPEC]            = { .strict_start_type = RTA_DPORT + 1 },
        [RTA_DST]               = { .type = NLA_U32 },
        [RTA_SRC]               = { .type = NLA_U32 },
        [RTA_IIF]               = { .type = NLA_U32 },
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b18e85cd7587..0760224927d2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4211,6 +4211,7 @@ void rt6_mtu_change(struct net_device *dev,
unsigned int mtu)
 }

 static const struct nla_policy rtm_ipv6_policy[RTA_MAX+1] = {
+       [RTA_UNSPEC]            = { .strict_start_type = RTA_DPORT + 1 },
        [RTA_GATEWAY]           = { .len = sizeof(struct in6_addr) },
        [RTA_PREFSRC]           = { .len = sizeof(struct in6_addr) },
        [RTA_OIF]               = { .type = NLA_U32 },

I do get that behavior - a new command using a new attribute fails on an
older kernel.

And yes, exact length checking works though with extack we do not need
to spam dmesg:

[  475.175153] netlink: 'ip': attribute type 30 has an invalid length.