Bug #1610286 “[MIR] libapache2-mod-auth-mellon, liblasso3” : Bugs : libapache2-mod-auth-mellon package : Ubuntu

Corey Bryant (corey.bryant) on 2016-08-05

affects:

ubuntu → libapache2-mod-auth-mellon (Ubuntu)

Revision history for this message

Michael Terry (mterry) wrote on 2016-08-10:

#1

libapache2-mod-auth-mellon has no security history? The last changelog entry has this:
- Fixes Denial of Service issues [CVE-2016-2145, CVE-2016-2146].

Looks like both of these are security sensitive, will pass to security team.

Changed in libapache2-mod-auth-mellon (Ubuntu):
assignee:	nobody → Ubuntu Security Team (ubuntu-security)
Changed in lasso (Ubuntu):
assignee:	nobody → Ubuntu Security Team (ubuntu-security)

Revision history for this message

Seth Arnold (seth-arnold) wrote on 2017-05-12:

#2

I reviewed libapache2-mod-auth-mellon version 0.12.0-1 as checked into
zesty. This should not be considered a full security audit but rather a
quick gauge of maintainability.

- Four previous CVEs were reported against this module. While this is
unfortunate I don't think it's unduly distressing.

- libapache2-mod-auth-mellon provides an authn and authz interface for
Apache that can perform queries against a centralized SAML IdP.
- Build-Depends: debhelper, autotools-dev, dh-apache2, apache2-dev,
libcurl3-dev, liblasso3-dev
- Does not itself daemonize
- pre/post inst/rm are automatically generated
- No initscripts
- No dbus services
- No setuid files
- No binaries in the path
- No sudo fragments
- No udev rules
- No test suite
- No cron jobs
- Clean build logs
- Clean cppcheck

- No subprocesses spawned
- Memory manage is usual for apache modules
- File IO is under control of configuration files
- Logging is very extensive
- Environment variables can be read as directed by configuration files
- No privileged system calls
- Does use curl to download data
- No obviously privileged portions of the code
- The use of /var/tmp/mellonLock should be changed
- No WebKit
- No JavaScript
- No PolicyKit

Here's some notes I took while reading the code; Olav will release a
new version soon with these issues addressed. Olav was responsive and
thoughtful.

- am_postdir_cleanup() could be extremely expensive if it had to walk a
  directory of twenty thousand saved requests. It has no mechanism to
  bail after a certain amount of work. am_save_post() appears to call
  am_postdir_cleanup() unconditionally on every saved post. This has
  the potential to have spiraling costs until finally all threads are
  spending all their time re-walking the same pile of files looking for
  old ones to delete.
  [The default setting is 100.]
- am_hc_block_write() is tail-recursive. If the compiler fails to optimize
  the tail call away, the call depth might wind up blowing out the stack
  frame available to the thread. How large is this stack frame? What is
  the largest amount of data that curl can be expected to retrieve? Having
  a thousand little 1012 byte structures in memory just to handle a one
  megabyte download sounds suboptimal.
  [Curl currently calls this function with no more than 16kB blocks, the
  responses from auth servers are typically < 4kB.]
