Comment 3 for bug 1392012

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