Security: newline injection

[kpcyrd]

I really enjoy using this library as it nicely abstracts the irc protocol on the wire. I was wondering how it handles some edge cases and noticed it just passes strings to the server:

server.send_privmsg(channel, "ohai\nTOPIC #rust :this is a bug").unwrap();

The library just writes this string to the irc connection which causes the topic to be set to this is a bug. This is an issue if you send data to irc which you don't trust 100% and could be abused to escalate privileges if you manage to pass crafted data into the irc library.

I believe there is code in the library that tries to handle this situation:

    fn send_privmsg(&self, target: &str, message: &str) -> Result<()> where Self: Sized {
        for line in message.split("\r\n") {
            try!(self.send(PRIVMSG(target.to_owned(), line.to_owned())))
        }
        Ok(())
    }

yet it only handles \r\n, not single \n (or potentionally single \r). There are also some functions that don't have this workaround, eg .send_topic could potentionally have untrusted data as well.

I'd really love if the library would safely abstract these sort of issues for me on all functions that write data to the irc connection. :)

[aatxe]

So, I'm a little confused about your input source. IRC messages cannot contain new lines. Thus, nothing your users say to a bot (or normal IRC client) should be able to result in the bot or client sending a message containing a new line. The code you've pointed out was really intended as a convenience to allow you to send multiline messages.

If you have some other source of data for your bot, I see how this could be an issue, but it's not really clear what the right course of action would be in such a case. We can offer some sort of sanitization function that strips everything following a new line from a string and you could call that on any input you send in. Doing the same sort of splitting that is done in PRIVMSG doesn't make much sense in general. You cannot send multiline topics. The new topics would simply wipe out the old.

[kpcyrd]

@aatxe I'm using multiple sources that may contain newlines (like json from http). I agree that there are multiple valid opinions on this, I think a library should safely abstract these issues for me, no matter how invalid my input is :)

Possible solutions (I can think of):

• split on "\r" or "\n" and execute multiple times
• truncate on the first "\r" or "\n"
• remove "\r" and "\n" from the input

It doesn't really matter how useful the result is (setting two different topics subsequentionally might be slightly pointless), the problem is that the irc library might have a privileged session and execute MODE commands or worse.

I wrote this function that I use to strip everything after newlines, feel free to use it. I'd prefer if this is called automatically inside the library.

pub fn sanitize(data: &str) -> &str {
    let mut data = data;

    for needle in &["\r", "\n"] {
        if let Some(pos) = data.find(needle) {
            data = data.split_at(pos).0;
        }
    }

    data
}

[aatxe]

I've added something along the lines of what you're doing, but with some slight modifications (need to keep in tact the actual newlines).