[MIR] python-saml2, xmlsec1

[James Page]

>> python-pysaml2 <<

Avaliability: In universe
Rationale: New dependency for keystone.
Security: No CVE's found.
Quality assurance: Unit tests executed as part of package build (two xfails).
Dependencies: All in main apart from those identified on this MIR
Standards compliance: OK
Maintenance: Server Team

>> xmlsec1 <<

Avaliability: In universe
Rationale: Dependency for pysaml2.
Security: Some old CVE's found, all resolved.
Quality assurance: Unit tests executed as part of package build.
Dependencies: All in main apart from those identified on this MIR
Standards compliance: OK
Maintenance: Server Team

[James Page (james-page)]

Still working on pysaml2 test suite enablement.

[Michael Terry (mterry)]

Passing xmlsec1 to Jamie, since it has security surface.

[Michael Terry (mterry)]

Regarding python-repoze.who... It looks fine (has tests, bug subscriber, no important bugs, etc). But it's orphaned in Debian. Can I get a comment on how much of a problem the server team thinks that will be?

I'll also pass to Seth for a quick look, since this is an authentication module.

[Michael Terry (mterry)]

python-pysaml2 is fine from a packaging point of view, but I'm also going to pass to Seth for a quick look.

[Michael Terry (mterry)]

I was looking at xmlsec1 too, from a packaging perspective. And it looks like test failures don't fail the build. That should be addressed.

[Seth Arnold (seth-arnold)]

I reviewed xmlsec1 version 1.2.20-2ubuntu1 as found in vivid. This
shouldn't be considered a full security audit but rather a quick gauge of
maintainability.

Two CVEs, one affecting many XML Security implementations, one specific
to the use of xslt in this library; both appeared to have been handled
quickly and with minimal patches available. While doing this review I
asked about the potential for the null prefix attack with the GnuTLS
and OpenSSL backends, and the author responded immediately, performed
his own review quickly, and responded clearly.

- xmlsec1 provides an XML Security interface to be used by other programs.
- Build-depends: debhelper, dh-autoreconf, chrpath, pkg-config,
  libxml2-dev, libxslt1-dev, libssl-dev, libgnutls28-dev, libnss3-dev,
  libgcrypt20-dev
- Requires GnuTLS, OpenSSL, and NSS.
- Does not itself do networking.
- Does not daemonize
- No customized pre/post inst/rm
- No init scripts
- No dbus services
- No setuid executables
- xmlsec1 and xmlsec1-config executables
- No sudo fragments
- No udev rules
- Extensive tests run during the build
- No cronjobs
- Some build warnings, mismatched types in printf format strings

- No subprocesses spawned
- Memory management looked careful
- Files to be opened are passed by clients
- Logging looked careful, extensive error reporting mechanisms
- No use of environment variables
- No privileged operations
- Extensive use of cryptography, only spot checks were possible, skipped
  all MS Cryptography; the checked sites looked careful.
- Only networking was in an example server not used in binary packaging
- No temporary file handling
- Does not use WebKit
- Does not use qtjsbackend
- Does not use PolicyKit
- Mostly clean cppcheck

This code is fairly dense and technically demanding as it lives in the
intersection of both XML and cryptography. The style is highly disciplined
with excellent error checking throughout, asserts and "friendlier" error
mechanisms are used extensively.

- gcrypt/hmac.c xmlSecGCryptHmacExecute() updates dgstSize rather than
  ctx->dgstSize -- is this correct?
- xkms-server.c main() fprintf format string bug
- dl.c getFunctions may be used uninitialized
- Some malloc() calls are casted, this isn't needed if a prototype is
  visible.
- xmlSecGnuTLSX509FindCert(), xmlSecOpenSSLX509FindCert(), among others,
  assume that subjectName, issuerName, are standard C strings rather than
  byte strings with a length attached. Author says this isn't a bug,
  the same user supplies both certificates and inputs.

Security team ACK for promoting xmlsec1 to main.

Thanks

[James Page (james-page)]

Michael

RE repoze.who; I'm not overly concerned at it being orphaned in Debian; the package is a little out-of-date but I think its manageable within the server team

I'll look at the xmlsec test suite/build failure issue soon

[Seth Arnold (seth-arnold)]

