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]

Message-ID: <20180925164715.338bb059@redhat.com>
Date:   Tue, 25 Sep 2018 16:47:15 +0200
From:   Jiri Benc <jbenc@...hat.com>
To:     Christian Brauner <christian@...uner.io>
Cc:     David Ahern <dsahern@...il.com>,
        "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 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).

This can be achieved by putting IFA_TARGET_NETNSID into a nested
attribute. Just define IFA_EXTENDED (feel free to invent a better name,
of course) and put IFA_TARGET_NETNSID inside. Then in the code, attempt
to parse only when the size is large enough:

	if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
		int err;
 
		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
		                  IFA_MAX, ifa_ipv6_policy, NULL);
		if (err < 0)
			return err;
		if (tb[IFA_EXTENDED]) {
			...parse the nested attribute...
			if (tb_nested[IFA_TARGET_NETNSID]) {
				...etc...
			}
		}
	}

Another option is forcing the user space to add another attribute, for
example, IFA_FLAGS_PRESENT, and attempt parsing only when it is
present. The logic would then be:

	if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
		int err;
 
		err = nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
		                  IFA_MAX, ifa_ipv6_policy, NULL);
		if (err < 0)
			return err;
		if (tb[IFA_FLAGS_PRESENT] && tb[IFA_TARGET_NETNSID]) {
			...etc...
		}
	}

 Jiri