Comment 1 for bug 1417608

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
  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_errd_dir() can leak errd_path via two early returns
- process_elog() can leak buf, output_dir via early returns
- mem_drcindex_to_drcname() leaks dir in final return
- get_dt_status() leaks fp1, fp2 via early returns
- add_drconf_phandles() leaks fd via early return
- MatchVariant::compute_regex_text() may have shell injection via popen(),
  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()+fork()+exec(), no
  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, they may not expect it to have shell injection
  problems
- read_vpd_from_lscfg() may have shell injection via popen(); it is
  difficult to determine if it is safe from the call history
- platform_log_write() does not prevent buffer overflows of buf; with this
  many callers, it's hard to verify them all for correctness.
- write_prrn_log(), close_prrn_log() test if (prrn_log_fd) -- but these
  tests will always succeed unless prrn_log_fd _is_ opened and gets fd 0.
  open_prrn_log() doesn't set prrn_log_fd to 0 in case of failure.
- check_scanlog_dump() sets 0700 on the scanlog filename at open(2) but
  then changes permissions to 0644 using the filename; this is a racy way
  to set the permissions. fchmod(2) wouldn't race, but open(2) could set
  the permissions correctly. Why are they set to two different values?
- log_msg(), _log_msg(), rotate_log_file() use system(3), rm(1) and mv(1)
  to perform log rotation rather than unlink(2) and rename(2)
- config_restart_policy() uses brittle system() == -1 to probe for
  features
- In C, no need to cast the return value from malloc(3), calloc(3) --
  casting it can hide errors, it'd be safer to remove all those casts.
- The daemons do not drop privileges

These issues would lead to simpler code or fix little mistakes:

- process_pre_v6(), report_srn(), get_loc_code() could use strdup(3) rather
  than malloc(strlen()), strcpy()
- set_srn_and_callouts(), check_scanlog_dump() could use strdup(3) rather
  than malloc(strlen()), strncpy(strlen())
- get_machine_serial() could use strdup(3)
- Non-FHS path /usr/sbin/rsct/bin/refrsrc
- rtas_errd/cscope.out and rtas_errd/cscope.files shouldn't be in the
  source tarball

We cannot support ppc64-diag in main as it is currently; ppc64-diag could
be supportable if most of the above issues are addressed. (Dropping
privileges is probably a long-term goal; adding that may or may not be
feasible.)

Please feel free to re-open when a new version is available.

Thanks