[MIR] openvswitch

Bug #914160 reported by Chuck Short
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
openvswitch (Ubuntu)
High
Chuck Short

Bug Description

Availability: Available in universe for all arches
Rationale: Apart of the servercloud-p-openvswitch-mir specification.
Security: No open CVE history.
Quatlity:
* Package works out of the box.
* No debconf questions.
* Package is well maintained in Ubuntu/Debian.
* No special hardware is required
* Package has a testsuite enabled.
* No debian/watch file.
Dependencies: All dependencies are in main
Maintenance: Simple package that is going to be maintained by the Ubuntu Server Team.

Revision history for this message
Ben Pfaff (blp-nicira) wrote :

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

Thanks,

Ben (upstream maintainer).

Revision history for this message
Chuck Short (zulcss) wrote :

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

Michael Terry (mterry)
Changed in openvswitch (Ubuntu):
assignee: nobody → Matthias Klose (doko)
Dave Walker (davewalker)
Changed in openvswitch (Ubuntu):
importance: Undecided → Medium
Revision history for this message
Matthias Klose (doko) wrote :

 - 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

Changed in openvswitch (Ubuntu):
status: New → Incomplete
assignee: Matthias Klose (doko) → nobody
Revision history for this message
Ben Pfaff (blp-nicira) wrote :

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

Revision history for this message
Ben Pfaff (blp-nicira) wrote :

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

Revision history for this message
Dave Walker (davewalker) wrote :

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

Thanks.

Changed in openvswitch (Ubuntu):
status: Incomplete → Won't Fix
Revision history for this message
Chuck Short (zulcss) wrote :

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

Changed in openvswitch (Ubuntu):
status: Won't Fix → Triaged
status: Triaged → In Progress
importance: Medium → High
James Page (james-page)
Changed in openvswitch (Ubuntu):
status: In Progress → New
Michael Terry (mterry)
Changed in openvswitch (Ubuntu):
assignee: nobody → Jamie Strandboge (jdstrand)
Changed in openvswitch (Ubuntu):
status: New → In Progress
Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

Whole slew of security fixes are needed:

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

Revision history for this message
James Page (james-page) wrote :
Download full text (3.5 KiB)

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

Read more...

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

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.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

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.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

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

Changed in openvswitch (Ubuntu):
assignee: Jamie Strandboge (jdstrand) → Chuck Short (zulcss)
Revision history for this message
Ben Pfaff (blp-nicira) wrote :

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

Revision history for this message
Chuck Short (zulcss) wrote :

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

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

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

Changed in openvswitch (Ubuntu):
status: In Progress → Fix Committed
Revision history for this message
Ben Pfaff (blp-nicira) wrote :

> 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?

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

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.

Revision history for this message
Ben Pfaff (blp-nicira) wrote :

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

Revision history for this message
Colin Watson (cjwatson) wrote :

Moved to main.

Changed in openvswitch (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Related blueprints

Remote bug watches

Bug watches keep track of this bug in other bug trackers.