I reviewed pcp version 3.12.0build2 as checked into artful. This should
not be considered a full security audit but rather a quick gauge of
maintainability.

PCP has a long history of being the go-to-package for performance
monitoring, especially across a network.

There's five CVEs in our database; CVE-2012-3418 is multi-purpose and
addresses what feels like twenty or so individual issues. The issues
that it raises feels very similar to what I found while reviewing pcp.

I should stress that the threat model of PCP may mean that nothing
I've found is actually a security issue -- however, the coding style
is what might have been appropriate twenty years ago but is no longer
appropriate today.

Much of what I read was using C as shell scripts. This is marginally
suitable in the case of PMDAs calling programs to collect statistics
but is far from ideal for the more structural components.

None of the snprintf() calls checked for error returns, even when the
inputs come from outside the process. Overflows could cause surprising
results as truncated commands are executed.

There are at least half a dozen functions that serve as system()
or popen() replacements or wrappers. I also saw several functions that
serve as execve() wrappers; I didn't annotate those, since they are less
likely to be a source of problems. Since most of the strings are built
from pieces anyway the much safer execve() style of execution should be
used instead. (system() and popen() are really best suited strictly for
instances of users supplying exact commands they want run.)

There are several cases of unchecked sprintf() calls that could overflow
buffers if provided with unsafe input. Again, I'd like to stress that
these may not be security flaws in the threat model of PCP but their
presence is worrying.

cppcheck reports roughly 117 issues. Some may be false positives and
some may have very limited impact (e.g. 78 instances of realloc()
mis-use that leaks memory on memory allocation failure) but this is an
easy source of objectively useful things to fix.

The documentation is superb. I was excited reading about PCP and wanted
to use it myself. I really do hope the PCP team can make the big changes
I feel are necessary for today's environments.


Security team NAK for promoting pcp to main.


Here's the notes I collected in the hopes that they are useful for
someone:


command executors
- start_cmd from src/pmdas/logger/util.c
- start_cmd from src/pmdas/pipe/util.c
- pmmgr_daemon::poll
- pmmgr_configurable::wrap_system
- do_shell
- local_pipe

collectl2pcp unsafe filename input mechanism to popen()

watch() from ./src/dbpmda/src/util.c unsafe filename use

pcpScript() unsafe system() via name and action; unchecked snprintf() call

lex() from ./src/libpcp/src/pmns.c unsafe bin_dir, fname use

popen_uncompress() from ./src/libpcp/src/logutil.c unsafe cmd, fname, pipecmd use

pmflush() from ./src/libpcp/src/util.c unsafe fname use

OpenViewDialog::openView() from ./src/pmchart/view.cpp unsafe PCP_VAR_DIR
and path use

OpenViewDialog::openView() _fname buffer overflow if path parameter is >
MAXPATHLEN

OpenViewDialog::openView() cmd buffer overflow if _fname is > MAXPATHLEN

main() from ./src/pmcpp/pmcpp.c may intentionally or not allow unsafe
shell use

write_pmiefile() doesn't check snprintf() for error return before calling
system() on the results; possible shell injection via fname, probably via
PCP_SYSCONF_DIR or HOME environment variables

pmmgr_job_spec::poll() calls out to /bin/rm -rf

pmmgr_pmlogger_daemon::daemon_command_line() calls out to /bin/rm -f

pmmgr_pmlogger_daemon::daemon_command_line() unsafe config
pmlogmerge-rewrite use

pmmgr_pmie_daemon::daemon_command_line() unsafe config pmieconf use

pmpython executable allows someone who can set PCP_CONF or PCP_DIR
environment variables to execute anything in the filesystem; this may be
more power than one might expect.

do_dialog() in ./src/pmlogger/src/pmlogger.c doesn't check snprintf()
return values, unsafe rsc_prog and msg use

SignalShutdown() from ./src/pmcd/src/pmcd.c uses signal-unsafe routines

ib_load_config() could execute the 'cp' parameter, which comes from
ibpmda_init() which doesn't do snprintf return value checking; the
parameter from main() could also be executed, which seems surprising for a
configuration file