[Bug 1349868] Re: [MIR] new build dependencies for ceilometer

[Seth Arnold]

I reviewed libsmi version 0.4.8+dfsg2-9ubuntu2 as checked into utopic.
This should not be considered a full security audit but rather a quick
gauge of maintainability.

CVE history: CVE-2010-2891

- libsmi provides bindings to manipulate OIDs stored in MIB files
- Build-Depends: flex, bison, debhelper, dh-autoreconf
- No cryptography
- May call wget or similar via smicache mechanism; I believe this isn't
  enabled by default in our packages
- Does not daemonize
- No pre/post inst/rm scripts
- No initscripts
- No dbus
- No setuid
- No sudo fragments
- No cronjobs
- No udev rules
- Binaries smistrip, smicache, smidump, smilint, smiquery, smixlate,
  smidiff
- Test suite isn't run during build
- Build logs are fairly messy
- binaries not PIE

- Subprocesses only spawned for smicache
- Memory management is hectic
- The individual tools will write to files designated by the user
- The logging looked safe
- The environment variable uses looked sane
- No privileged operations
- No cryptography
- May call wget to download MIB files
- No temporary files
- No WebKit
- No PolicyKit
- A handful of errors from cppcheck, some common, some surprising

Here are some notes I've collected while reviewing the code in the hopes
someone finds them useful:

- getOidString() very complicated, no protection against overflowing 's'
  buffer. I suspect bugs live in this function.
- parseDH() very complicated, I suspect bugs live in this function. case
  '*' at least seems to hide memory leaks.
- printClass() no protection against overflowing 'string' buffer
- fprint() missing error return check on 'fputs()'
- getValueString() no protection against overflowing 's' buffer
- optString() may not nul-terminate the return string
- getStringIndexList() insufficient allocation for strIdxLst, uses +4 but
  should use +5, one-byte buffer overflow overwrites space for nul. This
  function may work by accident.
- getStringSubrange() memory leak minStr, maxStr
- getStringRange() memory leak str, subRange
- undefined behaviours (sprintf(dest, "%s", dest))
- many warnings
- smidiff.c:840:1: note: the ABI of passing union with long double has
  changed in GCC 4.4

This codebase is pretty messy; there's duplicated code in multiple files
rather than using shared utility files; there's awkward uses of C string
routines, there's extensive memory allocation and re-allocation when
manipulating strings, there's extensive use of fixed-length buffers
without visible bounds checking, etc.

There are doubtless many bugs left in this library; cppcheck has reported
some, gcc reports many warnings, and there is a lot of room for
improvement. It really needs something like a StringBuffer-style
datastructure to replace the extensive C-string operations.

However, despite my large misgivings about this library, by and large it
should process mostly-static data from trusted sources. In this role its
flaws may not be a big deal.

Please enable build-time tests. Please enable PIE for the binaries.

Please ensure that ceilometer does not need the smicache functionality;
this library should not be used on unauthenticated data.

Security team ACK for promoting libsmi to main.

-- 
You received this bug notification because you are a member of Ubuntu
Server Team, which is subscribed to the bug report.
https://bugs.launchpad.net/bugs/1349868

Title:
  [MIR] new build dependencies for ceilometer

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/libsmi/+bug/1349868/+subscriptions