[MIR] libestr

[Steve Langasek]

The new upstream version of rsyslog found in Debian unstable depends unconditionally on libestr. As a string handling library that will be used by a privileged process, this is a fairly security-sensitive library.

http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=libestr and http://people.canonical.com/~ubuntu-security/cve/universe.html show zero CVEs for this package, but as a little-known library that's only been around for 3 years, a more thorough security audit is probably needed. The source does build cleanly with -Werror -Wall, which is a hopeful sign.

The package has no other dependencies.

[Seth Arnold (seth-arnold)]

NAK on libestr 0.1.5-2.

While auditing this code I discovered a flaw in the es_unescapeStr() function -- luckily, it's been discovered and fixed in 0.1.6: http://libestr.adiscon.com/uncategorized/libestr-0-1-6-2/

The flaws fixed in newer versions look too important to only fix this one issue.

The overall code quality looks decent, despite the sobering entries in the changelog, so I'm inclined to accept a future version.

A unit test suite is sorely lacking; even simple tests of es_unescapeStr() should have found the flaws I discovered by manual inspection.

Thanks

[Seth Arnold (seth-arnold)]

I reviewed libestr version 0.1.9-0ubuntu1 as checked into trusty. This
should not be considered a full security audit, but instead a quick gauge
of code quality.

Thanks for the fast upgrade.

- libestr provides a (length, data) string object and associated functions
  for C to address the anemic standard C string handling routines. It
  provides its own versions of many of the C standard routines.
- Build-depends upon autotools
- No networking
- No cryptography
- No daemons
- No pre/post inst/rm scripts
- No init scripts
- No dbus services
- No setuid binaries
- No binaries
- No sudo fragments
- No udev rules
- No cron jobs
- Clean build logs
- No test suite; the last several releases have changelog entries that
  indicate problems that would have been found with even small,
  functionality-only, unit test suites.

- No spawned subprocesses
- Sane memory management
- No file IO
- No logging
- No environment variables
- No privileged operations
- No cryptography
- No networking
- No temporary files
- No WebKit
- No JavaScript
- No PolicyKit

The code is decent enough, though some fairly simple coding errors have
been fixed in the last few releases, and while auditing 0.1.9 I discovered
that flaws fixed in a previous release were not completely repaired.

This code is not suitable for use with completely arbitrary inputs;
there are no preventions in place for signed integer overflows. This
library is intended to handle human-sized strings, not fully arbitrary
inputs. This might be fine for e.g. rsyslog, which is unlikely to ever
handle an input larger than ethernet's MTU of 1500 or 9000 bytes, give or
take, but other code may not understand the limitations of this library.
Billions of bytes is outright not acceptable.

This code sorely needs a test suite. In the 0.1.5 version, I found
three or four different faults in one function, and the newer version
0.1.9 version had only fixed one of the problems. At least two of the
faults would have been discovered by only simple tests. A similar story
exists for a different function (also from the older 0.1.5 version),
with a significant functional bug and a further limitation that would
have been obvious to a test suite author.

There's little enough code here that we can take on maintenance of this
codebase if necessary, but I fear that the complete lack of testing might
freeze a broken ABI in place for years to come. We should prepare a
test suite for this package well in advance of code freeze. (I know we
don't block MIR requests for missing test suites, and I expect it feels
like I'm harping on about this, but I feel that some of the code in this
library has never been executed and had its output examined.)

Security team ACK for including in main despite the above.

Thanks

[Michael Terry (mterry)]

Approved. From a packaging side, things seem OK. Bug subscriber, no important bugs, small delta (just a version bump). I share Seth's concerns that this really needs tests. But if Seth ACKs, that's enough for me.

We have actually pushed a patch for DEP8 tests on to Debian (bug 1117222). So hopefully that can start a Debian-side test suite.

[Jeremy Bícha (jbicha)]

libestr has been in main for a while so I'm closing this bug.