search

bitwalker / exirc

IRC client adapter for Elixir projects
http://bitwalker.github.io/exirc/
MIT License
152 stars 37 forks source link

Injection vulnerability in ExIRC.Client.msg() / IRC standard nonconformance #97

Open calimeroteknik opened 2 years ago

calimeroteknik commented 2 years ago

I'll present this as a little story:

Suppose we are implementing a bot that reads the titles from Web pages and posts them on IRC, a classic.

As a short introduction (only vaguely related to the bug), further suppose that we are using Floki (which is also not standards conformant) to parse the title of say, this webpage:

<!DOCTYPE html>
<html><head><title>I don&#39;t want to
quit programming!</title></head><body>
…</body></html>

(this is conformant HTML, even though the formatting isn't pretty!)

Assume the result gets into title = "I don't want to\nquit programming!"… and we get to the ExIRC bug:

ExIRC.Client.msg(state.client, :privmsg, dest, "Title: #{title}")

In the IRC channel we see:

<someone> https://domain.tld/blog/i-dont-want-to-quit-programming
<potionbot> Title: I don't want to
*potionbot has quit ("programming!")

Oops.

Obvious solutions:

My preference goes to the last two, and specifically both of them at once:

For more ideas, see https://eiv.dev

bitwalker commented 2 years ago

This is the same class of issue as SQL injection, in that unknown (theoretically attacker-controlled) content is being directly passed through to an API that expects text that may contain control sequences - after all, msg/4 can't know whether a particular string given to it was crafted intentionally or not.

The actual behavior here (IIRC) is that of your "Creative" solution - i.e. it results in multiple messages. The problem is that of control sequences like \quit. Those are perfectly valid messages to send, so one must either escape message content that should not be able to use such sequences, or disallow messages containing them altogether. Both of those are questions for the consumer of the library to decide, in my opinion. That said, I do think it makes sense to add an option to msg/4 that indicates that the message should be escaped to prevent any control sequences from being used OR expose an API for escaping text that can be called prior to msg/4. Since the escaping rules are something best understood by the IRC library, it makes sense to provide some facility for that.

I'm not the primary maintainer of this library any more, that would be @tchoutri, but figured I would chime in with my two cents at least.

calimeroteknik commented 2 years ago

No, that's an actual newline, not a weird escape sequence or anything. (IRC is line-wise, so this sends more than one message!)

bitwalker commented 2 years ago

Yes, I'm aware, I'm saying that it is perfectly valid to call msg/4 with content containing newlines, it gets converted into multiple messages as per the protocol.

calimeroteknik commented 2 years ago

Er, actually it does not… it would be fine if it did!

Specifically, this is what was sent by the bot to the TCP socket:

privmsg #somechannel :Title: I don't want to
quit programming!

It would be completely fine if it had sent:

privmsg #somechannel :Title: I don't want to
privmsg #somechannel :quit programming!

Because in the channel we would see this:

<someone> https://domain.tld/blog/i-dont-want-to-quit-programming
<potionbot> Title: I don't want to
<potionbot> quit programming!

(this is what I called "Creative", but unfortunately is not what happens)

calimeroteknik commented 2 years ago

Just insisting that this made the bot disconnect from IRC, not post an Action Message.