I reviewed libsmbios version 2.3.1-0ubuntu1 as checked into Yakkety. This
should not be considered a full security audit but rather a quick check of
maintainability.
- libsmbios provides libraries and utilities for working with Dell BIOS
- No CVEs in our database
- The library is used by fwupd and fwupdate
- Build-Depends: debhelper, autotools-dev, libcppunit-dev, doxygen,
graphviz, python, chrpath, libxml2-dev, pkg-config, autoconf, automake,
libtool, autopoint
- provides the following binaries and symlinks in /usr/sbin:
-rwxr-xr-x root/root dellBiosUpdate-compat
-rwxr-xr-x root/root dellLEDCtl
-rwxr-xr-x root/root dellMediaDirectCtl
-rwxr-xr-x root/root smbios-battery-ctl
-rwxr-xr-x root/root smbios-get-ut-data
-rwxr-xr-x root/root smbios-keyboard-ctl
-rwxr-xr-x root/root smbios-lcd-brightness
-rwxr-xr-x root/root smbios-passwd
-rwxr-xr-x root/root smbios-rbu-bios-update
-rwxr-xr-x root/root smbios-state-byte-ctl
-rwxr-xr-x root/root smbios-sys-info
-rwxr-xr-x root/root smbios-sys-info-lite
-rwxr-xr-x root/root smbios-thermal-ctl
-rwxr-xr-x root/root smbios-token-ctl
-rwxr-xr-x root/root smbios-upflag-ctl
-rwxr-xr-x root/root smbios-wakeup-ctl
-rwxr-xr-x root/root smbios-wireless-ctl
lrwxrwxrwx root/root dellBiosUpdate -> smbios-rbu-bios-update
lrwxrwxrwx root/root dellLcdBrightness -> smbios-lcd-brightness
lrwxrwxrwx root/root dellWirelessCtl -> smbios-wireless-ctl
lrwxrwxrwx root/root getSystemId -> smbios-sys-info
- I don't believe any packages daemonize
- pre/post inst/rm scripts autogenerated
- No initscripts / systemd units
- No dbus services itself
- No setuid executables
- No sudo fragments
- No udev rules
- The test suites are not run during build -- please do so
- Build logs have hundreds of warnings
- No subprocesses spawned in C/C++ code
- Subprocesses spawned in Python BackCompatRbu::doUpdate() looks unsafe
but I think this is dead code
- Memory management is extremely difficult to decipher
- "memory factory" is way too much trouble for whatever benefits it may
provide
- many C++ operator new allocations are checked for NULL returns when
those would instead throw an exception
- many memory-failure error pathways try to use facilities that would
fail when memory is scarce (gettext especially)
- I suspect copy_mmap() / trycopy() / remap() is probably not safe
- files usually passed in as parameters, sometimes writing to current
working directory
- logging looked fine, assuming the debug logs aren't enabled
- Environment variable handling looked fine
- Uses iopl()
- No cryptography
- No networking
- Portions that use iopl are very privileged; what felt like minimal
sanity checks before using iopl() are used first
- No files in /tmp
- No webkit
- No javasript
- No policykit
- One cppcheck error for Windows:
- [src/libsmbios_c++/memory/Memory_Windows.cpp:336]: (error) Mismatching
allocation and deallocation: MemoryAtRequestedOffSet
Here's my notes collected while reading the sources:
- /etc/yum/ -- etc/yum/pluginconf.d/dellsysid.conf is packaged in
smbios-utils
- many binaries without manpages
- lintian warning about embedded js libraries in docs
- memory_obj_factory() appears to leak toReturn if either
init_mem_struct_filename() or init_mem_struct() return failure
- smbios_table_factory() appears to leak toReturn if init_smbios_struct()
fails
- ./src/bin/biosacpi.c main() assumes well-formatted input file; it looks
vastly unsafe for 'arbitrary file' access.
- _init_cmos_std_stuff() (and elsewhere) try to use gettext() (via _()) in
many error paths, including out-of-memory errors. This seems prone to
failure.
- init_mem_struct_filename() strlen(errbuf) isn't protected by the
if (errbuf) guard
- I'm just plain baffled with memory_obj_factory() and friends
- I'm just plain baffled with dell_smi_factory() and friends
- There's more const_cast<> and reinterpret_cast<> than I'm accustomed to
seeing in C++ code
- There's a few hundred uses of deprecated std::auto_ptr.
- Some calls to new check result for non-NULL; but C++ throws an exception
when 'new' fails (unless new (std::nothrow) is used); it might be worth
re-examining the uses of 'new' to see if the std::nothrow variant should
be used instead, or if the throw variant is still intended.
- cmos::readByteArray() calls fopen()/fseek()/fclose() once for every byte
that is read
- cmos::writeByteArray() calls fopen()/fseek()/fclose() once for every
byte that is written
- CmosTokenD5::setString() allocates targetBuffer and I can't figure out
why
- I suspect copy_mmap() / trycopy() / remap() is probably not safe
- SmiArchStrategy::lock() flock() call lacks error handling
- SmiArchStrategy::lock() the writes are not flushed; they may not happen
until fclose() is called in the destructor.
- Extensive references to Xerces in sources even though xerces-c
apparently hasn't been needed for nine years?
This package is a confusing amalgamation of C, C++, Python, baffling
"memory object factory", probably wasteful optimizations, extremely
wasteful extra complexity. I think it could have been a quarter the size,
or less, if three programs for three architectures been written in one
language instead of one tool to handle Windows, Solaris, and Linux,
in three languages. I think a simple C toolkit with shims to C++ and
Python would have been a lot less work.
The extreme number of const_cast, static_cast, and reinterpret_cast
calls suggests that this code was written hastily, made to work just
well enough, and then left unmaintained since. The compiler warnings
reinforces the impression that this package is severely unloved.
auto_ptr was deprecated in C++11 and C++17 removes it entirely. Fixing
this package to use the replacement pointer types will probably take
some time. We cannot support this package indefinitely in its current
state. Upstream needs to invest in reducing the outstanding technical
debt.
This was a difficult choice; Mario has convinced me that this package is
used in significantly restricted capacities, so its many oddities and
cruft may not represent as much attack surface as it looks like. I
recognize that this code does work for its purpose but I'm worried that
making it run in an automated fashion is beyond its purpose.
There are tests in src/cppunit and src/pyunit that don't appear to be
run during the build. Please enable these and make sure that they fail
the build if tests fail.
Security team ACK for promoting libsmbios to main is given once a new
package is checked into the archive with tests enabled and passing.
Thanks
[I'd like to assign some homework for upstream:
- compile with UBSAN and make sure it's clean
- compile with ASAN and make sure it's clean
- fix the readByteArray() / writeByteArray() to work on more than one byte
at a time
- replace auto_ptr with the correct replacements at each use
- remove the factories altogether
- refactor copy_mmap() / trycopy() / remap()
- try afl-fuzz on the different tools
If the above steps are taken with a view towards removing code as a side
goal, I think this project could be a lot easier to maintain. It won't be
easy to get there.]
** Changed in: libsmbios (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/1603072
Title:
[MIR] libsmbios
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/libsmbios/+bug/1603072/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