I reviewed python-pysaml2 version 2.2.0-0ubuntu2 as found in Ubuntu vivid.
This should not be considered a full security audit, but rather a quick
gauge of maintainability.

- python-pysaml2 is a middleware designed to handle SAML2 authentication,
  a competitor to oauth and FIDO. SAML2 is popular in enterprise
  environments.
- Build-Depends: debhelper, python-all, python-setuptools, python-sphinx,
  python-crypto, python-dateutil, python-decorator, python-mako,
  python-memcache, python-openssl, python-paste, python-pyasn1,
  python-pytest, python-pymongo, python-repoze.who, python-requests,
  python-tz, python-zope.interface, xmlsec1
- Does not itself daemonize
- Does not itself listen on external interfaces
- pre/post inst/rm are automatically added
- No initscripts
- No dbus services
- No setuid executables
- No sudo fragments
- No udev rules
- No cron entries

- Spawns subprocesses, looks careful
- Files opened under direction of controlling programs
- Logging looked careful, except for logged passwords
- No environment variables
- No privileged operations
- Extensive cryptography
- No privileged portions of the program
- No temporary files
- No webkit
- No javascript
- No PolicyKit

Here's some issues I discovered while reading this program:

- src/saml2/s_utils.py sid() provides highly-guessable session identifiers
- src/saml2/s_utils.py rndstr() strings are not cryptographically strong,
  appear to be used for cryptographic purposes
- src/sigver.py create_id() generated identifiers are not
  cryptographically strong
- example/idp2/idp.py, example/idp2/idp_uwsgi.y, example/aa/aa.py,
  example/idp2_repoze/idp, all have a staticfile() method that will serve
  every file on the computer that is readable by the server userid. No
  effort is made to filter out .. path traversals.
- example/idp2/service.py, example/idp2/idp.py, example/idp2/idp_uwsgi.py,
  example/aa/aa.py, example/idp2_repoze/idp.py all have password checks
  that do not attempt to prevent timing analysis.
- src/saml2/authn.py verify() will logger.debug() a password
- src/saml2/authn.py _verify() has a password check that does not attempt
  to prevent timing analysis
- example/idp2/service.py, example/idp2/idp.py, example/idp2/idp_uwsgi.py,
  example/aa/aa.py, example/idp2_repoze/idp.py info_from_cookie() do not
  handle TypeError exception from b64decode, will these provide a simple
  DOS attack vector?
- example/idp2/service.py, example/idp2/idp.py, example/idp2/idp_uwsgi.py,
  example/aa/aa.py, example/idp2_repoze/idp.py ecp() do not handle
  TypeError exception from b64decode, will these provide a simple DOS
  attack vector? This method also logs HTTP_AUTHORIZATION to
  logger.debug(), this may include passwords.

I reported the above issues to the author, who provided fixes for them
very quickly; he's inexperienced with CVEs but sounded willing to learn.

Please update the packaged version to include these fixes; I do not know
if they are security fixes, but it's plausible that some might be.
Security team ACK for promoting version 2.3.0 or higher to main.

Thanks

[Seth Arnold (seth-arnold)]

I reviewed python-repoze.who version 1.0.18-4 from Ubuntu vivid. This
should not be considered a full security audit but instead a quick gauge
of maintainability.

- python-repoze,who is a generic authentication middleware for python
  applications; it sits between a wsgi server and application and modifies
  http requests and responses.
- Build-Depends: debhelper, cdbs, python-dev, dh-python,
  python-setuptools, python-sphinx, python-zope.interface, python-paste
- Does not daemonize
- pre/post inst/rm scripts automatically generated
- No initscripts
- No dbus services
- No setuid executables
- No sudo fragments
- No udev rules
- No cronjobs
- Test suite run during the build

- No subprocesses spawned
- Files read under command of configurations
- Logging looked simple
- No environment variables used
- No privileged portions of code
- Networking driven by webserver
- Slight cryptography used, actual provided password storage mechanisms
  are weak
- No temporary files
- No webkit
- No javascript
- No policykit

While reviewing this code I found a few things that seemed worth reporting
here:

- ./repoze/who/plugins/htpasswd.py plain_check() function allows
  timing-based password discovery, crypt_check() hard-codes two character
  salt
