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
