I reviewed libteam version 1.12-1 as checked into vivid. This should not be considered a full security audit but rather a quick gauge of maintainability.
- libteam provides a userspace interface to manage different kinds of network interface bonding; it uses json-based configuration files to configure kernel interfaces using netlink, among other tasks - Build-Depends: debhelper, dh-autoreconf, libdaemon-dev, libdbus-1-dev, libjansson-dev, libnl-3-dev, libnl-cli-3-dev, libnl-genl-3-dev, libnl-route-3-dev, pkg-config - No cryptography - Extensive networking - Daemonizes using libdaemon, assumed fine - pre/post inst/rm automatically generated - No initscripts - No dbus - No setuid - No sudo fragments - No udev rules - Provides teamdctl, teamnl, bond2team, teamd binaries in /usr/bin - No test suite - No cron jobs - Clean build logs - No subprocesses spawned - Most memory management looks safe; several potential memory leaks, pidfile handling is curious - No direct writing to files, logging function can be changed - Extensive privileged operations to manipulate networking stack - No cryptography - Extensive network administrative, can use zmq or unix domain sockets to talk with drivers - No temporary files - No privileged portions of code - No WebKit - No JavaScript - No PolicyKit - Noisy cppcheck, found several flaws and some odd operations probably designed to silence a poor static analysis checker libteam is mixed-quality code; some portions look like they are fairly solid while other portions feel heavily "over engineered" or made prematurely pluggable. The pidfile handling is awkward, some of the json APIs introduce many layers of "shim" functions that seem pointless, and there's a repeated use of assigning uninitialized variables to themselves, presumably to silence some poor static analysis tool. (Discovered in part by cppcheck, which reported these and other more serious issues.) libteam would probably benefit strongly from signing up for Coverity scans. While many of the items listed below may not be actual issues when call graphs are followed through to their entirety, the fact that they looked like issues on a quick inspection gives me the feeling that the code is still brittle and unreviewed in places: - char *recvmsg = recvmsg; Aliasing a system call is really poor form. - The json_obj in teamd_config_path_exists() is passed to teamd_config_object_get() without being used. It is then passed to teamd_json_path_lite_va() without being used. It is then passed to __teamd_json_path_lite_va(), which assigns to it. This API is complicated for no reason. - Strongly suspect __teamd_json_path_lite_va() is buggy Awkward construct used over and over again for no reason: - json_t *json_obj = json_obj; - int fd = fd; - struct dispatch_priv *dp = dp; - char *msg = msg; - json_t *val_json_obj = val_json_obj; - json_t *vg_json_obj = vg_json_obj; - int ret = ret; - run_cmd_getoption() can leak 'buf' if realloc() fails, the usual approach is to use a different lvalue. - update_phys_port_id() doesn't validate phys_port_id_len is positive or less than MAX_PHYS_PORT_ID_LEN; doesn't validate that phys_port_id isn't NULL. - cli_zmq_recv() "zmq: send failed" message is incorrect, a recv failed - __strencode() walks str five separate times - pid file handling is unusual and awkward; a function pointer that is used to call a statically declared function a few lines after definition, the only job of the function is to return a global variable that points towards dynamically allocated ctx structures; action-at-a-distance initialization via teamd_context_init(). Why is it so much more complicated than most programs? - __slow_addr_add_del() memcpy() to ifr.ifr_name uses strlen(devname) rather than size of the destination. This may mean ifr.ifr_name isn't NUL terminated if the name is exactly IF_NAMESIZE bytes long, or may overflow the buffer space available if devname is longer. (The struct teamd_port doesn't restrict the size of devname=ifname.) - __set_sockaddr() may leak memory from getaddrinfo() in "should not happen" case - lw_tipc_load_options() memory leak in strlen >= TIPC_MAX_BEARER_NAME case - lw_tipc_link_state_change() no malloc NULL check, memory allocation failure could kill the whole daemon I propose the following path for including libteam into main: - Fix all cppcheck errors - Rename 'recvmsg' variable to not alias a system call - Fix memory leaks in - __set_sockaddr() - lw_tipc_load_options() - run_cmd_getoption() - Fix lw_tipc_link_state_change() missing malloc NULL check - Modify update_phys_port_id() to impose upper and lower bounds on phys_port_id_len - Modify __slow_addr_add_del() to ensure the amount of data copied to ifr.ifr_name doesn't exceed the space available I think the program would be stronger and more reliable if the pidfile handling is addressed; this would be a much larger modification than the conditions above, though, so I don't make it a condition. The fixes above ought to be straightforward and non-controversial. Not Yet on libteam. Thanks ** Changed in: libteam (Ubuntu) Assignee: Seth Arnold (seth-arnold) => (unassigned) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1392012 Title: [MIR] libteam To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/libteam/+bug/1392012/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs