Thanks for the feedback Frank. Apologies in advance for a long response.
On 19/04/13 10:15, Frank Ch. Eigler wrote:
...
It looks like it should cure the problem ... but ... now it puts a
blocking loop into the core pduread() function, which is not supposed
to block. So it makes it possible for a remote attacker to open a
connection, send just one byte down the pipe, and DoS the pmcd.
I don't thing this is the case. The only blocking occurs in the code
guarded by
if (timeout == TIMEOUT_ASYNC)
or if the caller specifies
timeout == TIMEOUT_NEVER
Otherwise the dead_hand timer will break us out of __pmRecv or read().
Now pduread() is only ever called from__pmGetPDU() passing in timeout
from the call to __pmGetPDU().
And __pmGetPDU() is called from all over the place (70+ calls below src
and qa), but the vast majority of these pass in the associated
prevailing timeout or TIMEOUT_DEFAULT (e.g. between a client and pmcd,
or pmcd and a pmda, or pmlogger and pmlc, ...) which are all safe I
believe. The notable exceptions are:
- dbpmda - always blocks waiting on a PMDA response ... not an issue
- pmcd uses TIMEOUT_NEVER when cleaning up a pmda that has sent an
unsolicited PDU or closed a socket ... this is OK
- pmlogger uses TIMEOUT_NEVER when waiting on a response from pmcd ...
this is OK
- the summary PMDA uses TIMEOUT_NEVER when communicating to either pmcd
or pmie (in secret agent mode), after it has established that data is
available on the socket ... this communication always involves whole
PDUs, so this is OK
- every pmda build with libpcp_pmda blocks (using TIMEOUT_NEVER) waiting
for a PDU from pmcd ... this is expected and OK
- pmproxy uses TIMEOUT_NEVER when communicating to either pmcd or
client, after it has established that data is available on the socket
... for pmproxy-pmcd communication this is OK (but sloppy) for
pmproxy-client communication this looks wrong
- a bunch of qa applications ... not an issue
So pmproxy seems exposed to a possible DOS attack with a short PDU, and
needs a review, but I think it is pmproxy that should be changed, not
pduread().
Note that noone (outside some qa apps) uses TIMEOUT_ASYNC nor its evil
twin GETPDU_ASYNC thanks to this piece of impl.h obscurity
#define GETPDU_ASYNC TIMEOUT_ASYNC /* backward-compatibility */
... I suspect both are Neanderthal throwbacks to the async PDU stuff
that I ripped out of libpcp when I did the thread-safe work ... I've put
pulling these on my TODO list, and this would make pduread() much simpler.
In the process of reviewing this, I notice there is another pduread()
implementation in libpcp_trace ... I'll need to audit that one.
Instead, how about inlining pduread() within __pmGetPDU(), where the
packet timeout may be observed during the incremental assembly of the
header *and* the payload.
I think the existing codes does something very similar to this already,
except popssibly taking a little less than 2 timeout delays in an
obscure corner case.
(Also, the new code shouldn't ever use read(2) on the fd, but only
__pmRecv, methinks.)
Not so. PDUs pass over pipes (pmcd-pmda, dbpmda-pmda, and perhaps
others) and over regular files (all PCP archive I/O).
Thanks again.
|