Twisted IRCClient Remote DoS

[XiX]

A flaw exists within IRCClient's CTCP implementation. Multiple CTCP requests can be sent on one line to the client. This can be exploited by a malicious user bundling many erroneous CTCP requests on one line, causing the Twisted client to flood back an errmsg for every request & effectively kill itself due to excess flood.

Example:

<XiX> \x01\\x01
<pyn> ERRMSG \\ None: Unknown query '\\'
<XiX> \x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01\x01\\x01
<pyn> ERRMSG \\ None: Unknown query '\\'
<pyn> ERRMSG \\ None: Unknown query '\\'
<pyn> ERRMSG \\ None: Unknown query '\\'
<pyn> ERRMSG \\ None: Unknown query '\\'
<pyn> ERRMSG \\ None: Unknown query '\\'
<pyn> ERRMSG \\ None: Unknown query '\\'
* pyn has quit (Excess Flood)

[jonathanj]

(In [31600]) Branching to 'ctcp-dos-5029'

[jonathanj]

I've changed the CTCP dispatch code to now only reply to the first CTCP query of each type, the CTCP specification (if you can call it that) doesn't say anything about multiple queries of the same type on a single line and I don't know of any IRC clients that do this either.

I've also elected to remove replying to unknown CTCP messages which might be a debatable topic but if a proper CTCP message cannot be constructed by the sender it doesn't seem unreasonable to ignore these, considering that there is very little else we can do without negatively impacting the recipient.

[exarkun]

Thanks for picking this up jonathanj.

I think the decision to only reply to one CTCP message of each type is a sensible one. I like the implementation, too. I think the decision not to reply to unrecognized CTCP messages at all makes sense as well. However, I don't think we need to get rid of the ability to define a fallback handler for CTCP messages at the same time. How about continuing to send unknown CTCP messages to ctcpUnknownQuery, but not doing anything in IRCClient's implementation of that method? (Deleting the method entirely would also raise a compatibility issue - it is a public API, application code might be calling it, which the change currently in the branch would break, so deleting it would require a deprecation period first).

Thanks for the other minor cleanups as well.

[jonathanj]

(In [31691]) Replace the CTCP fallback handler with one that does not reply, refs #5029.

[jonathanj]

Replying to exarkun:

However, I don't think we need to get rid of the ability to define a fallback handler for CTCP messages at the same time. How about continuing to send unknown CTCP messages to ctcpUnknownQuery, but not doing anything in IRCClient's implementation of that method?

Sounds good. I've left the log message in ctcpUnknownQuery since I thought that would be useful in the case where you're not choosing to override it.

[exarkun]

1. the test_noDuplicateCTCPDispatch docstring suggests that no reply is made to duplicate messages, but there's no assertion about replies here.
2. the test_noDefaultDispatch docstring says unrecognized messages are ignored. It also says the fallback handler is invoked for them. I think only one of these can be true. ;)
3. I think duplicate unknown messages will be dispatched repeatedly because of the way the if/elif is done in ctcpQuery. There's no test case to prove me wrong, at least.

Looks good otherwise! Please merge when you've addressed the above to your satisfaction.

[jonathanj]

(In [31734]) Address review commentary, refs #5029.

[jonathanj]

(In [31735]) Merge ctcp-dos-5029.

Author: jonathanj Reviewer: exarkun Fixes: #5029

IRCClient contained a potential denial-of-service attack point where it would reply to all CTCP queries, known and unknown contained on a single line, in the form of a new line for each reply. Now only the first CTCP query of a type, per line, is replied to and unknown CTCP queries are not replied to at all.