- auth_mellon_commands claims the lockfile path is /tmp/mellonLock -- this
  is not a safe default if it's correct. (I think it's no longer correct,
  but /var/tmp/mellonLock isn't better -- someplace only the webserver
  user can write to would be ideal. mellonPost directory perhaps?)
  [Not currently used on Linux,

Security team ACK for promoting libapache2-mod-auth-mellon to main.

Thanks

I reviewed libapache2-mod-auth-mellon version 0.12.0-1 as checked into
zesty. This should not be considered a full security audit but rather a
quick gauge of maintainability.

- Four previous CVEs were reported against this module. While this is
  unfortunate I don't think it's unduly distressing.

- libapache2-mod-auth-mellon provides an authn and authz interface for
  Apache that can perform queries against a centralized SAML IdP.
- Build-Depends: debhelper, autotools-dev, dh-apache2, apache2-dev,
  libcurl3-dev, liblasso3-dev
- Does not itself daemonize
- pre/post inst/rm are automatically generated
- No initscripts
- No dbus services
- No setuid files
- No binaries in the path
- No sudo fragments
- No udev rules
- No test suite
- No cron jobs
- Clean build logs
- Clean cppcheck

- No subprocesses spawned
- Memory manage is usual for apache modules
- File IO is under control of configuration files
- Logging is very extensive
- Environment variables can be read as directed by configuration files
- No privileged system calls
- Does use curl to download data
- No obviously privileged portions of the code
- The use of /var/tmp/mellonLock should be changed
- No WebKit
- No JavaScript
- No PolicyKit

Here's some notes I took while reading the code; Olav will release a
new version soon with these issues addressed. Olav was responsive and
thoughtful.

- am_postdir_cleanup() could be extremely expensive if it had to walk a
  directory of twenty thousand saved requests. It has no mechanism to
  bail after a certain amount of work. am_save_post() appears to call
  am_postdir_cleanup() unconditionally on every saved post. This has
  the potential to have spiraling costs until finally all threads are
  spending all their time re-walking the same pile of files looking for
  old ones to delete.
  [The default setting is 100.]
- am_hc_block_write() is tail-recursive. If the compiler fails to optimize
  the tail call away, the call depth might wind up blowing out the stack
  frame available to the thread. How large is this stack frame? What is
  the largest amount of data that curl can be expected to retrieve? Having
  a thousand little 1012 byte structures in memory just to handle a one
  megabyte download sounds suboptimal.
  [Curl currently calls this function with no more than 16kB blocks, the
  responses from auth servers are typically < 4kB.]
- auth_mellon_commands claims the lockfile path is /tmp/mellonLock -- this
  is not a safe default if it's correct. (I think it's no longer correct,
  but /var/tmp/mellonLock isn't better -- someplace only the webserver
  user can write to would be ideal. mellonPost directory perhaps?)
  [Not currently used on Linux,

Security team ACK for promoting libapache2-mod-auth-mellon to main.

Thanks

Revision history for this message

Seth Arnold (seth-arnold) wrote on 2017-05-12:

#3

I reviewed lasso 2.5.0-5build1 as checked into ubuntu zesty. This should
not be considered a full security audit but rather a quick gauge of
maintainability.

lasso has two CVEs in our databases, CVE-2009-0050 and CVE-2015-1783. The
first was an OpenSSL API misuse which was common to many other
applications due to awkward API design. The second was use of an
uninitialized variable, which appeared to be due to a typo.

Lasso is part of a single-signon system as part of Project Liberty.

- Build-Depends: debhelper, dh-python, libxml2-dev, libxslt1-dev,
  libxmlsec1-dev, libxmlsec1-openssl, libglib2.0-dev, python-all-dev,
  python3-all-dev, libexpat1-dev, fastjar, python-lxml, python3-lxml,
  python-six, python3-six, chrpath, dh-python

- does not itself do networking
- does not itself daemonize
- pre/post inst/rm scripts are automatically generated
- no init scripts
- no dbus services
- no setuid
- no binaries in PATH
- no sudo fragments
- no udev rules
- there is a test suite but it is not run at build time
- no cron jobs
- build logs are clean

- no subprocesses spawned
- memory management is using glib's allocators, which kill the program on
  failure. There are roughly 360 of these in the sources; it is a big
  assumption for a library to make that falling over is the best way to
  handle an error.
- file IO under control of callers
- extensive error logging, spot checks looked safe
- uses environment variables REQUEST_METHOD and QUERY_STRING presumably
  from a web server
- no privileged functions
- extensive use of encryption
- does not itself do networking
- no privileged portions of code
- no temporary files
- no webkit
- no javascript
- no policykit
- many cppcheck warnings but probably only one real error:
  - lasso/id-wsf/discovery.c lasso_discovery_build_credential()
    'profile' may be NULL in this expression:
    response = LASSO_DISCO_QUERY_RESPONSE(profile->response);

- tests/tests.c my_malloc() fails to account for size=0 allocations

While lasso looks like it was programmed defensively, the subject matter
is extremely complex, and logic errors are likely. It's just too large to
avoid. We will rely heavily on upstream or other teams inside of Canonical
in the event vulnerabilities in the protocol handling are discovered.

Please enable the test suite.

Security team ACK on promoting lasso to main on the condition that the
test suite is enabled.