Comment 4 for bug 1704130

I reviewed papi version 5.6.0-1 as checked into bionic. This should not be
considered a full security audit; in fact I stopped part-way through the
process after discovering sufficient grounds to decide that the Ubuntu
security team does not want to support this package at this time. Perhaps
with some cleanup we can re-address this package for a future cycle.

I believe there is a mistake in the Debian packaging:

- 'grep -C5 recipe' on the build log shows several failed build targets,
  but the build as a whole does not fail.

Here's some of the issues I found while reading the sources. Many of
these may not be security issues but suggest that PAPI was not intended
to be used on computers that may have malicious actors.

- xscale_iresume() buffer overflow of stack variable pmc.

- scan_prtconf() in ./src/solaris-ultra.c uses tmpnam(3) rather than
  mkstemp(3).

- _mx_init_component() in ./src/components/mx/linux-mx.c executes
  CWD-relative program if mx_counters isn't in PATH

- find_pmu_type_by_name() in ./src/libpfm4/lib/pfmlib_powerpc_perf_event.c
  may overflow its buffer if a caller provides a too-long name. Callers
  are dynamic enough that this needs its own check.

- find_pmu_type_by_name() in
  ./src/libpfm4/lib/pfmlib_intel_x86_perf_event.c
  may overflow its buffer if a caller provides a too-long name. Callers
  are dynamic enough that this needs its own check.

- find_pmu_type_by_name() in
  ./src/libpfm4/lib/pfmlib_amd64_perf_event.c
  may overflow its buffer if a caller provides a too-long name. Callers
  are dynamic enough that this needs its own check.

- pfmlib_build_event_pattrs() in ./src/libpfm4/lib/pfmlib_common.c runs
  the risk of an integer overflow in a malloc() call, calloc() would be
  safer

- main() in ./src/utils/papi_cost.c exposes an integer overflow malloc()
  call directly to the command line arguments.

- main() in ./src/utils/papi_multiplex_cost.c exposes an integer overflow
  malloc() call directly to the command line arguments.

- main() in ./src/utils/papi_command_line.c exposes an integer overflow
  malloc() call directly to the command line arguments.

- uninitialized variable 'nr' in src/perfctr-2.7.x/usr.lib/x86.c -- an
  #else #error case would be very kind

There's many cases of malloc(foo * bar); in the sources; a few I inspected
looked safe, a few weren't obvious, and some were obviously dangerous (as
above). Basically all of these should be replaced with calloc() to avoid
having to do further analysis.

There's a few cases of system() or popen() in the sources. A few of the
uses looked fine, a few looked like they deserved more hardening. I
don't think any outright need converting to execve() or libpipeline,
but I didn't inspect the callers sufficiently to be sure.

It appeared to be a recurring pattern to use sprintf() to a buffer
without checking for success; many cases did appear to constrain
referenced strings to fit within the available buffer space, but the
end result may not always be well-formed.

The full cppcheck output is 92 lines. Even after removing everything
with 'test' and 'example' and a few others, there's ~25 issues that look
like they need investigating. Here's the ones that look like they may
be legitimate issues: [I've removed ones that looked safe at the moment,
too, but reference them in the next paragraph.]

[src/components/perfctr_ppc/ppc64_events.c:336]: (error) Memory leak: ntv_evt_info
[src/components/perfmon2/perfmon.c:601]: (error) Memory leak: newctx
[src/components/perfmon2/perfmon.c:601]: (error) Memory leak: load_args
[src/components/perfmon2/perfmon.c:757]: (error) Uninitialized variable: buf
[src/aix.c:1098]: (error) Resource leak: map_f
[src/papi_libpfm3_events.c:70]: (error) Possible null pointer dereference: mask_values
[src/linux-memory.c:248]: (error) Resource leak: f
[src/papi_internal.c:357]: (error) Common realloc mistake: '_papi_native_events' nulled but not freed upon failure
[src/papi_internal.c:400]: (error) Common realloc mistake: '_papi_errlist' nulled but not freed upon failure
[src/papi_libpfm3_events.c:69]: (error) Possible null pointer dereference: mask_values
[src/papi_preset.c:376]: (error) Buffer is accessed out of bounds.
[src/perfctr-2.6.x/linux/drivers/perfctr/arm.c:508]: (error) Array 'pmc[4]' accessed at index 5, which is out of bounds.
[src/perfctr-2.6.x/linux/drivers/perfctr/arm.c:509]: (error) Array 'pmc[4]' accessed at index 4, which is out of bounds.
[src/perfctr-2.7.x/usr.lib/x86.c:42]: (error) Uninitialized variable: nr
[src/solaris-ultra.c:327]: (error) Resource leak: f
[src/utils/papi_command_line.c:175]: (error) Memory leak: success
[src/utils/papi_native_avail.c:273]: (error) Common realloc mistake: 'event_output_buffer' nulled but not freed upon failure
[src/sw_multiplex.c:659]: (error) Uninitialized variable: lastthread

At least the Infiniband warning can probably not be removed. Rewriting
the linux-memory.c file to not trip 'Possible null pointer dereference:
tmp' wouldn't be quick. But it would be nice to aim for a clean cppcheck
project-wide.

Thanks