+++ 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).
Created attachment 680486 [details] [PATCH] libmnl: Add filtering support to library as a convienience Theres been recent discussion about detecting and discarding unwanted netlink messages in libmnl, so that we can avoid having applications get spoofed by user space processes sending messages with malformed netlink headers. Commonly applications want to be able to only receive messages from the kernel, but libmnl currently doesn't offer a mechanism to do that. This patch adds such a mechanism. It creates a function mnl_socket_recvfrom_filter, that adds an extra function pointer parameter which is used to interrogate recieved frames and filter them based on a desired criteria. It also adds a convieninece function mnl_recvfrom_filter_user which can be passed as the filter agrument in mnl_socket_recvfrom_filter, so as to prevent individual applications from re-inventing the wheel over and over again. Signed-off-by: Neil Horman <nhorman> --- include/libmnl/libmnl.h | 4 ++ src/socket.c | 99 +++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 92 insertions(+), 11 deletions(-)
Note the above is untested, but it builds fine, and looks pretty straightforward. Since the kernel bits, according to the mailing list appear to have languished, I thought perhaps would be a good alternate solution. It gives us a common method to do filtering in libmnl without making each application re-invent the same type of filtering). Please let me know what you think.
I was finally able to build this. I needed an additional patch which makes the new symbols visible. We should really try to get this patch upstream, so that we can get an official version for the symbol. I'm not sure if the retry loop in mnl_socket_recvfrom_filter is actually a good idea. I think this can cause cgdcbxd to get stuck waiting on one socket if a spoofed message is received which is not quickly followed by a non-spoofed message. I think mnl_socket_recvfrom_filter should return with an error code if a message is received which is then filtered. Or we should just expose the sender socket address to the caller and do the filtering there.
Created attachment 716400 [details] libmnl-filter-map.patch Additional patch for exporting the new symbols.
Florian, regarding your suggestion about the loop, My feeling was that the detection could be done in the filter function, since we have the socket address available there. If thats unclear, we could document it as the point at which to break out of the loop. If you like I can propose this upstream. This needs to go to the netfilter-devel list, yes?
(In reply to comment #5) > Florian, regarding your suggestion about the loop, My feeling was that the > detection could be done in the filter function, since we have the socket > address available there. If thats unclear, we could document it as the > point at which to break out of the loop. I'm worried that the caller performs select()/poll(), calls the receive function to obtain one packet (which should succeed without blocking because select()/poll() indicated that data was available). But after receiving the that packet, recvmsg() is called again if it was filtered, and that call can now block unexpectedly. > If you like I can propose this upstream. This needs to go to the > netfilter-devel list, yes? Yes, that would be the right list.
hmm, ok, I see your point. Ok, given the options, I rather like the error code return a bit better. Either that, or a second method pointer to a receive function, where the caller can pass some code to do exactly the receive operation that they would like. I'll recode, and post it shortly.
This bug appears to have been reported against 'rawhide' during the Fedora 19 development cycle. Changing version to '19'. (As we did not run this process for some time, it could affect also pre-Fedora 19 development cycle bugs. We are very sorry. It will help us with cleanup during Fedora 19 End Of Life. Thank you.) More information and reason for this action is here: https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora19
Fedora 19 changed to end-of-life (EOL) status on 2015-01-06. Fedora 19 is no longer maintained, which means that it will not receive any further security or bug fix updates. As a result we are closing this bug. If you can reproduce this bug against a currently maintained version of Fedora please feel free to reopen this bug against that version. If you are unable to reopen this bug, please file a new report against the current release. If you experience problems, please add a comment to this bug. Thank you for reporting this bug and we are sorry it could not be fixed.
This was fixed in the kernel.