[MIR] ppc64-diag needed in minimal for hotplug capabilities
Bug #1417608 reported by
Adam Conrad
This bug affects 2 people
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
iprutils (Ubuntu) |
Fix Released
|
High
|
Unassigned | ||
libservicelog (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned | ||
libvpd (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned | ||
lsvpd (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned | ||
ppc64-diag (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned | ||
servicelog (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
IBM has requested that ppc64-diag be installed by default to support hotplug features on ppc64el kit. Packaging, security history, and upstream maintenance all seem fine, but I'd like a quick security review before we go about promoting it for them.
NOTE: This MIR is meant to be retroactive all the way back to trusty, but if the conclusion of the review is that trusty's versions are unsupportable, we can backport whatever acceptable versions we end up with in wily back to trusty through vivid, so long as they meet general SRU/HWE criteria (which they generally have done up until now).
Changed in ppc64-diag (Ubuntu): | |
assignee: | nobody → Ubuntu Security Team (ubuntu-security) |
description: | updated |
Changed in iprutils (Ubuntu): | |
importance: | Undecided → High |
Changed in iprutils (Ubuntu): | |
assignee: | Adam Conrad (adconrad) → Matthias Klose (doko) |
To post a comment you must log in.
I reviewed ppc64-diag version 2.6.7-0ubuntu2 from vivid. This shouldn't be
considered a full security audit but rather a quick gauge of
maintainability.
The collection of daemons and tools in this package read
platform-dependant information and report it for sysadmins to use.
The code quality was widely variable; almost all functions checked error
returns correctly and the handful of exceptions are common. However,
many complicated idioms are used in place of simpler functions such as
strdup(3), error handling often neglects to clean up acquired resources,
and unsafe shell-based execution mechanisms are used pervasively.
This code would benefit from removing almost every use of system(3) and
popen(3); some cases can be easily replaced with small wrappers around
fork(2) and execve(2) but some should be replaced with C-native functions
such as rename(2), unlink(2), and nftw(3).
Some of the programs process data from /var/log/messages or
/var/log/syslog; both of these files should be considered hostile as
it is trivial for potentially malicious local users to inject nearly
arbitrary contents into the logs. Because the programs run as root without
dropping privileges they must demonstrate strong data hygiene.
While the code is straightforward enough that the security team could fix
issues as they are raised, there's probably a week's work to handle this
(non-exhaustive) list of issues.
These issues may be security relevant or lead to unreliable behaviour:
- print_mtms_struct() write beyond the end of the model buffer, may not errd_dir( ) can leak errd_path via two early returns to_drcname( ) leaks dir in final return phandles( ) leaks fd via early return :compute_ regex_text( ) may have shell injection via popen(), fork()+ exec(), no
trip stack protection mitigations
- servevent() can leak entry on encl == NULL case
- safe_overwrite() can leak f on fputs(data.c_str(), f) == EOF case
- write_log() may not NUL-terminate dump_suffix, appears to read the data
from a file on disk
- find_opal_
- process_elog() can leak buf, output_dir via early returns
- mem_drcindex_
- get_dt_status() leaks fp1, fp2 via early returns
- add_drconf_
- MatchVariant:
hard to verify that it's not an issue
- tail_message_file() shell injection; bad_chars misses many shell
metachars. The least-effort patch involves quoting all ' chars in the
input then wrapping it with ' chars. (The better approach is to use the
array-based exec() with manual pipe-assembly, but this is error-prone
code to write. Given how many places use popen() in this code, it would
be a good investment.)
- get_dt_status() hardcoded /tmp/get_dt_files filename, unsafe on systems
without symlink and hardlink restrictions -- seems like a lot of work to
avoid using nftw(3)
- get_ses_indicator() uses popen() rather than pipe()+
protections against shell metachar injection via loc->dev or fru_loc
- set_ses_indicator() uses system() rather than fork()+exec(), no
protections against shell metachar injection via loc->dev or fru_loc
- loc_code_device() may have shell injection via popen(); the parameter
may come from argument parsing in main(), but if the encl_led tool is
used by other tools,...