netdev - Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

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: <20140324143843.GC28666@madcap2.tricolour.ca>
Date:	Mon, 24 Mar 2014 10:38:43 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	David Miller <davem@...emloft.net>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
	eparis@...hat.com, sgrubb@...hat.com, hadi@...atatu.com
Subject: Re: [PATCH] netlink: have netlink per-protocol bind function return
 an error code.

On 14/03/23, David Miller wrote:
> From: Richard Guy Briggs <rgb@...hat.com>
> Date: Fri, 21 Mar 2014 12:39:11 -0400
> 
> > @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> >  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> >  		return 0;
> >  
> > +	if (nlk->netlink_bind && nladdr->nl_groups) {
> > +		int i;
> > +
> > +		for (i = 0; i < nlk->ngroups; i++)
> > +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> > +				err = nlk->netlink_bind(i);
> > +				if (err)
> > +					return err;
> > +			}
> > +	}
> > +
> 
> You can't just leave a partially set of completed bindings in place.

In the general case, I agree.

> It's not valid to leave half-baked state like this.

In the one existing case (netfilter), it adds a module that is never
unloaded.  (refcounts are bumped up and down, but I don't see an
auto-reap based on cleared multicast group subscriptions.)  For that
matter, netlink_realloc_groups() isn't reversed on error either.

In the proposed case (audit) it is only a permissions check, so there is
nothing to undo.

So, I was being lazy looking at the existing situation.

> If you return an error, all of the binding state changes must be
> completely undone.

Is it time to add a ".unbind = netlink_unbind" to struct proto_ops
netlink_ops?  (I am only half serious here...)

> If you can't find a way to do this cleanly, you'll need to find
> a way for the audit code to not return an error.

Fair enough.  I'll go back and look at updating subscriptions and
listeners first and undoing those actions if the bind fails.  In the
case of netlink_setsockopt() it is just one to undo, which is easy.
netlink_bind() is a bit more complex, but doable.

The whole purpose here was to add a way for each protocol to be able to
add its own permissions check and signal a way for netlink to refuse the
subscription if the userspace process doesn't have the required
permissions, so not returning an error defeats that whole purpose.

- RGB

--
Richard Guy Briggs <rbriggs@...hat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
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