I reviewed thermald version 1.1~rc2-8 as checked into Ubuntu Trusty. This
should not be considered a full security audit, but rather a quick gauge
of maintainability.
- thermald provides a daemon to monitor machine temperature sensors and
modify different sysfs-exposed properties to control fans and processor
governors with the goal of keeping the temperature in hysteresis.
- Build-depends: debhelper, dh-autoreconf, quilt, autoconf,
libglib2.0-dev, libdbus-1-dev, libdbus-glib-1-dev, libxml2-dev,
dh-systemd
- No off-machine networking
- No cryptography
- Provides a daemon that polls status files and provides a dbus service
- Runs as root
- Uses g_main_loop_run() to start daemon
- Doesn't umask(), doesn't chdir(), doesn't fork(), doesn't setsid(),
doesn't close descriptors, doesn't open /dev/null to 0, 1, 2.
- Pre, post install and rm scripts looks proper
- DBus service appears to allow changing between PERFORMANCE,
ENERGY_CONSERVE, and DISABLED
- No setuid executables
- One binary in /usr/sbin/thermald
- No sudo fragments
- No test suite
- No cronjobs
- No subprocesses are spawned
- Most memory allocations are checked, memory leaks are possible via error
conditions
- Files are written to:
- static preference file, awkward code but looked alright
- sysfs files, looked alright
- android_main.cpp writes a pidfile in /tmp unsafely, should receive a CVE
- Most file paths are roughly hardcoded and computations on file paths
assume a trusted kernel is providing the filesystem non-maliciously
- Logging functions looked safe
- No use of environment variables
- All code executes with privilege, no management of privileges
- No cryptography
- The only networking is for netlink and dbus
- Aside from the android_main.cpp pid file, no "temporary" files
- No WebKit
- No PolicyKit
The code looks to be of moderate quality; the amount of leaked resources
is surprising.
There's more than the usual amount of casting numbers between integers
and floating point that has historically been a rich source of bugs. This
is potentially troubling, given that there's at least one location in
the daemon that will forcibly reboot the device if a sensor measurement
is out-of-bounds. None of the integer inputs use the safer stroul() family
of functions, instead using atoi(), which does not do any error handling.
I didn't discover any mechanism to authenticate dbus-initiated changes of
status -- any process able to connect to the dbus system bus should be
able to instruct this daemon to switch performance profiles.
Fair warning that the assembly in cthd_engine::check_cpu_id() is beyond my
abilities to audit: I do not know what it does.
Here's a list of issues I found, in the hopes that someone will take the
effort to fix them:
- android_main.cpp chdir(/tmp) at startup, preventing /tmp from ever being
umounted
- android_main.cpp daemonize() uses O_CREAT without O_EXCL for the pidfile
daemonShutdown() doesn't unlink the pidfile
Why is the pidfile stored in /tmp anyway?
- cthd_engine::thd_read_default_thermal_sensors() memory leak in
!= THD_SUCCESS branch
- cthd_engine::thd_read_default_thermal_zones() memory leak in
!= THD_SUCCESS branch
- cthd_engine::thd_read_default_cooling_devices() memory leak in
!= THD_SUCCESS branch
- cthd_engine::get_sensor() doesn't protect against negative indexes
- cthd_engine::get_zone() doesn't protect against negative zones
(function appears unused though)
- cthd_kobj_uevent::check_for_event() appears to be very complicated
re-implementation of strstr(3)
- cthd_engine::takeover_thermal_control() variable type_val looks useless
- cthd_engine::giveup_thermal_control() variable type_val looks useless
- cthd_engine_default::read_thermal_sensors() memory leaks in !THD_SUCCESS
branches for coretemp, hwmon sensors
- csys_fs::write() fd leak via ::lseek() error return
- csys_fs::read() fd leak via ::lseek() error return
I'd personally look for a different tool, this feels complicated and
heavy for the task at hand, however nothing here is serious enough to
justify keeping this tool out of main. Security team ACK for moving
thermald to main.
Thanks
** Changed in: thermald (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/1279388
Title:
[MIR] thermald
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/thermald/+bug/1279388/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
