nathans wrote:
>> - Consider reworking the mmvdump program to rely on pmda_mmv.so to do
>> the parsing of the mmv shmem region, instead of partly duplicating
>> the code. That way one gets a single implementation.
>
> The use cases are very different. mmvdump.c attempts to keep going and
> to report as much of the mapping as it possibly can (a debugging aid),
> whereas pmdammv silently discards mappings at the first sign of trouble.
That difference could be expressed by a flag.
> I agree with the original authors thoughts on this one - implementation
> is sufficiently simple as to not bother obfuscating the pmdammv code in
> this way, so I don't intend to either.
If the implementation were sufficiently simple, they would not be
suffering from the sort of robustness problems that your patches
to some extent improve.
>> - Consider engaging a real fuzzer like afl against pmdammv. This could
>> work especially effectively if mmvdump links to pmda_mmv.so, so they
>> could be easily recompiled/run together under afl control.
>
> That's well beyond the scope of the bug fix here - interesting project to
> undertake though, so feel free to hack on it. pmlogcheck may also benefit
> from such approaches FWLIW.
The problem is that by picking off five or six individual bugs, you
have no way of knowing how many problems are left, other than a very
close inspection of the code. Do you believe there are now zero bugs
there?
>> - The new implementation is still susceptible to a malevolent
>> unprivileged local programs DoS'ing pmdammv by junk files and/or
>> rapidly-cycling hdr.g1!=g2 numbers. The core code should consider
>> backing off suspected problematic slist[] entries in
>> mmv_reload_maybe.
>
> Seems a bit of a stretch to me (esp. considering setup of MMV mappings
> dir is opt-in so unprivileged local programs cannot write by default &
> then it depends on which permissions scheme is chosen by local admin).
That is false.
% ls -ald /var/lib/pcp/tmp/mmv
drwxrwxrwt. 2 root root 4096 May 13 16:02 /var/lib/pcp/tmp/mmv
i.e., globally writable, due to:
src/pmdas/mmv/src/Install:if [ ! -e "$PCP_TMP_DIR/mmv" ]
src/pmdas/mmv/src/Install: echo "creating $PCP_TMP_DIR/mmv"
src/pmdas/mmv/src/Install: mkdir -p -m 1777 "$PCP_TMP_DIR/mmv"
> So, not seeing it as a real issue (relative to the same sorts of attack
> being applied to say pmcd itself)
pmcd is indeed susceptible to DoS from the network - that's not news,
but traffic load can be ameliorated by firewalls etc.
It better not be susceptible to corrupt packets from the network. I
suspect our testing of this is not particularly thorough. (ISTR
floating an idea long ago to let pmcd take packets from stdin/stdout,
as per a normal inetd daemon. That would make mechanical fuzzing a
little easier.)
> - perhaps produce a test case showing this scenario in practice & I
> might be convinced. [...]
To person looking critically at the code, a thought experiment
suffices. A hostile or buggy program can create one or more mmv
files, then in a tight loop proceed to increment the header g1/g2
numbers. pmdammv rescans those numbers in *all* mmv files, for *each*
incoming pmda fetch/etc. operation. In case of any g1/g2 change
anywhere, it sets reload=1. Then it throws away all of its state in
map_stats() and rescans the whole bog.
It's comforting not to see obvious infinite loops inside the code that
could be triggered by this, but the effort pmdammv expends is so much
higher than the hostile/buggy mmv data supplier as to be a DoS. (By
the way, have you measured or estimated how many large mmv's the code
could possibly reload/process within the pmcd-pmda 5s timeout?)
- FChE
|