I reviewed openwsman version 2.3.6-0ubuntu1 as checked into Trusty. This
should not be considered a full security audit, but rather a quick gauge
of maintainability.
The build depended upon libcimcclient0-dev from universe source package
sblim-sfcc; does this package have a MIR already underway?
- It's difficult to figure out what this package does. It bundles a
webserver, manages plugins, generates XML, generates SWIG bindings, but
the overall purpose is difficult to discern either from source code
analysis or from reading documentation.
- Build-depends on cmake, libssl, libpam0g, libxml2, libcurl4-opeenssl,
libcimcclient0-dev, swig, python-dev (ruby bindings are disabled)
- Uses OpenSSL in the embedded web server; does no certificate validation
- Uses libcurl to load remote resources; does not correctly validate
hostnames
- Uses an embedded version of shttpd to serve HTTP and HTTPS
- Provides a pam module that can be used for authentication
- shttpd daemonizes correctly
- Simple pre/post inst/rm scripts are automatically generated content
- No initscripts
- No setuid executables
- No Dbus services
- openwsman package provides openwsmand, owsmangencert binaries
- No sudo fragments
- No udev rules
- Test suite, not enabled during build
- No cron jobs
- Build logs are clean but only by accident -- most useful compiler
warnings are disabled, very few files are compiled with -Wall or
-Wextra. Many oddities in the code could have been caught by asking the
compiler for help.
- Subprocesses are spawned, looked careful
- Some memory management is overly complicated, much string manipulation
code is awkward, some allocations are used without checking.
- Files are written to, many might be under control of remote systems via
the web server
- Most logging functions were safe, only unsafe debug() is stubbed out
- Environment variable handling looked safe
- No privileged portions of code
- Extensive use of networking, looked safe
- No temp file handling
- No WebKit
- No PolicyKit
The code quality is highly variable. This codebase was cobbled together
out of several different projects and while some portions, particularly
the embedded webserver, look good, much of the code base is rough and
unpolished. There are many odd decisions throughout, and some of them
would have been reported by the compiler's built-in warnings, if only
they were enabled.
Many string handling functions allocate more memory than is necessary.
String duplicates are created and destroyed often. Memory management is
awkward with heap storage being used for objects with only call-chain
lifetimes. This significantly complicated looking for errors because usual
idioms are not observed.
Some memory allocations are used without checking for success. Very
rarely are structures zeroed after allocation -- and the plugin-based
architecture makes it very difficult to determine if there are information
leaks due to incomplete object initialization or structure holes.
Libcurl has a poor design -- to properly verify host names against X.509
certificates, a parameter must be set to 2. Due to this bad design,
libcurl versions greater than 7.28.0 return an error if the value 1 is
passed as a parameter. This library has made this mistake. Due to this
mistake, I suspect no one has executed this code in this package in
Raring, Saucy, or Trusty, otherwise this bug would have been discovered
earlier. In 10.04 LTS, 12.04 LTS, and 12.10, this library simply does
not perform hostname validation as it intended.
Overall, this code feels like it was debugged into existence and works
"just well enough" to be thrown over the fence to the next team. Much
of the code is complicated and shows a lot of motion but it is far from
clear what a lot of it does. It does not feel like this project has had a
curator to tend to it, and simple quality steps like turning on compiler
warnings are completely lacking, let alone a comprehensive test suite,
valgrind runs, or Coverity scans.
The package has not been modified since 13.04. It is not packaged in
Debian.
As this package currently stands, we cannot include it in main.
However, I understand the importance of CIM to some ecosystems, and
I can understand that this package may be the easiest way to provide
CIM infrastructure. I would be willing to accept this package if the
following issues are addressed:
- ws_xml_make_default_prefix() can overflow buf parameter via sprintf()
- wsmc_create_request() potential buf[20] overflow via WSMAN_ACTION_RENEW
- Note especially that %f formatting of -DBL_MAX is 317 bytes long
- init_curl_transport() uses the cl->authentication.verify_host value
directly when setting CURLOPT_SSL_VERIFYHOST. wsmc_create() sets
verify_host=1. curl_easy_setopt() should throw an error return in 13.04,
13.10, and trusty, because the only legal values in those versions are
'0' and '2'. (In earlier releases, '1' was a debugging option that led
to no shortage of bugs.)
- redirect.c plugin leaks memory from redirect_data struct, and depending
upon details of callers, may return uninitialized memory to callers
- LocalSubscriptionOpUpdate() unchecked fopen()
- Incorrect order of sanity guards in wsman_get_fault_status_from_doc()
- Unchecked memory allocation in wsman_init_plugins(), p->ifc
- Unchecked memory allocation in mem_double(), newptr
- Unchecked memory allocation in dictionary_new(), d, d->val, d->key, d->hash
- Unchecked memory allocation in u_error_new(), *error
- SSL layer appears to neglect checking peer certificate validity and peer
hostname, this may or may not be an issue depending upon threat model
I did not follow the callchains of the buffer overflows far enough to
determine if the variables may be under control of either data from
another computer system or another execution domain on the same system.
Please let us know if we need to request CVEs for these issues.
I do not know the architecture well enough to determine if the incorrect
libcurl hostname validation is a security problem. Please let us know if
we need to request a CVE for this issue.
The following issues would be nice to have addressed:
- authorize() in file_auth.c can only use traditional DES, MD5, SHA-256;
not enough storage space is allocated for SHA-512 password hashes
- authorize() in file_auth.c uses strcmp() to compare usernames and
password hashes in a fashion that allows an attacker to brute-force
learn both usernames and password hashes one character at a time.
- vfork() has been removed from POSIX and used incorrectly in compat_unix.c
- sighup_handler() in wsmand.c uses unsafe functions in a signal handler
- dispatch_inbound_call() unsafe debug(buf) call -- #if 0
- LocalSubscriptionOpGet() could be a simple stat, malloc, read
- LocalSubscriptionOpLoad() could be a simple stat, malloc, read
- Consider replacing most malloc(), u_malloc(), etc. calls with a call to
calloc() or u_calloc() to prevent information leaks
- Consider turning on -Wall -Wextra and fixing the warnings
- Consider creating a test suite that can run during build
- Consider running the test suite under valgrind
- Consider submitting the code to Coverity
Security team NAK for including into main; feel free to re-open when the
above list of issues has been addressed.
Thanks.
** Changed in: openwsman (Ubuntu)
Assignee: Seth Arnold (seth-arnold) => (unassigned)
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1262299
Title:
[MIR] libopenwsman1
To manage notifications about this bug go to:
https://bugs.launchpad.net/dell-poweredge/+bug/1262299/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
