Comment 9 for bug 1700827

Hi, Seth -

On Thu, Aug 17, 2017 at 07:15:01AM -0000, Seth Arnold wrote:
> 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.

Thanks.

> [...]
> 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.

These (and sprintf) are generally used with data coming from trusted
sources such as kernel /proc files. Their format is relatively fixed,
and is generally not amenable to end-user interference, so large
buffers and unchecked snprintf() seem risk-free. I suppose an
assert() wouldn't hurt, just be unsightly.

> There are at least half a dozen functions that serve as system()
> or popen() replacements or wrappers. [...]

Other than their existence, what did you see wrong?

> [...]
> Security team NAK for promoting pcp to main.
> Here's the notes I collected in the hopes that they are useful for
> someone: [...]

Thanks! It would be very useful to see individual bug reports about
that subset of issues that you consider to earn that NAK. Then we can
focus on analyzing them in detail - to see if there is any real
security risk; to discuss why the code is the way it is; to track
fixes of actual problems. Your rough list of notes is good but not
easily actionable.

- FChE