From ee7c92ec9b57c671c9091ff993b1a24251020c25 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 14 Nov 2022 11:38:02 -0500 Subject: [PATCH] reverseproxy: Mask the WS close message when we're the client (#5199) * reverseproxy: Mask the WS close message when we're the client * weakrand * Bump golangci-lint version so path ignores work on Windows * gofmt * ugh, gofmt everything, I guess --- .github/workflows/lint.yml | 2 +- .golangci.yml | 4 + caddyconfig/httpcaddyfile/pkiapp.go | 34 ++--- cmd/caddy/main.go | 8 +- modules/caddyhttp/caddyauth/caddyfile.go | 8 +- modules/caddyhttp/encode/caddyfile.go | 24 ++-- modules/caddyhttp/push/caddyfile.go | 14 +-- modules/caddyhttp/push/link.go | 6 +- modules/caddyhttp/push/link_test.go | 2 +- modules/caddyhttp/responsematchers.go | 11 +- .../reverseproxy/selectionpolicies.go | 3 +- modules/caddyhttp/reverseproxy/streaming.go | 118 +++++++++++++++--- modules/caddyhttp/rewrite/caddyfile.go | 13 +- modules/caddyhttp/staticerror.go | 6 +- modules/caddyhttp/templates/caddyfile.go | 11 +- modules/caddyhttp/templates/templates.go | 22 ++-- modules/caddypki/acmeserver/caddyfile.go | 7 +- modules/logging/filewriter.go | 16 +-- modules/logging/filterencoder.go | 16 +-- modules/logging/netwriter.go | 7 +- 20 files changed, 209 insertions(+), 123 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index c8580d39..7e56afc3 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -34,7 +34,7 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v3 with: - version: v1.47 + version: v1.50 # Windows times out frequently after about 5m50s if we don't set a longer timeout. args: --timeout 10m # Optional: show only new issues if it's a pull request. The default value is `false`. diff --git a/.golangci.yml b/.golangci.yml index 79fd8f2f..c36821cc 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -96,3 +96,7 @@ issues: text: "G404" # G404: Insecure random number source (rand) linters: - gosec + - path: modules/caddyhttp/reverseproxy/streaming.go + text: "G404" # G404: Insecure random number source (rand) + linters: + - gosec diff --git a/caddyconfig/httpcaddyfile/pkiapp.go b/caddyconfig/httpcaddyfile/pkiapp.go index f8aba9ff..a67ac992 100644 --- a/caddyconfig/httpcaddyfile/pkiapp.go +++ b/caddyconfig/httpcaddyfile/pkiapp.go @@ -26,23 +26,23 @@ func init() { // parsePKIApp parses the global log option. Syntax: // -// pki { -// ca [] { -// name -// root_cn -// intermediate_cn -// root { -// cert -// key -// format -// } -// intermediate { -// cert -// key -// format -// } -// } -// } +// pki { +// ca [] { +// name +// root_cn +// intermediate_cn +// root { +// cert +// key +// format +// } +// intermediate { +// cert +// key +// format +// } +// } +// } // // When the CA ID is unspecified, 'local' is assumed. func parsePKIApp(d *caddyfile.Dispenser, existingVal any) (any, error) { diff --git a/cmd/caddy/main.go b/cmd/caddy/main.go index ee706d58..48fa149a 100644 --- a/cmd/caddy/main.go +++ b/cmd/caddy/main.go @@ -19,10 +19,10 @@ // There is no need to modify the Caddy source code to customize your // builds. You can easily build a custom Caddy with these simple steps: // -// 1. Copy this file (main.go) into a new folder -// 2. Edit the imports below to include the modules you want plugged in -// 3. Run `go mod init caddy` -// 4. Run `go install` or `go build` - you now have a custom binary! +// 1. Copy this file (main.go) into a new folder +// 2. Edit the imports below to include the modules you want plugged in +// 3. Run `go mod init caddy` +// 4. Run `go install` or `go build` - you now have a custom binary! // // Or you can use xcaddy which does it all for you as a command: // https://github.com/caddyserver/xcaddy diff --git a/modules/caddyhttp/caddyauth/caddyfile.go b/modules/caddyhttp/caddyauth/caddyfile.go index afa38087..05c02320 100644 --- a/modules/caddyhttp/caddyauth/caddyfile.go +++ b/modules/caddyhttp/caddyauth/caddyfile.go @@ -27,10 +27,10 @@ func init() { // parseCaddyfile sets up the handler from Caddyfile tokens. Syntax: // -// basicauth [] [ []] { -// [] -// ... -// } +// basicauth [] [ []] { +// [] +// ... +// } // // If no hash algorithm is supplied, bcrypt will be assumed. func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) { diff --git a/modules/caddyhttp/encode/caddyfile.go b/modules/caddyhttp/encode/caddyfile.go index 2541b1aa..25d13f7c 100644 --- a/modules/caddyhttp/encode/caddyfile.go +++ b/modules/caddyhttp/encode/caddyfile.go @@ -39,18 +39,18 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) // UnmarshalCaddyfile sets up the handler from Caddyfile tokens. Syntax: // -// encode [] { -// gzip [] -// zstd -// minimum_length -// # response matcher block -// match { -// status -// header [] -// } -// # or response matcher single line syntax -// match [header []] | [status ] -// } +// encode [] { +// gzip [] +// zstd +// minimum_length +// # response matcher block +// match { +// status +// header [] +// } +// # or response matcher single line syntax +// match [header []] | [status ] +// } // // Specifying the formats on the first line will use those formats' defaults. func (enc *Encode) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { diff --git a/modules/caddyhttp/push/caddyfile.go b/modules/caddyhttp/push/caddyfile.go index 61b868c5..963117c1 100644 --- a/modules/caddyhttp/push/caddyfile.go +++ b/modules/caddyhttp/push/caddyfile.go @@ -26,13 +26,13 @@ func init() { // parseCaddyfile sets up the push handler. Syntax: // -// push [] [] { -// [GET|HEAD] -// headers { -// [+] [ []] -// - -// } -// } +// push [] [] { +// [GET|HEAD] +// headers { +// [+] [ []] +// - +// } +// } // // A single resource can be specified inline without opening a // block for the most common/simple case. Or, a block can be diff --git a/modules/caddyhttp/push/link.go b/modules/caddyhttp/push/link.go index f7c1dd89..855dffd0 100644 --- a/modules/caddyhttp/push/link.go +++ b/modules/caddyhttp/push/link.go @@ -29,9 +29,9 @@ type linkResource struct { // // Accepted formats are: // -// Link: ; as=script -// Link: ; as=script,; as=style -// Link: ; +// Link: ; as=script +// Link: ; as=script,; as=style +// Link: ; // // where begins with a forward slash (/). func parseLinkHeader(header string) []linkResource { diff --git a/modules/caddyhttp/push/link_test.go b/modules/caddyhttp/push/link_test.go index 238e284b..634bcb6d 100644 --- a/modules/caddyhttp/push/link_test.go +++ b/modules/caddyhttp/push/link_test.go @@ -4,7 +4,7 @@ // you may not use this file except in compliance with the License. // You may obtain a copy of the License at // -// http://www.apache.org/licenses/LICENSE-2.0 +// http://www.apache.org/licenses/LICENSE-2.0 // // Unless required by applicable law or agreed to in writing, software // distributed under the License is distributed on an "AS IS" BASIS, diff --git a/modules/caddyhttp/responsematchers.go b/modules/caddyhttp/responsematchers.go index 821def97..6bac7800 100644 --- a/modules/caddyhttp/responsematchers.go +++ b/modules/caddyhttp/responsematchers.go @@ -58,15 +58,14 @@ func (rm ResponseMatcher) matchStatusCode(statusCode int) bool { // ParseNamedResponseMatcher parses the tokens of a named response matcher. // -// @name { -// header [] -// status -// } +// @name { +// header [] +// status +// } // // Or, single line syntax: // -// @name [header []] | [status ] -// +// @name [header []] | [status ] func ParseNamedResponseMatcher(d *caddyfile.Dispenser, matchers map[string]ResponseMatcher) error { for d.Next() { definitionName := d.Val() diff --git a/modules/caddyhttp/reverseproxy/selectionpolicies.go b/modules/caddyhttp/reverseproxy/selectionpolicies.go index 5fc7136d..2de830c1 100644 --- a/modules/caddyhttp/reverseproxy/selectionpolicies.go +++ b/modules/caddyhttp/reverseproxy/selectionpolicies.go @@ -418,7 +418,8 @@ func (s CookieHashSelection) Select(pool UpstreamPool, req *http.Request, w http } // UnmarshalCaddyfile sets up the module from Caddyfile tokens. Syntax: -// lb_policy cookie [ []] +// +// lb_policy cookie [ []] // // By default name is `lb` func (s *CookieHashSelection) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { diff --git a/modules/caddyhttp/reverseproxy/streaming.go b/modules/caddyhttp/reverseproxy/streaming.go index 01d865db..834cb9e1 100644 --- a/modules/caddyhttp/reverseproxy/streaming.go +++ b/modules/caddyhttp/reverseproxy/streaming.go @@ -20,12 +20,13 @@ package reverseproxy import ( "context" - "encoding/binary" "io" + weakrand "math/rand" "mime" "net/http" "sync" "time" + "unsafe" "go.uber.org/zap" "golang.org/x/net/http/httpguts" @@ -103,16 +104,19 @@ func (h Handler) handleUpgradeResponse(logger *zap.Logger, rw http.ResponseWrite // with the backend, are both closed in the event of a server shutdown. This // is done by registering them. We also try to gracefully close connections // we recognize as websockets. - gracefulClose := func(conn io.ReadWriteCloser) func() error { + // We need to make sure the client connection messages (i.e. to upstream) + // are masked, so we need to know whether the connection is considered the + // server or the client side of the proxy. + gracefulClose := func(conn io.ReadWriteCloser, isClient bool) func() error { if isWebsocket(req) { return func() error { - return writeCloseControl(conn) + return writeCloseControl(conn, isClient) } } return nil } - deleteFrontConn := h.registerConnection(conn, gracefulClose(conn)) - deleteBackConn := h.registerConnection(backConn, gracefulClose(backConn)) + deleteFrontConn := h.registerConnection(conn, gracefulClose(conn, false)) + deleteBackConn := h.registerConnection(backConn, gracefulClose(backConn, true)) defer deleteFrontConn() defer deleteBackConn() @@ -248,27 +252,108 @@ func (h *Handler) registerConnection(conn io.ReadWriteCloser, gracefulClose func // writeCloseControl sends a best-effort Close control message to the given // WebSocket connection. Thanks to @pascaldekloe who provided inspiration // from his simple implementation of this I was able to learn from at: -// github.com/pascaldekloe/websocket. -func writeCloseControl(conn io.Writer) error { +// github.com/pascaldekloe/websocket. Further work for handling masking +// taken from github.com/gorilla/websocket. +func writeCloseControl(conn io.Writer, isClient bool) error { + // Sources: // https://github.com/pascaldekloe/websocket/blob/32050af67a5d/websocket.go#L119 + // https://github.com/gorilla/websocket/blob/v1.5.0/conn.go#L413 + // For now, we're not using a reason. We might later, though. + // The code handling the reason is left in var reason string // max 123 bytes (control frame payload limit is 125; status code takes 2) - const goingAway uint16 = 1001 - // TODO: we might need to ensure we are the exclusive writer by this point (io.Copy is stopped)? - var writeBuf [127]byte const closeMessage = 8 - const finalBit = 1 << 7 - writeBuf[0] = closeMessage | finalBit - writeBuf[1] = byte(len(reason) + 2) - binary.BigEndian.PutUint16(writeBuf[2:4], goingAway) - copy(writeBuf[4:], reason) + const finalBit = 1 << 7 // Frame header byte 0 bits from Section 5.2 of RFC 6455 + const maskBit = 1 << 7 // Frame header byte 1 bits from Section 5.2 of RFC 6455 + const goingAwayUpper uint8 = 1001 >> 8 + const goingAwayLower uint8 = 1001 & 0xff + + b0 := byte(closeMessage) | finalBit + b1 := byte(len(reason) + 2) + if isClient { + b1 |= maskBit + } + + buf := make([]byte, 0, 127) + buf = append(buf, b0, b1) + msgLength := 4 + len(reason) + + // Both branches below append the "going away" code and reason + appendMessage := func(buf []byte) []byte { + buf = append(buf, goingAwayUpper, goingAwayLower) + buf = append(buf, []byte(reason)...) + return buf + } + + // When we're the client, we need to mask the message as per + // https://www.rfc-editor.org/rfc/rfc6455#section-5.3 + if isClient { + key := newMaskKey() + buf = append(buf, key[:]...) + msgLength += len(key) + buf = appendMessage(buf) + maskBytes(key, 0, buf[2+len(key):]) + } else { + buf = appendMessage(buf) + } // simply best-effort, but return error for logging purposes - _, err := conn.Write(writeBuf[:4+len(reason)]) + // TODO: we might need to ensure we are the exclusive writer by this point (io.Copy is stopped)? + _, err := conn.Write(buf[:msgLength]) return err } +// Copied from https://github.com/gorilla/websocket/blob/v1.5.0/mask.go +func maskBytes(key [4]byte, pos int, b []byte) int { + // Mask one byte at a time for small buffers. + if len(b) < 2*wordSize { + for i := range b { + b[i] ^= key[pos&3] + pos++ + } + return pos & 3 + } + + // Mask one byte at a time to word boundary. + if n := int(uintptr(unsafe.Pointer(&b[0]))) % wordSize; n != 0 { + n = wordSize - n + for i := range b[:n] { + b[i] ^= key[pos&3] + pos++ + } + b = b[n:] + } + + // Create aligned word size key. + var k [wordSize]byte + for i := range k { + k[i] = key[(pos+i)&3] + } + kw := *(*uintptr)(unsafe.Pointer(&k)) + + // Mask one word at a time. + n := (len(b) / wordSize) * wordSize + for i := 0; i < n; i += wordSize { + *(*uintptr)(unsafe.Pointer(uintptr(unsafe.Pointer(&b[0])) + uintptr(i))) ^= kw + } + + // Mask one byte at a time for remaining bytes. + b = b[n:] + for i := range b { + b[i] ^= key[pos&3] + pos++ + } + + return pos & 3 +} + +// Copied from https://github.com/gorilla/websocket/blob/v1.5.0/conn.go#L184 +func newMaskKey() [4]byte { + n := weakrand.Uint32() + return [4]byte{byte(n), byte(n >> 8), byte(n >> 16), byte(n >> 24)} +} + // isWebsocket returns true if r looks to be an upgrade request for WebSockets. // It is a fairly naive check. func isWebsocket(r *http.Request) bool { @@ -364,3 +449,4 @@ var streamingBufPool = sync.Pool{ } const defaultBufferSize = 32 * 1024 +const wordSize = int(unsafe.Sizeof(uintptr(0))) diff --git a/modules/caddyhttp/rewrite/caddyfile.go b/modules/caddyhttp/rewrite/caddyfile.go index 15abbfa7..a34c1bb0 100644 --- a/modules/caddyhttp/rewrite/caddyfile.go +++ b/modules/caddyhttp/rewrite/caddyfile.go @@ -34,7 +34,7 @@ func init() { // parseCaddyfileRewrite sets up a basic rewrite handler from Caddyfile tokens. Syntax: // -// rewrite [] +// rewrite [] // // Only URI components which are given in will be set in the resulting URI. // See the docs for the rewrite handler for more information. @@ -54,8 +54,7 @@ func parseCaddyfileRewrite(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, // parseCaddyfileMethod sets up a basic method rewrite handler from Caddyfile tokens. Syntax: // -// method [] -// +// method [] func parseCaddyfileMethod(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) { var rewr Rewrite for h.Next() { @@ -73,7 +72,7 @@ func parseCaddyfileMethod(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, // parseCaddyfileURI sets up a handler for manipulating (but not "rewriting") the // URI from Caddyfile tokens. Syntax: // -// uri [] strip_prefix|strip_suffix|replace|path_regexp [ []] +// uri [] strip_prefix|strip_suffix|replace|path_regexp [ []] // // If strip_prefix or strip_suffix are used, then will be stripped // only if it is the beginning or the end, respectively, of the URI path. If @@ -147,9 +146,9 @@ func parseCaddyfileURI(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, err // parseCaddyfileHandlePath parses the handle_path directive. Syntax: // -// handle_path [] { -// -// } +// handle_path [] { +// +// } // // Only path matchers (with a `/` prefix) are supported as this is a shortcut // for the handle directive with a strip_prefix rewrite. diff --git a/modules/caddyhttp/staticerror.go b/modules/caddyhttp/staticerror.go index 914e6c14..7cf1a3e9 100644 --- a/modules/caddyhttp/staticerror.go +++ b/modules/caddyhttp/staticerror.go @@ -53,9 +53,9 @@ func (StaticError) CaddyModule() caddy.ModuleInfo { // UnmarshalCaddyfile sets up the handler from Caddyfile tokens. Syntax: // -// error [] | [] { -// message -// } +// error [] | [] { +// message +// } // // If there is just one argument (other than the matcher), it is considered // to be a status code if it's a valid positive integer of 3 digits. diff --git a/modules/caddyhttp/templates/caddyfile.go b/modules/caddyhttp/templates/caddyfile.go index ea6aa9f2..06ca3e26 100644 --- a/modules/caddyhttp/templates/caddyfile.go +++ b/modules/caddyhttp/templates/caddyfile.go @@ -25,12 +25,11 @@ func init() { // parseCaddyfile sets up the handler from Caddyfile tokens. Syntax: // -// templates [] { -// mime -// between -// root -// } -// +// templates [] { +// mime +// between +// root +// } func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) { t := new(Templates) for h.Next() { diff --git a/modules/caddyhttp/templates/templates.go b/modules/caddyhttp/templates/templates.go index e32e1c3a..449536ac 100644 --- a/modules/caddyhttp/templates/templates.go +++ b/modules/caddyhttp/templates/templates.go @@ -152,10 +152,10 @@ func init() { // // Accesses the current HTTP request, which has various fields, including: // -// - `.Method` - the method -// - `.URL` - the URL, which in turn has component fields (Scheme, Host, Path, etc.) -// - `.Header` - the header fields -// - `.Host` - the Host or :authority header of the request +// - `.Method` - the method +// - `.URL` - the URL, which in turn has component fields (Scheme, Host, Path, etc.) +// - `.Header` - the header fields +// - `.Host` - the Host or :authority header of the request // // ``` // {{.Req.Header.Get "User-Agent"}} @@ -221,15 +221,16 @@ func init() { // --- // ``` // -// // **JSON** is simply `{` and `}`: // // ``` -// { -// "template": "blog", -// "title": "Blog Homepage", -// "sitename": "A Caddy site" -// } +// +// { +// "template": "blog", +// "title": "Blog Homepage", +// "sitename": "A Caddy site" +// } +// // ``` // // The resulting front matter will be made available like so: @@ -237,7 +238,6 @@ func init() { // - `.Meta` to access the metadata fields, for example: `{{$parsed.Meta.title}}` // - `.Body` to access the body after the front matter, for example: `{{markdown $parsed.Body}}` // -// // ##### `stripHTML` // // Removes HTML from a string. diff --git a/modules/caddypki/acmeserver/caddyfile.go b/modules/caddypki/acmeserver/caddyfile.go index 9ac0bb20..fe12712b 100644 --- a/modules/caddypki/acmeserver/caddyfile.go +++ b/modules/caddypki/acmeserver/caddyfile.go @@ -25,10 +25,9 @@ func init() { // parseACMEServer sets up an ACME server handler from Caddyfile tokens. // -// acme_server [] { -// ca -// } -// +// acme_server [] { +// ca +// } func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) { if !h.Next() { return nil, h.ArgErr() diff --git a/modules/logging/filewriter.go b/modules/logging/filewriter.go index c0b5715d..f902a6db 100644 --- a/modules/logging/filewriter.go +++ b/modules/logging/filewriter.go @@ -131,14 +131,14 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { // UnmarshalCaddyfile sets up the module from Caddyfile tokens. Syntax: // -// file { -// roll_disabled -// roll_size -// roll_uncompressed -// roll_local_time -// roll_keep -// roll_keep_for -// } +// file { +// roll_disabled +// roll_size +// roll_uncompressed +// roll_local_time +// roll_keep +// roll_keep_for +// } // // The roll_size value has megabyte resolution. // Fractional values are rounded up to the next whole megabyte (MiB). diff --git a/modules/logging/filterencoder.go b/modules/logging/filterencoder.go index 6a768dd6..1a6842e1 100644 --- a/modules/logging/filterencoder.go +++ b/modules/logging/filterencoder.go @@ -98,14 +98,14 @@ func (fe *FilterEncoder) Provision(ctx caddy.Context) error { // UnmarshalCaddyfile sets up the module from Caddyfile tokens. Syntax: // -// filter { -// wrap -// fields { -// { -// -// } -// } -// } +// filter { +// wrap +// fields { +// { +// +// } +// } +// } func (fe *FilterEncoder) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { for d.Next() { for d.NextBlock(0) { diff --git a/modules/logging/netwriter.go b/modules/logging/netwriter.go index 2bf23bf0..5a6cf394 100644 --- a/modules/logging/netwriter.go +++ b/modules/logging/netwriter.go @@ -102,10 +102,9 @@ func (nw NetWriter) OpenWriter() (io.WriteCloser, error) { // UnmarshalCaddyfile sets up the handler from Caddyfile tokens. Syntax: // -// net
{ -// dial_timeout -// } -// +// net
{ +// dial_timeout +// } func (nw *NetWriter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { for d.Next() { if !d.NextArg() {