darkirc: Bound IRC input and queues

bin/darkirc/src/irc/client.rs

@@ -33,7 +33,7 @@ use darkfi_serial::{deserialize_async_partial, serialize_async};
 use futures::FutureExt;
 use sled_overlay::sled;
 use smol::{
-    io::{self, AsyncBufReadExt, AsyncWriteExt, BufReader},
+    io::{self, AsyncBufRead, AsyncBufReadExt, AsyncWriteExt, BufReader},
     lock::{OnceCell, RwLock},
     net::SocketAddr,
     prelude::{AsyncRead, AsyncWrite},
@@ -47,6 +47,56 @@ use super::{
 use crate::{crypto::rln::RLN2_SIGNAL_ZKBIN, Privmsg};
 
 const PENALTY_LIMIT: usize = 5;
+const MAX_IRC_LINE_LEN: usize = 1024;
+const MAX_PENDING_PRIVMSGS: usize = 128;
+
+/// Read one IRC line without allowing unbounded buffer growth.
+async fn read_bounded_line<R>(reader: &mut R, line: &mut String) -> Result<usize>
+where
+    R: AsyncBufRead + Unpin,
+{
+    line.clear();
+    let mut bytes = Vec::new();
+
+    loop {
+        let (consumed, complete) = {
+            let available = reader.fill_buf().await?;
+            if available.is_empty() {
+                if bytes.is_empty() {
+                    return Ok(0)
+                }
+
+                *line = String::from_utf8(bytes)?;
+                return Ok(line.len())
+            }
+
+            let newline = available.iter().position(|b| *b == b'\n');
+            let take = newline.map_or(available.len(), |idx| idx + 1);
+            if bytes.len().saturating_add(take) > MAX_IRC_LINE_LEN {
+                return Err(Error::ParseFailed("IRC line too long"))
+            }
+
+            bytes.extend_from_slice(&available[..take]);
+            (take, newline.is_some())
+        };
+
+        reader.consume(consumed);
+
+        if complete {
+            *line = String::from_utf8(bytes)?;
+            return Ok(line.len())
+        }
+    }
+}
+
+fn enqueue_pending_privmsg(args_queue: &mut VecDeque<Privmsg>, privmsg: Privmsg) -> bool {
+    if args_queue.len() >= MAX_PENDING_PRIVMSGS {
+        return false
+    }
+
+    args_queue.push_back(privmsg);
+    true
+}
 
 /// Reply types, we can either send server replies, or client replies.
 pub enum ReplyType {
@@ -157,7 +207,7 @@ impl Client {
         loop {
             futures::select! {
                 // Process message from the IRC client
-                r = reader.read_line(&mut line).fuse() => {
+                r = read_bounded_line(&mut reader, &mut line).fuse() => {
                     // If client closed unexpectedly, we disconnect.
                     if let Ok(0) = r {
                         error!("[IRC CLIENT] Read failed for {}: Client disconnected", self.addr);
@@ -537,8 +587,21 @@ impl Client {
             // Once synced, send queued lines and continue as normal
             if !self.server.darkirc.event_graph.is_synced() {
                 debug!("DAG is still syncing, queuing and skipping...");
-                let privmsg = self.args_to_privmsg(args).await;
-                args_queue.push_back(privmsg);
+                let Some(privmsg) = self.args_to_privmsg(args).await else {
+                    self.penalty.fetch_add(1, SeqCst);
+                    return Ok(None)
+                };
+
+                if !enqueue_pending_privmsg(args_queue, privmsg) {
+                    self.penalty.fetch_add(1, SeqCst);
+                    let nick = self.nickname.read().await.to_string();
+                    let reply = ReplyType::Notice((
+                        SERVER_NAME.to_string(),
+                        nick,
+                        "PRIVMSG queue is full; wait for sync before sending more".to_string(),
+                    ));
+                    self.reply(writer, &reply).await?;
+                }
                 return Ok(None)
             }
 
@@ -553,7 +616,10 @@ impl Client {
             }
 
             // If queue is empty, create an event and return it
-            let privmsg = self.args_to_privmsg(args).await;
+            let Some(privmsg) = self.args_to_privmsg(args).await else {
+                self.penalty.fetch_add(1, SeqCst);
+                return Ok(None)
+            };
             let event = self.privmsg_to_event(privmsg).await?;
 
             return Ok(Some(vec![event]))
@@ -563,15 +629,15 @@ impl Client {
     }
 
     // Internal helper function that creates a PRIVMSG from IRC client arguments
-    async fn args_to_privmsg(&self, args: String) -> Privmsg {
+    async fn args_to_privmsg(&self, args: String) -> Option<Privmsg> {
         let nick = self.nickname.read().await.to_string();
-        let channel = args.split_ascii_whitespace().next().unwrap().to_string();
-        let msg_offset = args.find(':').unwrap() + 1;
+        let channel = args.split_ascii_whitespace().next()?.to_string();
+        let msg_offset = args.find(':')? + 1;
         let (_, msg) = args.split_at(msg_offset);
 
         // Truncate messages longer than MAX_MSG_LEN
         let msg = if msg.len() > MAX_MSG_LEN { msg.split_at(MAX_MSG_LEN).0 } else { msg };
-        Privmsg { version: 0, msg_type: 0, channel, nick, msg: msg.to_string() }
+        Some(Privmsg { version: 0, msg_type: 0, channel, nick, msg: msg.to_string() })
     }
 
     // Internal helper function that creates an Event from PRIVMSG arguments
@@ -612,3 +678,61 @@ impl Client {
         Ok(db.contains_key(event_id.as_bytes())?)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::collections::VecDeque;
+
+    use smol::io::{BufReader, Cursor};
+
+    use super::{
+        enqueue_pending_privmsg, read_bounded_line, MAX_IRC_LINE_LEN, MAX_PENDING_PRIVMSGS,
+    };
+    use crate::irc::Privmsg;
+
+    #[test]
+    fn read_bounded_line_accepts_line_within_limit() {
+        smol::block_on(async {
+            let input = format!("{}\n", "a".repeat(MAX_IRC_LINE_LEN - 1));
+            let mut reader = BufReader::new(Cursor::new(input.into_bytes()));
+            let mut line = String::new();
+
+            let read = read_bounded_line(&mut reader, &mut line).await.unwrap();
+
+            assert_eq!(read, MAX_IRC_LINE_LEN);
+            assert!(line.ends_with('\n'));
+        });
+    }
+
+    #[test]
+    fn read_bounded_line_rejects_oversized_line() {
+        smol::block_on(async {
+            let input = format!("{}\n", "a".repeat(MAX_IRC_LINE_LEN));
+            let mut reader = BufReader::new(Cursor::new(input.into_bytes()));
+            let mut line = String::new();
+
+            assert!(read_bounded_line(&mut reader, &mut line).await.is_err());
+        });
+    }
+
+    #[test]
+    fn pending_privmsg_queue_has_fixed_capacity() {
+        let mut queue = VecDeque::new();
+        for _ in 0..MAX_PENDING_PRIVMSGS {
+            assert!(enqueue_pending_privmsg(&mut queue, privmsg()));
+        }
+
+        assert!(!enqueue_pending_privmsg(&mut queue, privmsg()));
+        assert_eq!(queue.len(), MAX_PENDING_PRIVMSGS);
+    }
+
+    fn privmsg() -> Privmsg {
+        Privmsg {
+            version: 0,
+            msg_type: 0,
+            channel: "#chan".to_string(),
+            nick: "nick".to_string(),
+            msg: "msg".to_string(),
+        }
+    }
+}

bin/darkirc/src/irc/command.rs

@@ -58,11 +58,48 @@ use tracing::{error, info};
 use super::{
     client::{Client, ReplyType},
     rpl::*,
-    server::MAX_NICK_LEN,
+    server::{MAX_MSG_LEN, MAX_NICK_LEN},
     IrcChannel, SERVER_NAME,
 };
 use crate::crypto::bcrypt::bcrypt_hash_password;
 
+const MAX_TOPIC_LEN: usize = MAX_MSG_LEN;
+
+fn is_identifier_char(byte: u8) -> bool {
+    byte.is_ascii_alphanumeric() ||
+        matches!(byte, b'-' | b'_' | b'[' | b']' | b'\\' | b'`' | b'^' | b'{' | b'}' | b'|')
+}
+
+/// Return true when a client-supplied nickname is safe to store and echo.
+fn is_valid_nickname(nickname: &str) -> bool {
+    !nickname.is_empty() &&
+        nickname.len() <= MAX_NICK_LEN &&
+        nickname.bytes().all(is_identifier_char)
+}
+
+/// Return true when a client-supplied username is safe for local state keys.
+fn is_valid_username(username: &str) -> bool {
+    is_valid_nickname(username)
+}
+
+/// Return true when a channel name is bounded and cannot split IRC replies.
+fn is_valid_channel_name(channel: &str) -> bool {
+    channel.len() >= 2 &&
+        channel.len() <= MAX_NICK_LEN &&
+        channel.starts_with('#') &&
+        !channel[1..].starts_with('#') &&
+        channel.bytes().all(|b| b.is_ascii_graphic() && b != b',')
+}
+
+/// Return true when a topic is bounded and line-safe.
+fn is_valid_topic(topic: &str) -> bool {
+    topic.len() <= MAX_TOPIC_LEN && !topic.bytes().any(|b| matches!(b, b'\0' | b'\r' | b'\n'))
+}
+
+fn invalid_syntax(nick: &str, command: &str) -> Vec<ReplyType> {
+    vec![ReplyType::Server((ERR_NEEDMOREPARAMS, format!("{nick} {command} :{INVALID_SYNTAX}")))]
+}
+
 #[derive(Debug, PartialEq, Eq)]
 enum TopicRequest<'a> {
     Get { channel: &'a str },
@@ -262,43 +299,29 @@ impl Client {
         // Here we'll hold valid channel names.
         let mut channels = HashSet::new();
 
-        // Let's scan through our channels. For now we'll only support
-        // channel names starting with a single '#' character.
+        // Weechat sends channels as `#chan1,#chan2,#chan3`. Handle both
+        // comma-separated and whitespace-separated channel lists uniformly.
         let nick = self.nickname.read().await.to_string();
-        let tokens = args.split_ascii_whitespace();
-        for channel in tokens {
-            if !channel.starts_with('#') {
-                self.penalty.fetch_add(1, SeqCst);
-                return Ok(vec![ReplyType::Server((
-                    ERR_NEEDMOREPARAMS,
-                    format!("{nick} JOIN :{INVALID_SYNTAX}"),
-                ))])
-            }
-
-            if !active_channels.contains(channel) {
-                channels.insert(channel.to_string());
-            }
-        }
-
-        // Weechat sends channels as `#chan1,#chan2,#chan3`. Handle it.
-        if channels.len() == 1 {
-            let list = channels.iter().next().unwrap().clone();
-            channels.remove(list.as_str());
-
+        let mut saw_channel = false;
+        for list in args.split_ascii_whitespace() {
             for channel in list.split(',') {
-                if !channel.starts_with('#') || channel.len() > MAX_NICK_LEN {
+                saw_channel = true;
+                if !is_valid_channel_name(channel) {
                     self.penalty.fetch_add(1, SeqCst);
-                    return Ok(vec![ReplyType::Server((
-                        ERR_NEEDMOREPARAMS,
-                        format!("{nick} JOIN :{INVALID_SYNTAX}"),
-                    ))])
+                    return Ok(invalid_syntax(&nick, "JOIN"))
                 }
+
                 if !active_channels.contains(channel) {
                     channels.insert(channel.to_string());
                 }
             }
         }
 
+        if !saw_channel {
+            self.penalty.fetch_add(1, SeqCst);
+            return Ok(invalid_syntax(&nick, "JOIN"))
+        }
+
         // Create new channels for this client and construct replies.
         let mut server_channels = self.server.channels.write().await;
         let mut replies = vec![];
@@ -405,6 +428,11 @@ impl Client {
             ))])
         }
 
+        if !is_valid_channel_name(target) {
+            self.penalty.fetch_add(1, SeqCst);
+            return Ok(invalid_syntax(&nick, "MODE"))
+        }
+
         if !self.server.channels.read().await.contains_key(target) {
             return Ok(vec![ReplyType::Server((
                 ERR_NOSUCHNICK,
@@ -451,6 +479,11 @@ impl Client {
         // If a channel was requested, reply only with that one.
         // Otherwise, return info for all known channels.
         if let Some(req_chan) = tokens.next() {
+            if !is_valid_channel_name(req_chan) {
+                self.penalty.fetch_add(1, SeqCst);
+                return Ok(invalid_syntax(&nick, "NAMES"))
+            }
+
             if let Some(chan) = self.server.channels.read().await.get(req_chan) {
                 let nicks: Vec<String> = chan.nicks.iter().cloned().collect();
 
@@ -503,7 +536,7 @@ impl Client {
 
         // Forbid disallowed characters.
         // The next() call is done to check for ASCII whitespace in the nick.
-        if tokens.next().is_some() || nickname.starts_with(':') || nickname.starts_with('#') {
+        if tokens.next().is_some() || !is_valid_nickname(nickname) {
             self.penalty.fetch_add(1, SeqCst);
             return Ok(vec![ReplyType::Server((
                 ERR_ERRONEOUSNICKNAME,
@@ -511,15 +544,6 @@ impl Client {
             ))])
         }
 
-        // Disallow too long nicks
-        if nickname.len() > MAX_NICK_LEN {
-            self.penalty.fetch_add(1, SeqCst);
-            return Ok(vec![ReplyType::Server((
-                ERR_ERRONEOUSNICKNAME,
-                format!("{old_nick} {nickname} :Nickname too long"),
-            ))])
-        }
-
         // Set the new nickname
         *self.nickname.write().await = nickname.to_string();
 
@@ -565,12 +589,9 @@ impl Client {
             ))])
         };
 
-        if !channel.starts_with('#') {
+        if !is_valid_channel_name(channel) {
             self.penalty.fetch_add(1, SeqCst);
-            return Ok(vec![ReplyType::Server((
-                ERR_NEEDMOREPARAMS,
-                format!("{nick} PART :{INVALID_SYNTAX}"),
-            ))])
+            return Ok(invalid_syntax(&nick, "PART"))
         }
 
         let mut active_channels = self.channels.write().await;
@@ -679,6 +700,19 @@ impl Client {
             ))])
         }
 
+        if target.starts_with('#') {
+            if !is_valid_channel_name(target) {
+                self.penalty.fetch_add(1, SeqCst);
+                return Ok(vec![ReplyType::Server((
+                    ERR_NORECIPIENT,
+                    format!("{nick} :Invalid recipient given (PRIVMSG)"),
+                ))])
+            }
+        } else if !target.eq_ignore_ascii_case("nickserv") && !is_valid_nickname(target) {
+            self.penalty.fetch_add(1, SeqCst);
+            return Ok(vec![ReplyType::Server((ERR_NOSUCHNICK, format!("{nick} :{target}")))])
+        }
+
         // We only send a client reply if the message is for ourself or if
         // we're trying to communicate with IRC services.
         // Anything else is rendered by the IRC client and not supposed
@@ -737,6 +771,11 @@ impl Client {
 
         match request {
             TopicRequest::Get { channel } => {
+                if !is_valid_channel_name(channel) {
+                    self.penalty.fetch_add(1, SeqCst);
+                    return Ok(invalid_syntax(&nick, "TOPIC"))
+                }
+
                 let channels = self.server.channels.read().await;
                 let Some(channel_state) = channels.get(channel) else {
                     return Ok(vec![ReplyType::Server((
@@ -758,6 +797,11 @@ impl Client {
                 }
             }
             TopicRequest::Set { channel, topic } => {
+                if !is_valid_channel_name(channel) || !is_valid_topic(topic) {
+                    self.penalty.fetch_add(1, SeqCst);
+                    return Ok(invalid_syntax(&nick, "TOPIC"))
+                }
+
                 let mut channels = self.server.channels.write().await;
                 let Some(channel_state) = channels.get_mut(channel) else {
                     return Ok(vec![ReplyType::Server((
@@ -834,10 +878,13 @@ impl Client {
 
         if !realname.starts_with(':') {
             self.penalty.fetch_add(1, SeqCst);
-            return Ok(vec![ReplyType::Server((
-                ERR_NEEDMOREPARAMS,
-                format!("{nick} USER :{INVALID_SYNTAX}"),
-            ))])
+            return Ok(invalid_syntax(&nick, "USER"))
+        }
+
+        let realname_body = &realname[1..];
+        if !is_valid_username(username) || !is_valid_topic(realname_body) {
+            self.penalty.fetch_add(1, SeqCst);
+            return Ok(invalid_syntax(&nick, "USER"))
         }
 
         *self.username.write().await = username.to_string();
@@ -1047,4 +1094,27 @@ mod tests {
     fn parse_topic_request_rejects_bare_topic_body() {
         assert_eq!(parse_topic_request("#chan value"), None);
     }
+
+    #[test]
+    fn channel_name_validation_rejects_empty_nested_and_overlong_names() {
+        assert!(super::is_valid_channel_name("#chan"));
+        assert!(!super::is_valid_channel_name("#"));
+        assert!(!super::is_valid_channel_name("##chan"));
+        assert!(!super::is_valid_channel_name(&format!("#{}", "a".repeat(24))));
+    }
+
+    #[test]
+    fn nickname_validation_rejects_unsafe_identifier_chars() {
+        assert!(super::is_valid_nickname("alice_1"));
+        assert!(!super::is_valid_nickname("bad!nick"));
+        assert!(!super::is_valid_nickname("bad@nick"));
+        assert!(!super::is_valid_nickname("bad.nick"));
+    }
+
+    #[test]
+    fn topic_validation_has_fixed_bound() {
+        assert!(super::is_valid_topic(&"a".repeat(super::MAX_TOPIC_LEN)));
+        assert!(!super::is_valid_topic(&"a".repeat(super::MAX_TOPIC_LEN + 1)));
+        assert!(!super::is_valid_topic("bad\nline"));
+    }
 }

bin/darkirc/src/irc/services/nickserv.rs

@@ -69,11 +69,12 @@ use darkfi_sdk::{crypto::pasta_prelude::PrimeField, pasta::pallas};
 use darkfi_serial::{deserialize_async, serialize_async};
 use smol::lock::RwLock;
 
-use super::super::{client::ReplyType, rpl::*};
+use super::super::{client::ReplyType, rpl::*, server::MAX_NICK_LEN};
 use crate::{crypto::rln::RlnIdentity, genesis_commits::is_pregenerated_commitment, IrcServer};
 
 pub const ACCOUNTS_DB_PREFIX: &str = "darkirc_account_";
 pub const ACCOUNTS_KEY_RLN_IDENTITY: &[u8] = b"rln_identity";
+const MAX_ACCOUNT_NAME_LEN: usize = MAX_NICK_LEN;
 
 /// Name of the sled tree that mirrors the currently-active identity.
 /// `IrcServer::new` reads this on startup.
@@ -313,7 +314,7 @@ impl NickServ {
             // want to list the mirror as if it were a separate
             // account.
             let Some(account_name) = name.strip_prefix(ACCOUNTS_DB_PREFIX) else { continue };
-            if account_name == "default" || account_name.is_empty() {
+            if !is_valid_account_name(account_name) {
                 continue
             }
 
@@ -359,7 +360,7 @@ impl NickServ {
     /// reconstruct the identity elsewhere.
     async fn handle_info_account(&self, nick: &str, account_name: &str) -> Result<Vec<ReplyType>> {
         let tree_name = format!("{ACCOUNTS_DB_PREFIX}{account_name}");
-        if account_name == "default" || account_name.is_empty() {
+        if !is_valid_account_name(account_name) {
             return Ok(vec![notice(nick, "Invalid account name.")])
         }
 
@@ -431,7 +432,7 @@ impl NickServ {
         };
 
         // Reserved name. We use `default` for the mirror tree.
-        if account_name == "default" || account_name.is_empty() {
+        if !is_valid_account_name(account_name) {
             return Ok(vec![notice(nick, "Invalid account name.")])
         }
 
@@ -543,7 +544,7 @@ impl NickServ {
         let Some(account_name) = tokens.next() else {
             return Ok(vec![notice(nick, "Invalid syntax. Use `DEREGISTER <account_name>`.")])
         };
-        if account_name == "default" || account_name.is_empty() {
+        if !is_valid_account_name(account_name) {
             return Ok(vec![notice(nick, "Invalid account name.")])
         }
 
@@ -612,7 +613,7 @@ impl NickServ {
         let Some(account_name) = tokens.next() else {
             return Ok(vec![notice(nick, "Invalid syntax. Use `SET <account_name>`.")])
         };
-        if account_name == "default" || account_name.is_empty() {
+        if !is_valid_account_name(account_name) {
             return Ok(vec![notice(nick, "Invalid account name.")])
         }
 
@@ -713,7 +714,7 @@ impl NickServ {
                 ],
             ))
         };
-        if account_name == "default" || account_name.is_empty() {
+        if !is_valid_account_name(account_name) {
             return Ok(vec![notice(nick, "Invalid account name.")])
         }
 
@@ -874,6 +875,18 @@ impl NickServ {
     }
 }
 
+fn is_account_name_char(byte: u8) -> bool {
+    byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_')
+}
+
+/// Return true when a local account name is safe as a sled tree suffix.
+fn is_valid_account_name(account_name: &str) -> bool {
+    account_name != "default" &&
+        !account_name.is_empty() &&
+        account_name.len() <= MAX_ACCOUNT_NAME_LEN &&
+        account_name.bytes().all(is_account_name_char)
+}
+
 /// Parse a NickServ PRIVMSG body into the service command and remaining arguments.
 fn parse_nickserv_command(query: &str) -> Option<(&str, SplitAsciiWhitespace<'_>)> {
     let mut tokens = query.split_ascii_whitespace();
@@ -919,4 +932,13 @@ mod tests {
     fn parse_nickserv_command_rejects_empty_command() {
         assert!(parse_nickserv_command("NickServ :").is_none());
     }
+
+    #[test]
+    fn account_name_validation_rejects_reserved_and_unsafe_names() {
+        assert!(super::is_valid_account_name("alice_1"));
+        assert!(!super::is_valid_account_name("default"));
+        assert!(!super::is_valid_account_name(""));
+        assert!(!super::is_valid_account_name("../alice"));
+        assert!(!super::is_valid_account_name(&"a".repeat(super::MAX_ACCOUNT_NAME_LEN + 1)));
+    }
 }