From 9a9820fa8867ee075fbf466e617a3555309f1315 Mon Sep 17 00:00:00 2001 From: Daniel Oaks Date: Wed, 16 Nov 2016 12:02:22 +1000 Subject: [PATCH] NICK: Prevent races, remove a DoS --- CHANGELOG.md | 2 ++ irc/client.go | 27 ++++++---------- irc/client_lookup_set.go | 67 +++++++++++++++++++++++----------------- irc/nickname.go | 11 +++---- irc/server.go | 1 - 5 files changed, 54 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eea7b4a8..e529f64f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ New release of Oragono! ### Removed ### Fixed +* Prevented a DoS related to lots of clients connecting at once. +* Removed races around setting and changing `NICK`s, to be more safe. * Fixed crash when using STATUSMSG-like messaging. * Fixed crash with gIRC-Go ircmsg library we depend on. diff --git a/irc/client.go b/irc/client.go index 237ed1a3..08918fcb 100644 --- a/irc/client.go +++ b/irc/client.go @@ -14,8 +14,6 @@ import ( "strconv" "time" - "strings" - "github.com/DanielOaks/girc-go/ircmsg" "github.com/DanielOaks/go-ident" ) @@ -146,14 +144,12 @@ func (client *Client) run() { client.rawHostname = AddrLookupHostname(client.socket.conn.RemoteAddr()) //TODO(dan): Make this a socketreactor from ircbnc - fmt.Println("START", &client) for { line, err = client.socket.Read() if err != nil { client.Quit("connection closed") break } - fmt.Println(" LINE", &client, strings.TrimSpace(line)) msg, err = ircmsg.ParseLine(line) if err != nil { @@ -163,7 +159,6 @@ func (client *Client) run() { cmd, exists := Commands[msg.Command] if !exists { - fmt.Println(" BADLINE", &client, strings.TrimSpace(line)) if len(msg.Command) > 0 { client.Send(nil, client.server.name, ERR_UNKNOWNCOMMAND, client.nick, msg.Command, "Unknown command") } else { @@ -171,15 +166,11 @@ func (client *Client) run() { } continue } - fmt.Println(" GUDLINE", &client, strings.TrimSpace(line)) isExiting = cmd.Run(client.server, client, msg) - fmt.Println(" CMDRUN", &client, strings.TrimSpace(line)) if isExiting || client.isQuitting { - fmt.Println(" BREAKING", &client, strings.TrimSpace(line)) break } - fmt.Println(" CONTINUE", &client, strings.TrimSpace(line)) } // ensure client connection gets closed @@ -368,22 +359,22 @@ func (client *Client) SetNickname(nickname string) error { client.nick = nickname client.updateNick() - client.server.clients.Add(client) - return nil + return client.server.clients.Add(client) } // ChangeNickname changes the existing nickname of the client. func (client *Client) ChangeNickname(nickname string) error { origNickMask := client.nickMaskString - client.server.clients.Remove(client) - client.server.whoWas.Append(client) - client.nick = nickname client.updateNickMask() - client.server.clients.Add(client) - for friend := range client.Friends() { - friend.Send(nil, origNickMask, "NICK", nickname) + err := client.server.clients.Replace(client.nick, nickname, client) + if err == nil { + client.server.whoWas.Append(client) + client.nick = nickname + for friend := range client.Friends() { + friend.Send(nil, origNickMask, "NICK", nickname) + } } - return nil + return err } func (client *Client) Quit(message string) { diff --git a/irc/client_lookup_set.go b/irc/client_lookup_set.go index e754b7b4..cdd4e350 100644 --- a/irc/client_lookup_set.go +++ b/irc/client_lookup_set.go @@ -13,8 +13,6 @@ import ( "sync" - "runtime/debug" - "github.com/DanielOaks/girc-go/ircmatch" ) @@ -37,24 +35,8 @@ func ExpandUserHost(userhost string) (expanded string) { return } -type LoggingMutex struct { - mutex sync.Mutex -} - -func (lm *LoggingMutex) Lock() { - lm.mutex.Lock() - fmt.Println("--- locked, stack:") - debug.PrintStack() -} - -func (lm *LoggingMutex) Unlock() { - fmt.Println("--- unlocking") - lm.mutex.Unlock() - fmt.Println("--- unlocked") -} - type ClientLookupSet struct { - ByNickMutex LoggingMutex + ByNickMutex sync.Mutex ByNick map[string]*Client } @@ -83,6 +65,7 @@ func (clients *ClientLookupSet) Has(nick string) bool { return exists } +// getNoMutex is used internally, for getting clients when no mutex is required (i.e. is already set). func (clients *ClientLookupSet) getNoMutex(nick string) *Client { casefoldedName, err := CasefoldName(nick) if err == nil { @@ -104,23 +87,15 @@ func (clients *ClientLookupSet) Get(nick string) *Client { } func (clients *ClientLookupSet) Add(client *Client) error { - fmt.Println("Initial nick:", client.nick) if !client.HasNick() { return ErrNickMissing } - fmt.Println("- getting lock:", client.nick) clients.ByNickMutex.Lock() - fmt.Println("- got lock:", client.nick) + defer clients.ByNickMutex.Unlock() if clients.getNoMutex(client.nick) != nil { - fmt.Println("- already exists:", client.nick) - clients.ByNickMutex.Unlock() return ErrNicknameInUse } - fmt.Println("- adding:", client.nick) clients.ByNick[client.nickCasefolded] = client - fmt.Println("- set:", client.nick) - clients.ByNickMutex.Unlock() - fmt.Println("- released lock:", client.nick) return nil } @@ -128,15 +103,49 @@ func (clients *ClientLookupSet) Remove(client *Client) error { if !client.HasNick() { return ErrNickMissing } + clients.ByNickMutex.Lock() if clients.getNoMutex(client.nick) != client { + clients.ByNickMutex.Unlock() return ErrNicknameMismatch } - clients.ByNickMutex.Lock() delete(clients.ByNick, client.nickCasefolded) clients.ByNickMutex.Unlock() return nil } +func (clients *ClientLookupSet) Replace(oldNick, newNick string, client *Client) error { + // get casefolded nicknames + oldNick, err := CasefoldName(oldNick) + if err != nil { + return err + } + newNick, err = CasefoldName(newNick) + if err != nil { + return err + } + + // remove and replace + clients.ByNickMutex.Lock() + defer clients.ByNickMutex.Unlock() + + oldClient := clients.getNoMutex(newNick) + if oldClient != nil { + return ErrNicknameInUse + } + if oldClient != client { + return ErrNicknameMismatch + } + + if oldNick == newNick { + // if they're only changing case, don't need to remove+re-add them + return nil + } + + delete(clients.ByNick, oldNick) + clients.ByNick[newNick] = client + return nil +} + func (clients *ClientLookupSet) AllWithCaps(caps ...Capability) (set ClientSet) { set = make(ClientSet) diff --git a/irc/nickname.go b/irc/nickname.go index 4c26146a..a32ab6a1 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -5,6 +5,7 @@ package irc import ( + "fmt" "strings" "github.com/DanielOaks/girc-go/ircmsg" @@ -35,19 +36,17 @@ func nickHandler(server *Server, client *Client, msg ircmsg.IrcMessage) bool { } // bleh, this will be replaced and done below - target := server.clients.Get(nickname) - if target != nil && target != client { - client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "Nickname is already in use") - return false - } if client.registered { err = client.ChangeNickname(nicknameRaw) } else { err = client.SetNickname(nicknameRaw) } - if err != nil { + if err == ErrNicknameInUse { client.Send(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nicknameRaw, "Nickname is already in use") return false + } else if err != nil { + client.Send(nil, server.name, ERR_UNKNOWNERROR, client.nick, "NICK", fmt.Sprintf("Could not set or change nickname: %s", err.Error())) + return false } if client.registered { client.alertMonitors() diff --git a/irc/server.go b/irc/server.go index dfc2a1d6..90faee53 100644 --- a/irc/server.go +++ b/irc/server.go @@ -567,7 +567,6 @@ func (server *Server) wslisten(addr string, tlsMap map[string]*TLSListenConfig) func (server *Server) tryRegister(c *Client) { if c.registered || !c.HasNick() || !c.HasUsername() || (c.capState == CapNegotiating) { - fmt.Println("Try Reg:", &c, c.registered, c.HasNick(), c.HasUsername(), c.capState == CapNegotiating, c.capState) return } c.Register()