From c92192ef48560941d8b2f0d2b7789690aea3cb36 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Tue, 5 May 2020 17:20:50 -0400 Subject: [PATCH] review fixes; add submatch support to glob --- conventional.yaml | 9 +++++---- irc/client.go | 2 +- irc/config.go | 13 ++++++------- irc/ircconn.go | 41 ++++++++++++++++++++--------------------- irc/listeners.go | 20 ++++++++++---------- irc/server.go | 24 ++++++++++++------------ irc/socket.go | 6 +++--- irc/utils/glob.go | 21 +++++++++++---------- irc/utils/glob_test.go | 11 +++++++++++ irc/utils/net_test.go | 10 ++++++++++ irc/utils/proxy.go | 6 +++--- oragono.yaml | 9 +++++---- 12 files changed, 97 insertions(+), 75 deletions(-) diff --git a/conventional.yaml b/conventional.yaml index 0fe1990b..d6af832a 100644 --- a/conventional.yaml +++ b/conventional.yaml @@ -89,10 +89,11 @@ server: preload: false websockets: - # sets the Origin headers that will be accepted for websocket connections. - # an empty list means any value (or no value) is allowed. the main use of this - # is to prevent malicious third-party Javascript from co-opting non-malicious - # clients (i.e., mainstream browsers) to DDoS your server. + # Restrict the origin of WebSocket connections by matching the "Origin" HTTP + # header. This settings makes oragono reject every WebSocket connection, + # except when it originates from one of the hosts in this list. Use this to + # prevent malicious websites from making their visitors connect to oragono + # without their knowledge. An empty list means that there are no restrictions. allowed-origins: # - "https://oragono.io" # - "https://*.oragono.io" diff --git a/irc/client.go b/irc/client.go index 6e016433..c387cacc 100644 --- a/irc/client.go +++ b/irc/client.go @@ -277,7 +277,7 @@ func (server *Server) RunClient(conn IRCConn) { if isBanned { // this might not show up properly on some clients, // but our objective here is just to close the connection out before it has a load impact on us - conn.Write([]byte(fmt.Sprintf(errorMsg, banMsg))) + conn.WriteLine([]byte(fmt.Sprintf(errorMsg, banMsg))) conn.Close() return } diff --git a/irc/config.go b/irc/config.go index fd7742d9..5695214d 100644 --- a/irc/config.go +++ b/irc/config.go @@ -772,7 +772,7 @@ func (conf *Config) prepareListeners() (err error) { conf.Server.trueListeners = make(map[string]utils.ListenerConfig) for addr, block := range conf.Server.Listeners { var lconf utils.ListenerConfig - lconf.ProxyDeadline = time.Minute + lconf.ProxyDeadline = RegisterTimeout lconf.Tor = block.Tor lconf.STSOnly = block.STSOnly if lconf.STSOnly && !conf.Server.STS.Enabled { @@ -1217,20 +1217,19 @@ func (config *Config) Diff(oldConfig *Config) (addedCaps, removedCaps *caps.Set) } func compileGuestRegexp(guestFormat string, casemapping Casemapping) (standard, folded *regexp.Regexp, err error) { + if strings.Count(guestFormat, "?") != 0 || strings.Count(guestFormat, "*") != 1 { + err = errors.New("guest format must contain 1 '*' and no '?'s") + return + } + standard, err = utils.CompileGlob(guestFormat) if err != nil { return } starIndex := strings.IndexByte(guestFormat, '*') - if starIndex == -1 { - return nil, nil, errors.New("guest format must contain exactly one *") - } initial := guestFormat[:starIndex] final := guestFormat[starIndex+1:] - if strings.IndexByte(final, '*') != -1 { - return nil, nil, errors.New("guest format must contain exactly one *") - } initialFolded, err := casefoldWithSetting(initial, casemapping) if err != nil { return diff --git a/irc/ircconn.go b/irc/ircconn.go index cb5d906d..bf53bad4 100644 --- a/irc/ircconn.go +++ b/irc/ircconn.go @@ -22,13 +22,16 @@ var ( // IRCConn abstracts away the distinction between a regular // net.Conn (which includes both raw TCP and TLS) and a websocket. -// it doesn't expose Read and Write because websockets are message-oriented, -// not stream-oriented. +// it doesn't expose the net.Conn, io.Reader, or io.Writer interfaces +// because websockets are message-oriented, not stream-oriented, and +// therefore this abstraction is message-oriented as well. type IRCConn interface { - UnderlyingConn() *utils.ProxiedConnection + UnderlyingConn() *utils.WrappedConn - Write([]byte) error - WriteBuffers([][]byte) error + // these take an IRC line or lines, correctly terminated with CRLF: + WriteLine([]byte) error + WriteLines([][]byte) error + // this returns an IRC line without the terminating CRLF: ReadLine() (line []byte, err error) Close() error @@ -36,26 +39,26 @@ type IRCConn interface { // IRCStreamConn is an IRCConn over a regular stream connection. type IRCStreamConn struct { - conn *utils.ProxiedConnection + conn *utils.WrappedConn reader *bufio.Reader } -func NewIRCStreamConn(conn *utils.ProxiedConnection) *IRCStreamConn { +func NewIRCStreamConn(conn *utils.WrappedConn) *IRCStreamConn { return &IRCStreamConn{ conn: conn, } } -func (cc *IRCStreamConn) UnderlyingConn() *utils.ProxiedConnection { +func (cc *IRCStreamConn) UnderlyingConn() *utils.WrappedConn { return cc.conn } -func (cc *IRCStreamConn) Write(buf []byte) (err error) { +func (cc *IRCStreamConn) WriteLine(buf []byte) (err error) { _, err = cc.conn.Write(buf) return } -func (cc *IRCStreamConn) WriteBuffers(buffers [][]byte) (err error) { +func (cc *IRCStreamConn) WriteLines(buffers [][]byte) (err error) { // on Linux, with a plaintext TCP or Unix domain socket, // the Go runtime will optimize this into a single writev(2) call: _, err = (*net.Buffers)(&buffers).WriteTo(cc.conn) @@ -90,17 +93,13 @@ func NewIRCWSConn(conn *websocket.Conn) IRCWSConn { return IRCWSConn{conn: conn} } -func (wc IRCWSConn) UnderlyingConn() *utils.ProxiedConnection { - pConn, ok := wc.conn.UnderlyingConn().(*utils.ProxiedConnection) - if ok { - return pConn - } else { - // this can't happen - return nil - } +func (wc IRCWSConn) UnderlyingConn() *utils.WrappedConn { + // just assume that the type is OK + wConn, _ := wc.conn.UnderlyingConn().(*utils.WrappedConn) + return wConn } -func (wc IRCWSConn) Write(buf []byte) (err error) { +func (wc IRCWSConn) WriteLine(buf []byte) (err error) { buf = bytes.TrimSuffix(buf, crlf) // there's not much we can do about this; // silently drop the message @@ -110,9 +109,9 @@ func (wc IRCWSConn) Write(buf []byte) (err error) { return wc.conn.WriteMessage(websocket.TextMessage, buf) } -func (wc IRCWSConn) WriteBuffers(buffers [][]byte) (err error) { +func (wc IRCWSConn) WriteLines(buffers [][]byte) (err error) { for _, buf := range buffers { - err = wc.Write(buf) + err = wc.WriteLine(buf) if err != nil { return } diff --git a/irc/listeners.go b/irc/listeners.go index 545f76f2..c6e6054f 100644 --- a/irc/listeners.go +++ b/irc/listeners.go @@ -89,7 +89,7 @@ func (nl *NetListener) Stop() error { } // ensure that any IP we got from the PROXY line is trustworthy (otherwise, clear it) -func validateProxiedIP(conn *utils.ProxiedConnection, config *Config) { +func validateProxiedIP(conn *utils.WrappedConn, config *Config) { if !utils.IPInNets(utils.AddrToIP(conn.RemoteAddr()), config.Server.proxyAllowedFromNets) { conn.ProxiedIP = nil } @@ -101,12 +101,12 @@ func (nl *NetListener) serve() { if err == nil { // hand off the connection - pConn, ok := conn.(*utils.ProxiedConnection) + wConn, ok := conn.(*utils.WrappedConn) if ok { - if pConn.ProxiedIP != nil { - validateProxiedIP(pConn, nl.server.Config()) + if wConn.ProxiedIP != nil { + validateProxiedIP(wConn, nl.server.Config()) } - go nl.server.RunClient(NewIRCStreamConn(pConn)) + go nl.server.RunClient(NewIRCStreamConn(wConn)) } else { nl.server.logger.Error("internal", "invalid connection type", nl.addr) } @@ -186,19 +186,19 @@ func (wl *WSListener) handle(w http.ResponseWriter, r *http.Request) { return } - pConn, ok := conn.UnderlyingConn().(*utils.ProxiedConnection) + wConn, ok := conn.UnderlyingConn().(*utils.WrappedConn) if !ok { wl.server.logger.Error("internal", "non-proxied connection on websocket", wl.addr) conn.Close() return } - if pConn.ProxiedIP != nil { - validateProxiedIP(pConn, config) + if wConn.ProxiedIP != nil { + validateProxiedIP(wConn, config) } else { // if there was no PROXY protocol IP, use the validated X-Forwarded-For IP instead, // unless it is redundant - if proxiedIP != nil && !proxiedIP.Equal(utils.AddrToIP(pConn.RemoteAddr())) { - pConn.ProxiedIP = proxiedIP + if proxiedIP != nil && !proxiedIP.Equal(utils.AddrToIP(wConn.RemoteAddr())) { + wConn.ProxiedIP = proxiedIP } } diff --git a/irc/server.go b/irc/server.go index b8bfbb40..1d8a970c 100644 --- a/irc/server.go +++ b/irc/server.go @@ -736,16 +736,16 @@ func (server *Server) setupListeners(config *Config) (err error) { newConfig, stillConfigured := config.Server.trueListeners[addr] if stillConfigured { - err := currentListener.Reload(newConfig) + reloadErr := currentListener.Reload(newConfig) // attempt to stop and replace the listener if the reload failed - if err != nil { + if reloadErr != nil { currentListener.Stop() - newListener, err := NewListener(server, addr, newConfig, config.Server.UnixBindMode) - if err != nil { - delete(server.listeners, addr) - return err - } else { + newListener, newErr := NewListener(server, addr, newConfig, config.Server.UnixBindMode) + if newErr == nil { server.listeners[addr] = newListener + } else { + delete(server.listeners, addr) + return newErr } } logListener(addr, newConfig) @@ -765,13 +765,13 @@ func (server *Server) setupListeners(config *Config) (err error) { _, exists := server.listeners[newAddr] if !exists { // make a new listener - newListener, listenerErr := NewListener(server, newAddr, newConfig, config.Server.UnixBindMode) - if listenerErr != nil { - server.logger.Error("server", "couldn't listen on", newAddr, listenerErr.Error()) - err = listenerErr - } else { + newListener, newErr := NewListener(server, newAddr, newConfig, config.Server.UnixBindMode) + if newErr == nil { server.listeners[newAddr] = newListener logListener(newAddr, newConfig) + } else { + server.logger.Error("server", "couldn't listen on", newAddr, newErr.Error()) + err = newErr } } } diff --git a/irc/socket.go b/irc/socket.go index b0fd28b8..289b7e2e 100644 --- a/irc/socket.go +++ b/irc/socket.go @@ -144,7 +144,7 @@ func (socket *Socket) BlockingWrite(data []byte) (err error) { return io.EOF } - err = socket.conn.Write(data) + err = socket.conn.WriteLine(data) if err != nil { socket.finalize() } @@ -216,7 +216,7 @@ func (socket *Socket) performWrite() (closed bool) { var err error if 0 < len(buffers) { - err = socket.conn.WriteBuffers(buffers) + err = socket.conn.WriteLines(buffers) } closed = closed || err != nil @@ -244,7 +244,7 @@ func (socket *Socket) finalize() { } if len(finalData) != 0 { - socket.conn.Write(finalData) + socket.conn.WriteLine(finalData) } // close the connection diff --git a/irc/utils/glob.go b/irc/utils/glob.go index 863a6067..58fb7f34 100644 --- a/irc/utils/glob.go +++ b/irc/utils/glob.go @@ -6,7 +6,7 @@ package utils import ( "bytes" "regexp" - "strings" + "regexp/syntax" ) // yet another glob implementation in Go @@ -14,15 +14,16 @@ import ( func CompileGlob(glob string) (result *regexp.Regexp, err error) { var buf bytes.Buffer buf.WriteByte('^') - for { - i := strings.IndexByte(glob, '*') - if i == -1 { - buf.WriteString(regexp.QuoteMeta(glob)) - break - } else { - buf.WriteString(regexp.QuoteMeta(glob[:i])) - buf.WriteString(".*") - glob = glob[i+1:] + for _, r := range glob { + switch r { + case '*': + buf.WriteString("(.*)") + case '?': + buf.WriteString("(.)") + case 0xFFFD: + return nil, &syntax.Error{Code: syntax.ErrInvalidUTF8, Expr: glob} + default: + buf.WriteString(regexp.QuoteMeta(string(r))) } } buf.WriteByte('$') diff --git a/irc/utils/glob_test.go b/irc/utils/glob_test.go index c7c158b6..2c9dee35 100644 --- a/irc/utils/glob_test.go +++ b/irc/utils/glob_test.go @@ -29,9 +29,20 @@ func TestGlob(t *testing.T) { assertMatches("*://*.oragono.io", "https://testnet.oragono.io", true, t) assertMatches("*://*.oragono.io", "https://oragono.io", false, t) assertMatches("*://*.oragono.io", "https://githubusercontent.com", false, t) + assertMatches("*://*.oragono.io", "https://testnet.oragono.io.example.com", false, t) assertMatches("", "", true, t) assertMatches("", "x", false, t) assertMatches("*", "", true, t) assertMatches("*", "x", true, t) + + assertMatches("c?b", "cab", true, t) + assertMatches("c?b", "cub", true, t) + assertMatches("c?b", "cb", false, t) + assertMatches("c?b", "cube", false, t) + assertMatches("?*", "cube", true, t) + assertMatches("?*", "", false, t) + + assertMatches("S*e", "Skåne", true, t) + assertMatches("Sk?ne", "Skåne", true, t) } diff --git a/irc/utils/net_test.go b/irc/utils/net_test.go index ec71885e..27bcddae 100644 --- a/irc/utils/net_test.go +++ b/irc/utils/net_test.go @@ -190,8 +190,18 @@ func TestXForwardedFor(t *testing.T) { checkXFF("10.0.0.4:28432", "10.0.0.3", "10.0.0.3", t) checkXFF("10.0.0.4:28432", "1.1.1.1, 8.8.4.4", "8.8.4.4", t) + checkXFF("10.0.0.4:28432", "1.1.1.1,8.8.4.4", "8.8.4.4", t) checkXFF("10.0.0.4:28432", "8.8.4.4, 1.1.1.1, 10.0.0.3", "1.1.1.1", t) checkXFF("10.0.0.4:28432", "10.0.0.1, 10.0.0.2, 10.0.0.3", "10.0.0.1", t) + checkXFF("10.0.0.4:28432", "10.0.0.1, 10.0.0.2,10.0.0.3", "10.0.0.1", t) checkXFF("@", "8.8.4.4, 1.1.1.1, 10.0.0.3", "1.1.1.1", t) + + // invalid IP tests: + checkXFF("8.8.4.4:9999", "not_an_ip", "8.8.4.4", t) + checkXFF("10.0.0.4:28432", "not_an_ip", "10.0.0.4", t) + checkXFF("10.0.0.4:28432", "not_an_ip, 1.1.1.1, 10.0.0.3", "1.1.1.1", t) + + checkXFF("10.0.0.4:28432", "8.8.4.4, not_an_ip, 10.0.0.3", "10.0.0.3", t) + checkXFF("10.0.0.4:28432", "8.8.4.4, not_an_ip", "10.0.0.4", t) } diff --git a/irc/utils/proxy.go b/irc/utils/proxy.go index 569373cd..805d06a8 100644 --- a/irc/utils/proxy.go +++ b/irc/utils/proxy.go @@ -86,10 +86,10 @@ func ParseProxyLine(line string) (ip net.IP, err error) { return ip.To16(), nil } -/// ProxiedConnection is a net.Conn with some additional data stapled to it; +/// WrappedConn is a net.Conn with some additional data stapled to it; // the proxied IP, if one was read via the PROXY protocol, and the listener // configuration. -type ProxiedConnection struct { +type WrappedConn struct { net.Conn ProxiedIP net.IP Config ListenerConfig @@ -154,7 +154,7 @@ func (rl *ReloadableListener) Accept() (conn net.Conn, err error) { conn = tls.Server(conn, config.TLSConfig) } - return &ProxiedConnection{ + return &WrappedConn{ Conn: conn, ProxiedIP: proxiedIP, Config: config, diff --git a/oragono.yaml b/oragono.yaml index 4d896a13..c4e68de3 100644 --- a/oragono.yaml +++ b/oragono.yaml @@ -110,10 +110,11 @@ server: preload: false websockets: - # sets the Origin headers that will be accepted for websocket connections. - # an empty list means any value (or no value) is allowed. the main use of this - # is to prevent malicious third-party Javascript from co-opting non-malicious - # clients (i.e., mainstream browsers) to DDoS your server. + # Restrict the origin of WebSocket connections by matching the "Origin" HTTP + # header. This settings makes oragono reject every WebSocket connection, + # except when it originates from one of the hosts in this list. Use this to + # prevent malicious websites from making their visitors connect to oragono + # without their knowledge. An empty list means that there are no restrictions. allowed-origins: # - "https://oragono.io" # - "https://*.oragono.io"