[MIR] openvswitch

I don't understand what problem this bug reports. Can you please rephrase it?

Thanks,

Ben (upstream maintainer).

Hi Ben,

This is a promotion report for openvswitch in order to promote it to main from universe.

Please see:

https://wiki.ubuntu.com/MainInclusionProcess

Regards
chuck

 - sfl_agent_*error uses a fixed length buffer. is it ensured that it won't overflow?
 - server team is not subscribed
 - bug #917309 has an existing upstream patch which is not applied?
 - the packaging is two versions behind the upstream version (1.4). how will
   this 1.2.x version supported for the LTS? Debian is already at 1.3
 - openvswitch-datapath-source should stay in universe

And asking the ubuntu-security team for a review

> - sfl_agent_*error uses a fixed length buffer. is it ensured that it won't overflow?

I only see calls to that function with short, fixed strings. The buffer is 1000 bytes. I don't see how it could overflow.

However, it can't hurt to switch to snprintf(), so I posted a patch to ovs-dev: http://openvswitch.org/pipermail/dev/2012-January/014601.html

>However, it can't hurt to switch to snprintf(), so I posted a patch to ovs-dev: http://openvswitch.org/pipermail/dev/2012-January/014601.html

Patch was reviewed. I've pushed it to master and all active 1.x branches.

Deferring for Precise cycle, unless there is reasoning for progression.

Thanks.

I am re-opening this bug because its needed for quantum

Whole slew of security fixes are needed:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=683665

Latest updates from debian unstable merged:

 openvswitch (1.4.2+git20120612-9ubuntu1) quantal; urgency=low
 .
   * Merge from Debian unstable; remaining changes:
     - d/control: Disable openvswitch-datapath-dkms package.
   * Dropped changes:
     - d/patches/kernel_3.5_support.patch: Superceded by
       bug-684057-ovs-ctl-Add-support-for-newer-module-name.patch
 .
 openvswitch (1.4.2+git20120612-9) unstable; urgency=low
 .
   * Apply bug-684057-ovs-ctl-Add-support-for-newer-module-name.patch to
     allow use of the openvswitch kernel module integrated into 3.3 and
     later. Closes: #684057.
 .
 openvswitch (1.4.2+git20120612-8) unstable; urgency=low
 .
   * Apply further patches to fix bugs resulting from moving
     /etc/openvswitch/conf.db to /var/lib/openvswitch in -7.
 .
     This required applying the following bug fix patches:
 .
     bug-681880-3-Make-the-location-of-the-database-separately-configu.patch
     bug-681880-4-tests-Slightly-generalize-utility-function-tests.patch
     bug-681880-5-util-New-function-follow_symlinks.patch
     bug-681880-6-lockfile-Be-more-forgiving-about-lockfiles-for-symli.patch
     bug-681880-7-ovsdb-Do-not-replace-symlinks-by-regular-files-durin.patch
     bug-681880-8-Fix-a-typo-in-commit-f973f2af2.patch
     bug-681880-9-dirs-dbdir-default-must-be-based-on-sysconfdir.patch
 .
   * debian/rules: Configure /var/lib/openvswitch as the database directory
     instead of working through symlinks. (The symlinks are still created
     for compatibility with people and existing software that are
     accustomed to seeing the database in its original location, but the
     Debian packages themselves never use the symlinks.)
 .
   * debian/openvswitch-switch.postrm: Also remove
     /ec/openvswitch/system-id.conf and conf.db backups on purge.
 .
   * utilities/ovs-pki.in: Use mode 0700 instead of 0733 for
     openvswitch-pki "incoming" directory, by applying
     bug-683665-use-mode-700-for-pki-incoming-dir.patch. See the patch for
     complete rationale. Closes: #683665. Thanks to Andreas Beckmann
     <email address hidden> for reporting this bug.
 .
   * debian/openvswitch-pki.postinst: Change mode of existing "incoming"
     directories to 0700 at configure time (see above).
 .
 openvswitch (1.4.2+git20120612-7) unstable; urgency=low
 .
   * Move /etc/openvswitch/conf.db to /var/lib/openvswitch, using symlinks.
     Closes: #681880. Thanks to Bastian Blank <email address hidden> for
     reporting this bug.
 .
     This required applying the following bug fix patches:
 .
     bug-681880-1-lockfile-Fix-hang-locking-through-a-dangling-symlink.patch
     bug-681880-2-ovsdb-Make-ovsdb-tool-create-work-through-a-dangling.patch
 .
   * Start the rest of OVS if bridge compatibility is enabled but the
     kernel bridge module cannot be loaded. Closes: #681955. Thanks to
     Bastian Blank <email address hidden> for reporting this bug.
 .
     This was fixed by applying the following bug fix patch:
 .
     bug-681955-ovs-ctl-Start-the-rest-of-Open-vSwitch.patch
 .
 openvswitch (1.4.2+git20120612-6) unstable; urgency=low
 .
   * utilities/automake.mk: Fix duplicate mention of ovs-vsctl in
     bin_PROGRAMS ...

