From 83675d926d05be4d633ce7ec7b1cc94e882d5a0d Mon Sep 17 00:00:00 2001 From: InterLinked1 <24227567+InterLinked1@users.noreply.github.com> Date: Thu, 14 Dec 2023 12:29:03 -0500 Subject: [PATCH] net_irc, door_irc: Fix various IRC bugs. There were several compounding bugs causing various issues with IRC, in particular with using door_irc to connect to IRC, both to an external network and directly to the local BBS. * menu.c: Reusing a small buffer truncated arguments to the menu handler. We now use a significantly large buffer. * door_irc: Use bbs_irc_client_msg, not bbs_irc_client_send. * door_irc: Abort client loop if irc_read returns <= 0, fixing infinite loop on disconnect. * door_irc: Fix passing wrong variable name into bbs_readline_append. * door_irc: If nickname is already in use, automatically append underscores to try to construct a unique nickname, so that this works even if the user is already logged in on another node. * net_irc: Allow nicknames to be either the actual username as before or also the username followed by an underscore and other characters, allowing for the same user to log in from multiple nodes. * net_irc: Properly respond with an error message for unhandled messages. --- bbs/menu.c | 21 ++++++++++----- bbs/variables.c | 4 +-- doors/door_irc.c | 34 ++++++++++++++++++++---- include/variables.h | 4 +-- nets/net_irc.c | 63 +++++++++++++++++++++++++++++++++------------ 5 files changed, 94 insertions(+), 32 deletions(-) diff --git a/bbs/menu.c b/bbs/menu.c index 7165eb24..20b5d210 100644 --- a/bbs/menu.c +++ b/bbs/menu.c @@ -470,6 +470,8 @@ static int menu_set_title(struct bbs_node *node, const char *name) return bbs_node_set_term_title(node, stripped); } +#define SCRATCH_BUF_SIZE 256 + /*! * \brief Run the BBS on a node * \param node node @@ -477,7 +479,7 @@ static int menu_set_title(struct bbs_node *node, const char *name) * \param stack Current stack count * \param optreq Option request string */ -static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char *menuitemname, int stack, const char *optreq) +static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char *menuitemname, int stack, const char *optreq, char *restrict scratchbuf) { int res; char opt = 0; @@ -653,13 +655,15 @@ static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char * we need to be able to recurse efficiently with arguments that other handlers don't (and shouldn't) get. */ if (STARTS_WITH(menuitem->action, "menu:")) { /* Execute another menu, recursively */ - res = bbs_menu_run(node, menuitem->action + 5, menuitem->name, stack, optreq ? optreq + 1 : NULL); + res = bbs_menu_run(node, menuitem->action + 5, menuitem->name, stack, optreq ? optreq + 1 : NULL, scratchbuf); } else { char *handler, *args; - /* At this point in the function, we're done with the options buffer, and if we need it again, we'll fill it again. - * So, reuse that buffer since we're in a recursive function, and we really want to avoid stack allocations if possible. */ - safe_strncpy(options, menuitem->action, sizeof(options)); - handler = options; + /* We need a buffer that is sufficiently long enough to avoid truncation, so use the scratch buffer. */ + safe_strncpy(scratchbuf, menuitem->action, SCRATCH_BUF_SIZE); + if (strlen(scratchbuf) >= SCRATCH_BUF_SIZE - 1) { + bbs_warning("Truncation occurred copying menu item arguments into buffer of size %d\n", SCRATCH_BUF_SIZE); + } + handler = scratchbuf; args = strchr(handler, ':'); if (args) { *args++ = '\0'; /* We just want the name of the menu handler to use. */ @@ -723,7 +727,10 @@ static int bbs_menu_run(struct bbs_node *node, const char *menuname, const char int bbs_node_menuexec(struct bbs_node *node) { - return bbs_menu_run(node, DEFAULT_MENU, NULL, 0, NULL); + /* Declare just once rather than on each stack frame, since bbs_menu_run recurses a lot. + * This allows us to save (BBS_MAX_MENUSTACK - 1) * SCRATCH_BUF_SIZE bytes of stack space. */ + char scratchbuf[SCRATCH_BUF_SIZE]; + return bbs_menu_run(node, DEFAULT_MENU, NULL, 0, NULL, scratchbuf); } static int load_config(int reload) diff --git a/bbs/variables.c b/bbs/variables.c index de98d290..c250e47c 100644 --- a/bbs/variables.c +++ b/bbs/variables.c @@ -433,7 +433,7 @@ const char *bbs_node_var_get(struct bbs_node *node, const char *key) return value; } -int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size_t len) +int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *restrict buf, size_t len) { const char *s; @@ -459,7 +459,7 @@ int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size return s ? 0 : -1; } -int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *buf, size_t len) +int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *restrict buf, size_t len) { char varname[64]; char *bufstart = buf; diff --git a/doors/door_irc.c b/doors/door_irc.c index f5dcce8e..1a1cab2c 100644 --- a/doors/door_irc.c +++ b/doors/door_irc.c @@ -159,15 +159,19 @@ static int __chat_send(struct client_relay *client, struct participant *sender, /* If sender is set, it's safe to use even with no locks, because the sender is a calling function of this one */ if (sender) { - bbs_debug(7, "Broadcasting %s to %s,%s (except node %d): %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, sender->node->id, datestr, len, msg); + bbs_debug(7, "Broadcasting %s -> %s,%s (except node %d): %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, sender->node->id, datestr, len, msg); } else { - bbs_debug(7, "Broadcasting %s to %s,%s: %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, datestr, len, msg); + bbs_debug(7, "Broadcasting %s -> %s,%s: %s%.*s\n", dorelay ? "to IRC" : "from IRC", client->name, channel, datestr, len, msg); } /* Relay the message to everyone */ RWLIST_RDLOCK(&client->participants); if (dorelay) { - bbs_irc_client_send(client->name, channel, msg); /* Actually send to IRC */ + char prefix[32] = ""; + if (sender) { /* XXX Can this ever be NULL here? */ + snprintf(prefix, sizeof(prefix), "%s@%u", bbs_username(sender->node->user), sender->node->id); + } + bbs_irc_client_msg(client->name, channel, prefix, "%s", msg); /* Actually send to IRC */ } RWLIST_TRAVERSE(&client->participants, p, entry) { ssize_t res; @@ -415,6 +419,7 @@ static int irc_single_client(struct bbs_node *node, char *constring, const char char usernamebuf[24]; char passwordbuf[64]; char buf[2048]; + int underscores = 0; char *username, *password, *hostname, *portstr; /* Parse the arguments. @@ -584,11 +589,14 @@ static int irc_single_client(struct bbs_node *node, char *constring, const char * so just pass 0 as poll should always return > 0 anyways, immediately, * since we haven't read any data yet to quell the poll. */ - /* Another clunky thing. Need to get data using irc_read, but we want to buffer it using a bbs_node_readline struct. + /* Another clunky thing. Need to get data using irc_read, but we want to buffer it using a readline_data struct. * So use bbs_readline_append. */ bres = irc_read(ircl, tmpbuf, sizeof(tmpbuf)); - res = bbs_readline_append(&rldata, "\r\n", tmpbuf, (size_t) res, &ready); + if (bres <= 0) { + break; + } + res = bbs_readline_append(&rldata, "\r\n", tmpbuf, (size_t) bres, &ready); if (!ready) { continue; } @@ -609,6 +617,22 @@ static int irc_single_client(struct bbs_node *node, char *constring, const char switch (irc_msg_type(msg)) { case IRC_NUMERIC: bbs_node_writef(node, "%s %d %s\n", NODE_IS_TDD(node) ? "" : S_IF(irc_msg_prefix(msg)), irc_msg_numeric(msg), irc_msg_body(msg)); + switch (irc_msg_numeric(msg)) { + case 433: /* Nickname in use - very likely to happen if already logged in using a regular IRC client */ + if (underscores++ < 5) { + char newnick[32]; +#define UNDERSCORES "_____" + snprintf(newnick, sizeof(newnick), "%s%.*s", irc_client_username(ircl), underscores, UNDERSCORES); + bbs_debug(1, "Auto-changing nickname to '%s' to avoid conflict\n", newnick); + irc_client_change_nick(ircl, newnick); + /* Assuming this succeeds, try joining the channel we intended to, + * since that would have failed. */ + irc_client_channel_join(ircl, channel); + } + break; + default: + break; + } break; case IRC_CMD_PRIVMSG: case IRC_CMD_NOTICE: diff --git a/include/variables.h b/include/variables.h index 7e7b5abc..4e9eb877 100644 --- a/include/variables.h +++ b/include/variables.h @@ -137,7 +137,7 @@ const char *bbs_node_var_get(struct bbs_node *node, const char *key); * \param len Size of buf. * \retval 0 if found, -1 if not found */ -int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size_t len); +int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *restrict buf, size_t len); /*! * \brief Substitute variables in a string @@ -147,4 +147,4 @@ int bbs_node_var_get_buf(struct bbs_node *node, const char *key, char *buf, size * \param len Size of buf. * \retval 0 if found, -1 if not found */ -int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *buf, size_t len); +int bbs_node_substitute_vars(struct bbs_node *node, const char *sub, char *restrict buf, size_t len); diff --git a/nets/net_irc.c b/nets/net_irc.c index c5bc97e1..abeee75d 100644 --- a/nets/net_irc.c +++ b/nets/net_irc.c @@ -2282,12 +2282,38 @@ static void broadcast_nick_change(struct irc_user *user, const char *oldnick) RWLIST_UNLOCK(&channels); } +static void do_identity_update(struct irc_user *user) +{ + if (!user->registered) { + add_user(user); + } else { + hostmask(user); + } +} + static void handle_nick(struct irc_user *user, char *s) { if (user->node->user && strcasecmp(s, bbs_username(user->node->user))) { - /* Don't allow changing nick if already logged in, unless it's to our actual username. */ - send_numeric(user, 902, "You must use a nick assigned to you\r\n"); - } else if (bbs_user_exists(s)) { + const char *suffix; + size_t usernamelen = strlen(bbs_username(user->node->user)); + /* Don't allow changing nick if already logged in, unless it's to our actual username, + * or something that starts with it, followed by an underscore. */ + if (strncasecmp(s, bbs_username(user->node->user), usernamelen)) { + send_numeric(user, 902, "You must use a nick assigned to you\r\n"); + return; + } + /* If username is jsmith, allow jsmith_, jsmith__, jsmith_m, etc. + * But it has to be jsmith or start with jsmith_ */ + suffix = s + usernamelen; + /* If the first n characters match but not the whole string the suffix must be non-empty */ + bbs_assert(!strlen_zero(suffix)); + if (*suffix != '_') { + send_numeric(user, 902, "You must use a nick assigned to you\r\n"); + return; + } + } + + if (bbs_user_exists(s)) { send_numeric(user, 433, "%s :Nickname is already in use.\r\n", s); send_reply(user, "NOTICE AUTH :*** This nickname is registered. Please choose a different nickname, or identify using NickServ\r\n"); /* Client will need to send NS IDENTIFY or PRIVMSG NickServ :IDENTIFY */ @@ -2307,9 +2333,10 @@ static void handle_nick(struct irc_user *user, char *s) } user->nickname = newnick; RWLIST_UNLOCK(&users); + do_identity_update(user); if (!s_strlen_zero(oldnick)) { send_reply(user, ":%s NICK %s\r\n", oldnick, user->nickname); - broadcast_nick_change(user, oldnick); /* XXX Won't actually traverse, if registered users aren't allowed to change nicks? */ + broadcast_nick_change(user, oldnick); } } } @@ -2341,11 +2368,7 @@ static void handle_identify(struct irc_user *user, char *s) if (strlen_zero(user->nickname) || strcasecmp(username, user->nickname)) { REPLACE(user->nickname, username); } - if (!user->registered) { - add_user(user); - } else { - hostmask(user); - } + do_identity_update(user); send_numeric(user, 900, IDENT_PREFIX_FMT " %s You are now logged in as %s\r\n", IDENT_PREFIX_ARGS(user), user->username, user->username); } } @@ -2945,8 +2968,8 @@ static int do_sasl_auth(struct irc_user *user, char *s) send_numeric(user, 904, "SASL authentication failed\r\n"); return -1; } - send_numeric(user, 903, "SASL authentication successful\r\n"); /* The prefix is nick!ident@host */ + send_numeric(user, 903, "SASL authentication successful\r\n"); send_numeric(user, 900, IDENT_PREFIX_FMT " %s You are now logged in as %s\r\n", IDENT_PREFIX_ARGS(user), user->username, user->username); return 0; } @@ -3028,15 +3051,14 @@ static void handle_client(struct irc_user *user) if (!started) { /* Users that aren't started, and more importantly, in the user list, (!started, !user->registered) * can change their nickname arbitrarily, but can't use it without identifying. */ - user->nickname = strdup(s); + REPLACE(user->nickname, s); bbs_debug(5, "Nickname is %s\n", user->nickname); } } else if (!strcasecmp(command, "USER")) { /* Whole message is something like 'ambassador * * :New Now Know How' */ char *realname; bbs_debug(5, "Username data is %s\n", s); realname = strsep(&s, " "); - free_if(user->realname); - user->realname = strdup(realname); + REPLACE(user->realname, realname); if (handle_user(user)) { break; } @@ -3098,7 +3120,10 @@ static void handle_client(struct irc_user *user) bbs_error("Client %p already started?\n", user); } } else { + char *command; bbs_warning("Unhandled message: %s\n", s); + command = strsep(&s, " "); + send_numeric2(user, 421, "%s :Unknown command or invalid in current state\r\n", command); } } else { bbs_warning("Unhandled message: %s\n", s); @@ -3166,7 +3191,7 @@ static void handle_client(struct irc_user *user) send_reply(user, "NOTICE AUTH :*** This server requires SASL for authentication. Please reconnect with SASL enabled.\r\n"); goto quit; /* Disconnect at this point, there's no point in lingering around further. */ /* We can't necessarily use %s (user->username) instead of %p (user), since if require_sasl == false, we might not have a username still. */ - } else if (!user->node->user) { + } else if (!user->node->user || !user->registered) { char *target; /* Okay to message NickServ without being registered, but nobody else. */ /* Can be NS IDENTIFY or a regular PRIVMSG */ @@ -3186,7 +3211,13 @@ static void handle_client(struct irc_user *user) continue; } } - send_numeric(user, 451, "You have not registered\r\n"); + if (!user->node->user) { + send_numeric(user, 451, "You have not registered\r\n"); + } else { + send_numeric(user, 433, "Nickname is already in use\r\n"); + } + } else if (!user->registered) { + send_numeric(user, 433, "Nickname is already in use\r\n"); } else if (!strcasecmp(command, "NS")) { /* NickServ alias */ nickserv(user, s); } else if (!strcasecmp(command, "CS")) { /* ChanServ alias (much like NS ~ NickServ) */ @@ -3376,7 +3407,7 @@ static void handle_client(struct irc_user *user) /* Ignore SQUIT for now, since this is a single-server network */ } else { send_numeric2(user, 421, "%s :Unknown command\r\n", command); - bbs_warning("%p: Unhandled message: %s %s\n", user, command, s); + bbs_warning("%p: Unhandled message: %s %s\n", user, command, S_IF(s)); } } }