Opened 4 years ago

Closed 3 years ago

#5029 defect closed fixed (fixed)

Twisted IRCClient Remote DoS

Reported by: XiX Owned by: jonathanj
Priority: high Milestone:
Component: words Keywords: IRCClient DoS security
Cc: ralphm Branch: branches/ctcp-dos-5029
(diff, github, buildbot, log)
Author: jonathanj Launchpad Bug:

Description (last modified by glyph)

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)

Change History (12)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc ralphm added

comment:2 Changed 4 years ago by glyph

  • Description modified (diff)

comment:3 Changed 4 years ago by exarkun

  • Author XiX deleted
  • Keywords security added

comment:4 Changed 4 years ago by exarkun

  • Summary changed from Twisted IRCClient Remote DDoS to Twisted IRCClient Remote DoS

comment:5 Changed 4 years ago by jonathanj

  • Author set to jonathanj
  • Branch set to branches/ctcp-dos-5029

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

comment:6 Changed 4 years ago by jonathanj

  • Keywords DoS review added; DDoS removed

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.

comment:7 follow-up: Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to jonathanj

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.

comment:8 Changed 4 years ago by jonathanj

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

comment:9 in reply to: ↑ 7 Changed 4 years ago by jonathanj

  • Keywords review added
  • Owner jonathanj deleted

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.

comment:10 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to jonathanj
  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.

comment:11 Changed 3 years ago by jonathanj

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

comment:12 Changed 3 years ago by jonathanj

  • Resolution set to fixed
  • Status changed from new to closed

(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.

Note: See TracTickets for help on using tickets.