MIR review:
 * Builds fine with only main
 * Has an extensive testsuite with 991 successful tests and 3 skipped
 * uses dh_python2, not a desktop package
 * There is an Ubuntu delta: drops the dkms package and kernel_3.5_support.patch
 * ubuntu-server is subscribed to the bugs
 * No debian/watch file.
 * Seems wells maintained in Debian with regular updates. Upstream is very active.
 * Upstream is at 1.6.1 now, and 1.4 series seems in maintenance mode now (only bug fixes)
 * There is a lintian warning:
W: openvswitch-switch: binary-without-manpage usr/bin/ovsdb-tool
 * debian/rules uses the old-style format
 * Has some compiler warnings:
../lib/leak-checker.c:77:5: warning: '__malloc_hook' is deprecated (declared at /usr/include/malloc.h:176) [-Wdeprecated-declarations]
../lib/leak-checker.c:78:5: warning: '__realloc_hook' is deprecated (declared at /usr/include/malloc.h:179) [-Wdeprecated-declarations]
../lib/leak-checker.c:79:5: warning: '__free_hook' is deprecated (declared at /usr/include/malloc.h:173) [-Wdeprecated-declarations]
../lib/leak-checker.c:85:5: warning: '__malloc_hook' is deprecated (declared at /usr/include/malloc.h:176) [-Wdeprecated-declarations]
../lib/leak-checker.c:86:5: warning: '__realloc_hook' is deprecated (declared at /usr/include/malloc.h:179) [-Wdeprecated-declarations]
../lib/leak-checker.c:87:5: warning: '__free_hook' is deprecated (declared at /usr/include/malloc.h:173) [-Wdeprecated-declarations]
 * There are 3 open bugs in Ubuntu-- two relating to the now disabled dkms package. No serio
us bugs in Debian.

Security review:
The package does not have a CVE history. No dbus servies, setuid or fscaps usage. No use of sudo and no cron jobs. There is a logrotate configuration. Inspecting the packaging:
 * initscripts/upstart jobs:
/etc/default/openvswitch-controller
/etc/init.d/openvswitch-ipsec
/etc/init.d/openvswitch-switch
 * Initial install of quantum creates no new open ports for openvswitch, but ovsdb-server and ovs-vswitchd are running as root.http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=648667 requests to run as non-root
 * Installing openvswitch* we get additional root services and ovs-controller listening on TCP port 6633 ( as root)
 * The package makes use of encryption (private CA and PKI using openssl)
 * hardening options are used. Since all this is running as root and there is a network listener, could we add PIE and BIND_NOW? Could ovs-controller have an apparmor profile?

Shallow code audit:
 * uses system in lib/netdev-linux.c. While this is generally not a good idea due to shell meta injection, the arguments do not seem to be user controllable
 * execvp: (process_start() and process_run()): arguments only manipulated on invocation from the shell in ovsdb-server.c. For capture_vsctl*, the command line is built up internally in a safe manner that is not user controllable
 * defines its own malloc routines (xmalloc, xrealloc): verifies return value and aborts. Also defines other routines in lib/util.c (eg string routines) and all are defensively coded.
 * there are some uses strcpy but they seemed safe
 * uses encryption:
  * may bootstrap a CA (stream-ssl.c in do_ca_cert_bootstrap())
  * it disables SSLv2 and SSLv3 and uses TLSv1
  * appropriately uses SSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERT
  * doesn't seem to check the CN or SAN of the certificate, but openvswitch uses private CA certificates and client certificates and in all cases the CA certificate needs to be setup on the client so this shouldn't be an issue
 * predictable sockets in /tmp: this is ok-- bind() errors out if the file exists and openvswitch checks the return code

This is a pretty large code base and I was not able to perform an in depth audit. However, openvswitch seems to be defensively coded and I didn't find anything wrong during the review.

Conditional ACK. Feel free to promote after:
* compiling with PIE and BIND_NOW (or demonstrating this is not appropriate)
* investigating feasibility of shipping an AppArmor profile for openvswitch-controller. If it is possible, please ship one

I hope that you guys at Ubuntu are aware that I'm proposing an alternative to get into Debian wheezy, as opposed to what's in Debian sid at the moment. The full details are at: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=683771

(I proposed this weeks ago but it seems that the Debian release team is moving very slowly.)

Hi,

I have enabled the hardening-wrapper in the last uploaded and opened up a bug for the apparmor wrapper. I need to know more of the cause and affect of enabling apparmor for openvswitch.

chuck

I just verified that 1.4.2+git20120612-9ubuntu2 has PIE and BIND_NOW enabled. Chuck filed a bug for apparmor, which satisfies this MIR. While not a condition of this MIR, IMHO it would be good to coordinate with Ben to see if what is happening for wheezy is good for 12.10.

ACK

> While not a condition of this MIR, IMHO it would be good to coordinate with Ben to see if what is happening for wheezy is good for 12.10.

I'm not sure what I'm being asked here, can you be more explicit?

Ben,

You said: "I hope that you guys at Ubuntu are aware that I'm proposing an alternative to get into Debian wheezy, as opposed to what's in Debian sid at the moment. The full details are at: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=683771

(I proposed this weeks ago but it seems that the Debian release team is moving very slowly.)"

I am only asking that the server team look at your work here.

>....I am only asking that the server team look at your work here.

Oh, thanks for clarifying, I hadn't made that connection. That makes sense, thank you.

Moved to main.