- InsecureCookiePlugin doesn't appear to authenticate or encrypt the
  cookie data, or set httponly flag or set secure flag; ignoring the
  secure flag makes some sense for an InsecureCookie mechanism but lacking
  httponly and authenicated data is perhaps surprising to authors.
- doesn't appear to use HttpOnly cookie flag
- no csrf protection in default login form in repoze/who/plugins/form.py
- unknown session fixation prevention in default login form
- default_password_compare in ./repoze/who/plugins/sql.py does not
  salt or iterate passwords; plaintext variant allows timing-based
  password guessing, and stored passwords cannot start with (SHA)

I believe the core code of python-repoze.who is reliable enough, but
the default providers for backends and forms don't look like they are
production quality. Passwords are stored in plaintext, or insufficiently
salted and iterated, and timing-sensitive comparison routines are used.
The login form doesn't protect against session fixation or csrf. Simple
and usual protections on cookies are ignored.

This presents a dilemma; essentially, all non-toy programs have to provide
their own storage and authentication plugins to be able to safely use this
tool. It seems incorrect to promote a project to main with many known
flaws in the defaults, but if no real tools actually use the defaults,
the issues might be mostly academic.

The use by python-pysaml2 seemed safe enough.

The upstream authors have not yet responded to my questions. The above
issues may warrant security fixes, issues that would be best to fix
before shipment if we can. I'm concerned to hear that this package is
orphaned in Debian because it also feels orphaned upstream.

While we probably could take on maintenance of this package ourselves I
have to ask if we should use a different mechanism for login tracking.

So I propose a conditional ACK to promote this package to main,
conditional on two pieces:

*...

[Seth Arnold (seth-arnold)]

I got a response from Tres Seaver to some of the issues I raised in this MIR:

====
Thanks for the report! 1.0.18 is a long time ago now (almost 4 1/2
years). The latest release is 2.2, and there will likely be a 2.2.1
released in the near future.

We are pretty unlikely to make another 1.x release, unless you (or
somebody else) submits PRs for them (I just opened a '1.0-maintenance'
branch, in case someone wants to tackle it):

  https://github.com/repoze/repoze.who/tree/1.0-maintenance

Changes since 1.0.18 relevant to your issues:

- - Made `htpasswd' plugin more isochronous (2.1).

- - Deprecated plugins, moving them to a new 'repoze.who.deprecatedplugins'
  project (2.0a3):

  - 'repoze.who.plugins.cookie.InsecureCookiePlugin'
  - 'repoze.who.plugins.form.FormPlugin'
  - 'repoze.who.plugins.form.RedirectingFormPlugin'

On the trunk, the SQL plugin issues you report should probably get some
attention: I don't actually use it myself, which makes that trickier to
think about.

====

It would be nice to use a more up-to-date version of repoze.who. Nearly five years out of date already, it would be nice to avoid being eleven years out of date at the end of the next LTS release.

Thanks

[James Page (james-page)]

Seth

Bumping pysaml2 to 2.3.0 is probably not to much of a stretch this late in cycle, but repoze.who 1.0.18 -> 2.2 does feel like a big jump post freeze - esp as it has reverse-depends outside of this chain.

Keystone federation (requring pysaml2) landed as part of core in kilo-3 so will focus on this in the next week.

[James Page (james-page)]

Here's an idea - I'm not sure keystone is using the repoze.who feature, so we could disable this as a BD (and the assocated test) and push it back to Suggests.

[James Page (james-page)]

2.4.0 uploaded (as requested in security review) with repoze-who support disabled in testing and pushed back to a suggests at runtime.

Please can this MIR be reviewed on this basis.

[Michael Terry (mterry)]

What's the story with xmlsec1's tests? I remember that failing tests don't stop the build on that one.

[James Page (james-page)]

Michael

I've uploaded a new version of xmlsec1 which detects any failures in the test suite exection and skips any tests that would fail in offline environments.

I also had to limit some test execution to openssl only due to the way the xmlsec1 apps are built in Debian/Ubuntu (openssl only).

[Michael Terry (mterry)]

OK, xmlsec1 and python-pysaml2 both are fine from a security and maintainability sense then. Approving both.

[Seth Arnold (seth-arnold)]

Thanks James and Michael, looks good to me.

[Adam Conrad (adconrad)]

Promoted python-pysaml2 and xmlsec1 to main.