Bug 851148

+++ This bug was initially created as a clone of Bug #848017 +++

cgdcbxd does not check if the DCB packets have PID/port ID 0 and originated in the kernel.  As a result, unprivileged local users can send messages to cgdcbxd, causing it to alter the cgroup configuration.

In addition, mnl_attr_get_str is used as if the resulting string is NUL-terminated.  This isn't always the case (at least when processing crafted messages).

--- Additional comment from fweimer on 2012-08-14 13:39:03 CEST ---

(In reply to comment #0)
> In addition, mnl_attr_get_str is used as if the resulting string is
> NUL-terminated.  This isn't always the case (at least when processing
> crafted messages).

Based on a discussion with the libmnl folks, mnl_attr_validate(attr, MNL_TYPE_NUL_STRING) should be used instead of mnl_attr_validate(attr, MNL_TYPE_STRING).  This is compatible with what the kernel sends.

--- Additional comment from nhorman on 2012-08-14 17:29:44 CEST ---

I'm not sure thats even necessecary.  Every received message from the kernel is handled in cgdcbxd by mnl_cb_run->__mnl_cb_run.  Internal to that function, we validate the pid and port id in mnl_nlmsg_portid_ok, which validates:

1) that nlmsg_pid is 0
2) That portid is either 0 or matches the expected port id

So it seems, unless I'm missing something that all of your concerns are addressed there

--- Additional comment from fweimer on 2012-08-14 17:39:57 CEST ---

(In reply to comment #2)
> I'm not sure thats even necessecary.  Every received message from the kernel
> is handled in cgdcbxd by mnl_cb_run->__mnl_cb_run.  Internal to that
> function, we validate the pid and port id in mnl_nlmsg_portid_ok, which
> validates:
> 
> 1) that nlmsg_pid is 0
> 2) That portid is either 0 or matches the expected port id
> 
> So it seems, unless I'm missing something that all of your concerns are
> addressed there

Here's an excerpt from a GDB session when processing a packet which was generated by the nobody user:

Breakpoint 2, mnl_nlmsg_portid_ok (nlh=nlh@entry=0x7fffffffcec0, portid=portid@entry=0) at nlmsg.c:238
238		return nlh->nlmsg_pid && portid ? nlh->nlmsg_pid == portid : true;
(gdb) print nlh->nlmsg_pid
$1 = 5971
(gdb) finish
Run till exit from #0  mnl_nlmsg_portid_ok (nlh=nlh@entry=0x7fffffffcec0, portid=portid@entry=0) at nlmsg.c:238
0x00000037f1a01fff in __mnl_cb_run (cb_ctl_array_len=0, cb_ctl_array=0x0, data=0x0, 
    cb_data=0x403182 <cgdcbx_dcb_cb>, portid=0, seq=0, numbytes=<optimized out>, buf=<optimized out>)
    at callback.c:58
58			if (!mnl_nlmsg_portid_ok(nlh, portid)) {
Value returned is $2 = true
(gdb) 

This is actually the documented behavior, "We skip the tracking for netlink message whose portID is zero since it is reserved for event-based kernel notifications.".  I think this function is not suitable for checking that the packet originated in the kernel, and you have to do this check on your own, prior to calling mnl_cb_run.

Perhaps we should ask upstream if this is a bug in libmnl, just to be sure.

--- Additional comment from nhorman on 2012-08-14 18:28:44 CEST ---

agreed, it seems that a non-zero pid and a valid port id should result in a failed check, as that would still allow kernel notifications to go through (as they would have a 0 pid), but catch users.

CCing hushan jia, the libmnl maintainer.  What are your thoughts on the issue?

--- Additional comment from fweimer on 2012-08-15 09:44:31 CEST ---

(In reply to comment #4)
> agreed, it seems that a non-zero pid and a valid port id should result in a
> failed check, as that would still allow kernel notifications to go through
> (as they would have a 0 pid), but catch users.
> 
> CCing hushan jia, the libmnl maintainer.  What are your thoughts on the
> issue?

Please note that this issue is still under embargo.

I've asked Pablo Neira Ayuso (libmnl upstream) for his opinion as well.

--- Additional comment from fweimer on 2012-08-15 14:59:52 CEST ---

(In reply to comment #5)

> I've asked Pablo Neira Ayuso (libmnl upstream) for his opinion as well.

He agrees that this is a bug in libmnl.  We're working on a fix.

--- Additional comment from nhorman on 2012-08-15 15:16:31 CEST ---

copy that, reassigning the component on this bz to libmnl.  Thanks!

--- Additional comment from hushan.jia on 2012-08-16 03:57:06 CEST ---

(In reply to comment #6)
> (In reply to comment #5)
> 
> > I've asked Pablo Neira Ayuso (libmnl upstream) for his opinion as well.
> 
> He agrees that this is a bug in libmnl.  We're working on a fix.
Can you copy me on the email thread? thanks.

--- Additional comment from fweimer on 2012-08-23 11:37:29 CEST ---

(In reply to comment #8)
> Can you copy me on the email thread? thanks.

The discussion resulted in this patch proposal for the kernel:

http://marc.info/?l=linux-netdev&m=134522422125983

This will fix the issue for cgdcbxd only very indirectly, if it is ever accepted (it might break existing userspace software).

I think the current libmnl API does not really provide a way to check message origin.  The source address of a packet is discarded in mnl_socket_recvfrom, and that would be the most natural place to check.  Looking at the port ID in the nlmsghdr header (which is what mnl_nlmsg_portid_ok does) is not actually helpful.

--- Additional comment from fweimer on 2012-08-23 13:10:01 CEST ---

Changing the component back to cgdcbxd because a code change in cgdcbxd will be needed (no transparent fix in libmnl is planned).