[MIR] libteam

[Mathieu Trudel-Lapierre]

Availability:
The package is available in universe, built on all architectures.

Rationale:
The package is a new (Build-)Depends for network-manager, for network device teaming (aka user-space bonding) support.

Security:
Could not find any related security advisories.

Quality assurance:
The package is well-maintained in Debian and expected to not require extensive effort to maintain in Ubuntu.

UI standards:
Not applicable.

Dependencies:
All build and binary dependencies are in main.

Standards compliance:
The package meets requirements.

Maintenance:
The package meets requirements.

Background information:
libteam is a library for communicating with the netlink kernel layer in order to create bonded interfaces.

[Michael Terry (mterry)]

This looks mostly fine, needs a team bug subscriber though. And a little nervous about debian WFH bug 743212, where the maintainer says libteam is a low priority for him.

Has a teamd daemon (though looks like it's not run automatically) that talks to the kernel, so handing off to security for a quick look to confirm this is OK.

[Michael Terry (mterry)]

Has a subscriber now, just leaving the security look.

[Seth Arnold (seth-arnold)]

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 NU...

[Launchpad Janitor (janitor)]

[Expired for libteam (Ubuntu) because there has been no activity for 60 days.]

[Tony Espy (awe)]

Re-opened, as there's some interest from OEMs in seeing NM teaming support land in 18.04.

[Mathieu Trudel-Lapierre (cyphermox)]

This one needs to be re-checked by the Security Team after their last code review; and a look by someone from the MIR team. I shouldn't review it, I filed the MIR myself.

[Seth Arnold (seth-arnold)]

I reviewed libteam version 1.26-1 as checked into bionic. This shouldn't
be considered a full security audit but rather a quick gauge of
maintainability.

No CVEs for libteam in our UCT database.

libteam provides a userspace version of various methods of bonding NICs
together that's more flexible than the kernelspace implementations.

- 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
- Daemon manages interfaces, listens on dbus and zmq
- Daemonization code looks complicated, pidfile handling is baffling, but
  nothing that looks insecure.
- No pre/post inst/rm scripts
- No initscripts
- No dbus services
- No setuid files
- bond2team, teamd, teamctl, and teamnl binaries in /usr/bin
- No sudo fragments
- No udev rules
- No test suite
- No cronjobs
- Clean build logs

- No subprocesses spawned
- Memory management looked clean
- Only real 'file' handling is the pidfile, and that's iffy
- Logging looked clean
- two uses of environment variables, TEAM_LOG and TEAMDCTL_LOG, looked
  clean
- privileged operations looked clean
- No cryptography
- Extensive networking operations
- No privileged portions of code
- No temp files
- No WebKit
- No javascript
- No policykit
- Clean cppcheck

Congratulations to the libteam authors, this feels like a large
improvement over the previous review review I performed. I found one
issue with the Debian packaging that I don't understand, one small bug
in teamd_usock_sock_open(), and I still hate the pidfile handling. (If
it works I can understand why no one wants to touch it. I just have
trouble believing it works.)

- lintian warning malformed-deb-archive newer compressed control.tar.xz

- teamd_usock_sock_open() forgot to check the return value from listen(2)

pidfile handling is baffling -- does it work?
 - teamd_context_init() sets the global variable to point to a pointer to
   the pidfile name (NULL at start)
 - a function pointer named daemon_pid_file_proc is created to alias
   teamd_pid_file_proc()
 - teamd_set_default_pid_file() generates a dynamically allocated name
 - when teamd_pid_file_proc() is called via the function pointer variable,
   it just returns the global variable that is probably NULL at this point
 - None of this looks security relevant but it sure is surprisingly gross.

Security team ACK for promoting libteam to main.

Thanks

[Mathieu Trudel-Lapierre (cyphermox)]

Looks good now from my point of view; there's a team subscriber, and the security review has been done.

MIR approved.

[Jeremy Bicha (jbicha)]

As discussed earlier today, we are thinking of waiting for NetworkManager 1.10 before enabling this option. At any rate, we do want it in Ubuntu 18.04 LTS.

[Jeremy Bicha (jbicha)]

NetworkManager 1.10 has been uploaded with libteam support enabled and libteam now shows on component-mismatches-proposed

https://launchpad.net/ubuntu/+source/network-manager/1.10.4-1ubuntu1

[Steve Langasek (vorlon)]

Override component to main
libteam 1.26-1 in bionic: universe/misc -> main
libteam-dev 1.26-1 in bionic amd64: universe/libdevel/optional/100% -> main
libteam-dev 1.26-1 in bionic arm64: universe/libdevel/optional/100% -> main
libteam-dev 1.26-1 in bionic armhf: universe/libdevel/optional/100% -> main
libteam-dev 1.26-1 in bionic i386: universe/libdevel/optional/100% -> main
libteam-dev 1.26-1 in bionic ppc64el: universe/libdevel/optional/100% -> main
libteam-dev 1.26-1 in bionic s390x: universe/libdevel/optional/100% -> main
libteam-utils 1.26-1 in bionic amd64: universe/net/optional/100% -> main
libteam-utils 1.26-1 in bionic arm64: universe/net/optional/100% -> main
libteam-utils 1.26-1 in bionic armhf: universe/net/optional/100% -> main
libteam-utils 1.26-1 in bionic i386: universe/net/optional/100% -> main
libteam-utils 1.26-1 in bionic ppc64el: universe/net/optional/100% -> main
libteam-utils 1.26-1 in bionic s390x: universe/net/optional/100% -> main
libteam5 1.26-1 in bionic amd64: universe/net/optional/100% -> main
libteam5 1.26-1 in bionic arm64: universe/net/optional/100% -> main
libteam5 1.26-1 in bionic armhf: universe/net/optional/100% -> main
libteam5 1.26-1 in bionic i386: universe/net/optional/100% -> main
libteam5 1.26-1 in bionic ppc64el: universe/net/optional/100% -> main
libteam5 1.26-1 in bionic s390x: universe/net/optional/100% -> main
libteamdctl0 1.26-1 in bionic amd64: universe/net/optional/100% -> main
libteamdctl0 1.26-1 in bionic arm64: universe/net/optional/100% -> main
libteamdctl0 1.26-1 in bionic armhf: universe/net/optional/100% -> main
libteamdctl0 1.26-1 in bionic i386: universe/net/optional/100% -> main
libteamdctl0 1.26-1 in bionic ppc64el: universe/net/optional/100% -> main
libteamdctl0 1.26-1 in bionic s390x: universe/net/optional/100% -> main
25 publications overridden.