Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
calimeroteknik opened this issue Sep 18, 2022 · 5 comments
Open

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

calimeroteknik opened this issue Sep 18, 2022 · 5 comments

Comments

@calimeroteknik
Copy link

calimeroteknik commented Sep 18, 2022 •
edited
Loading

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:

  • Panicking: cut off all text after the first newline to prevent the injection.
  • Vengeful: throw an error if there is a newline in the message (this is not legal in the IRC standard anyway).
  • Posed: make the type of the fourth argument of ExIRC.Client.msg() a StringWithoutNewlines type, preventing the error at compile time.
  • Creative: post several messages in IRC, one per line.

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

  • Make ExIRC.Client.msg() refuse newlines, preventing the application from starting if that's not guarded against, and
  • Introduce a new ExIRC.Client.msg_multiline() method that posts several messages to accomodate for the line feeds.

For more ideas, see https://eiv.dev
@bitwalker
Copy link
Owner

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
Copy link
Author

calimeroteknik commented Sep 18, 2022 •
edited
Loading

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
Copy link
Owner

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
Copy link
Author

calimeroteknik commented Sep 18, 2022 •
edited
Loading

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bitwalker @calimeroteknik