netdev - Bug in netlink

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

Message-ID: <20150128234646.GA23945@ycc.fr>
Date:	Thu, 29 Jan 2015 00:46:46 +0100
From:	Ivan Delalande <colona@...sta.com>
To:	netdev@...r.kernel.org, pablo@...filter.org
Subject: Bug in netlink_bind

Hi,

I’ve been trying to debug some of our tests that began failing when
upgrading to 3.18. Our actual failure is caused by the condition added
in commit 97840cb to nfnetlink_bind but the bug has probably been
introduced by 0329274.

Our tests execute the following code, at some point:

	localAddr.nl_groups =
		  NF_NETLINK_CONNTRACK_NEW
		| NF_NETLINK_CONNTRACK_UPDATE
		| NF_NETLINK_CONNTRACK_DESTROY;
	int ret = bind(sd, (sockaddr*)&localAddr, sizeof(localAddr));

these constants are from include/uapi/linux/netfilter/nfnetlink_compat.h
and used as a bit set:

	#define NF_NETLINK_CONNTRACK_NEW        0x00000001
	#define NF_NETLINK_CONNTRACK_UPDATE     0x00000002
	#define NF_NETLINK_CONNTRACK_DESTROY    0x00000004

but, if I understand correctly, internally, constants from
include/uapi/linux/netfilter/nfnetlink.h are used instead:

	enum nfnetlink_groups {
		NFNLGRP_NONE,
		NFNLGRP_CONNTRACK_NEW,
		NFNLGRP_CONNTRACK_UPDATE,
		NFNLGRP_CONNTRACK_DESTROY,

	...

	static const int nfnl_group2type[NFNLGRP_MAX+1] = {
		[NFNLGRP_CONNTRACK_NEW]     = NFNL_SUBSYS_CTNETLINK,
		[NFNLGRP_CONNTRACK_UPDATE]  = NFNL_SUBSYS_CTNETLINK,
		[NFNLGRP_CONNTRACK_DESTROY] = NFNL_SUBSYS_CTNETLINK,

Now in netlink_bind (net/netlink/af_netlink.c), our localAddr.nl_groups
value is assigned to the groups variable and tested with test_bit:

	for (group = 0; group < nlk->ngroups; group++) {
		if (!test_bit(group, &groups)) {
			continue;
		}
		err = nlk->netlink_bind(group);

In our case, for group = 0, bit 0 is indeed set because nl_groups was
ORed with NF_NETLINK_CONNTRACK_NEW (= 1), so nlk->netlink_bind is called
with 0, that is nfnetlink_bind(0) (net/netfilter/nfnetlink.c):

	if (group <= NFNLGRP_NONE || group > NFNLGRP_MAX)
		return -EINVAL;

	type = nfnl_group2type[group];

And so, with this condition added by 97840cb, the syscall fails with
EINVAL. But it means that, before this commit, we would have tried to
get nfnl_group2type[0].
In the same way, with NF_NETLINK_CONNTRACK_UPDATE (= 2),
nfnetlink_bind(1) would have been called and fetched nfnl_group2type[1],
which is declared as nfnl_group2type[NFNLGRP_CONNTRACK_NEW] =
NFNL_SUBSYS_CTNETLINK, and so on.

So, am I missing something or are the group values incorrectly
interpreted differently between netlink_bind and nfnetlink_bind, with a
difference of one? I tried a really naive patch and it made this part of
ours tests pass:

	diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
	index b6bf8e8..d2c65b0 100644
	--- a/net/netlink/af_netlink.c
	+++ b/net/netlink/af_netlink.c
	@@ -1479,7 +1479,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
			for (group = 0; group < nlk->ngroups; group++) {
				if (!test_bit(group, &groups))
					continue;
	-                       err = nlk->netlink_bind(group);
	+                       err = nlk->netlink_bind(group + 1);
				if (!err)
					continue;
				netlink_unbind(group, groups, nlk);

But that was really to test if this would fix my problem, I haven’t
really looked if the value nlk->ngroups was still correct with that or
if there was any other nlk->netlink_bind than netlink_bind that would be
affected.

Thanks,
-- 
Ivan "Colona" Delalande
Arista Networks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html