netdev - Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.

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:   Tue, 25 Sep 2018 09:37:41 -0600
From:   David Ahern <dsahern@...il.com>
To:     Jiri Benc <jbenc@...hat.com>,
        Christian Brauner <christian@...uner.io>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: netlink: 16 bytes leftover after parsing attributes in process
 `ip'.

On 9/25/18 8:47 AM, Jiri Benc wrote:
> On Tue, 25 Sep 2018 11:49:10 +0200, Christian Brauner wrote:
>> So if people really want to hide this issue as much as we can then we
>> can play the guessing game. I could send a patch that roughly does the
>> following:
>>
>> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
>>         guessed_header_len = sizeof(struct ifaddrmsg);
>> else
>>         guessed_header_len = sizeof(struct ifinfomsg);
>>
>> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
>> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
>> This propert is a __s32 which should bring the message up to 12 bytes
>> (not sure about alignment requiremnts and where we might wend up ten)
>> which is still less than the 16 bytes without that property from
>> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
>> break when ifaddrmsg grows a new member or we introduce another property
>> that is valid in RTM_GETADDR requests. It also will not work cleanly
>> when users stuff additional properties in there that are vaif (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,lid for the
>> address family but are not used int RTM_GETADDR requests.
> 
> I'd expect that any potential existing code that makes use of other
> attributes already assumes ifaddrmsg. Hence, if the nlmsg_len is >
> sizeof(ifinfomsg), you can be sure that there are attributes and thus
> the struct used was ifaddrmsg.
> 
> So, in order for RTM_GETADDR to work reliably with attributes, you have
> to ensure that the length is > sizeof(ifinfomsg).

One of the many on-going efforts I have in progress is kernel side
filtering of route dumps. It has this same problem. For it I am
proposing a new flag:

#define RTM_F_PROPER_HEADER    0x4000

ifinfomsg is 16 bytes which is > rtmsg at 12. If the message length is >
16, then rtm_flags can be checked to know if the proper header is sent.

For ifaddrmsg things do not line up as well. Worst all of the flag bits
are used. But, perhaps one can be overloaded with the limit that you can
never filter on its presence. Since you are adding the first filter to
address dumps such a limitation should be ok.

For example something like this (whitespace damaged on paste) to remove
the guess work:

diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
index dfcf3ce0097f..8e3e9d475db5 100644
--- a/include/uapi/linux/if_addr.h
+++ b/include/uapi/linux/if_addr.h
@@ -41,6 +41,11 @@ enum {
 #define IFA_MAX (__IFA_MAX - 1)

 /* ifa_flags */
+/* IFA_F_PROPER_HEADER is only set in ifa_flags for dumps
+ * to indicate the ancillary header is the expected ifaddrmsg
+ * vs ifinfomsg from legacy userspace
+ */
+#define IFA_F_PROPER_HEADER    0x01
 #define IFA_F_SECONDARY                0x01
 #define IFA_F_TEMPORARY                IFA_F_SECONDARY

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index bfe3ec7ecb14..256b9f88db8f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -5022,8 +5022,13 @@ static int inet6_dump_addr(struct sk_buff *skb,
struct netlink_callback *cb,
        s_idx = idx = cb->args[1];
        s_ip_idx = ip_idx = cb->args[2];

-       if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
-                       ifa_ipv6_policy, NULL) >= 0) {
+       if (nlmsg_len(cb->nlh) >= sizeof(struct ifaddrmsg) &&
+           ((struct ifaddrmsg *) nlmsg_data(cb->nlh))->ifa_flags &
IFA_F_PROPER_HEADER) {
+
+               if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
IFA_MAX,
+                               ifa_ipv6_policy, NULL) >= 0) {
+       ...
+
                if (tb[IFA_TARGET_NETNSID]) {
                        netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);


For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
so this should be ok.