From 3fe2c73dd04f7769a9d9673236cb94b79ac45659 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Thu, 30 Dec 2021 04:15:48 -0500 Subject: [PATCH 1/7] caddyhttp: Fix `MatchPath` sanitizing (#4499) This is a followup to #4407, in response to a report on the forums: https://caddy.community/t/php-fastcgi-phishing-redirection/14542 Turns out that doing `TrimRight` to remove trailing dots, _before_ cleaning the path, will cause double-dots at the end of the path to not be cleaned away as they should. We should instead remove the dots _after_ cleaning. --- modules/caddyhttp/matchers.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/caddyhttp/matchers.go b/modules/caddyhttp/matchers.go index 439c40730e53..272c92421fd3 100644 --- a/modules/caddyhttp/matchers.go +++ b/modules/caddyhttp/matchers.go @@ -325,6 +325,11 @@ func (m MatchPath) Match(r *http.Request) bool { lowerPath := strings.ToLower(unescapedPath) + // Clean the path, merges doubled slashes, etc. + // This ensures maliciously crafted requests can't bypass + // the path matcher. See #4407 + lowerPath = path.Clean(lowerPath) + // see #2917; Windows ignores trailing dots and spaces // when accessing files (sigh), potentially causing a // security risk (cry) if PHP files end up being served @@ -332,11 +337,6 @@ func (m MatchPath) Match(r *http.Request) bool { // being matched by *.php to be treated as PHP scripts lowerPath = strings.TrimRight(lowerPath, ". ") - // Clean the path, merges doubled slashes, etc. - // This ensures maliciously crafted requests can't bypass - // the path matcher. See #4407 - lowerPath = path.Clean(lowerPath) - // Cleaning may remove the trailing slash, but we want to keep it if lowerPath != "/" && strings.HasSuffix(r.URL.Path, "/") { lowerPath = lowerPath + "/" From e9dde230247ec5525d4c3803ff99c97e0dc8e348 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Tue, 4 Jan 2022 12:10:11 -0500 Subject: [PATCH 2/7] headers: Fix `+` in Caddyfile to properly append rather than set (#4506) --- caddytest/integration/caddyfile_adapt/header.txt | 15 +++++++++++++++ modules/caddyhttp/headers/caddyfile.go | 2 +- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/caddytest/integration/caddyfile_adapt/header.txt b/caddytest/integration/caddyfile_adapt/header.txt index 223839efaa81..34a044dffc1e 100644 --- a/caddytest/integration/caddyfile_adapt/header.txt +++ b/caddytest/integration/caddyfile_adapt/header.txt @@ -13,6 +13,10 @@ header @images { Cache-Control "public, max-age=3600, stale-while-revalidate=86400" } + header { + +Link "Foo" + +Link "Bar" + } } ---------- { @@ -121,6 +125,17 @@ ] } } + }, + { + "handler": "headers", + "response": { + "add": { + "Link": [ + "Foo", + "Bar" + ] + } + } } ] } diff --git a/modules/caddyhttp/headers/caddyfile.go b/modules/caddyhttp/headers/caddyfile.go index c6ea2fb0372e..eec11490c26a 100644 --- a/modules/caddyhttp/headers/caddyfile.go +++ b/modules/caddyhttp/headers/caddyfile.go @@ -222,7 +222,7 @@ func applyHeaderOp(ops *HeaderOps, respHeaderOps *RespHeaderOps, field, value, r if ops.Add == nil { ops.Add = make(http.Header) } - ops.Add.Set(field[1:], value) + ops.Add.Add(field[1:], value) case strings.HasPrefix(field, "-"): // delete ops.Delete = append(ops.Delete, field[1:]) From 249adc1c872ae46e640cfb7c330e332229d8d32a Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Tue, 4 Jan 2022 14:11:27 -0500 Subject: [PATCH 3/7] logging: Support turning off roll compression via Caddyfile (#4505) --- .../integration/caddyfile_adapt/log_roll_days.txt | 2 ++ modules/logging/filewriter.go | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/caddytest/integration/caddyfile_adapt/log_roll_days.txt b/caddytest/integration/caddyfile_adapt/log_roll_days.txt index 2d146f9c8319..5762c92b978a 100644 --- a/caddytest/integration/caddyfile_adapt/log_roll_days.txt +++ b/caddytest/integration/caddyfile_adapt/log_roll_days.txt @@ -3,6 +3,7 @@ log { output file /var/log/access.log { roll_size 1gb + roll_uncompressed roll_keep 5 roll_keep_for 90d } @@ -20,6 +21,7 @@ log { "writer": { "filename": "/var/log/access.log", "output": "file", + "roll_gzip": false, "roll_keep": 5, "roll_keep_days": 90, "roll_size_mb": 954 diff --git a/modules/logging/filewriter.go b/modules/logging/filewriter.go index 376deeb2e2b0..7333fb20e7f1 100644 --- a/modules/logging/filewriter.go +++ b/modules/logging/filewriter.go @@ -134,6 +134,7 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { // file { // roll_disabled // roll_size +// roll_uncompressed // roll_keep // roll_keep_for // } @@ -141,6 +142,9 @@ func (fw FileWriter) OpenWriter() (io.WriteCloser, error) { // The roll_size value has megabyte resolution. // Fractional values are rounded up to the next whole megabyte (MiB). // +// By default, compression is enabled, but can be turned off by setting +// the roll_uncompressed option. +// // The roll_keep_for duration has day resolution. // Fractional values are rounded up to the next whole number of days. // @@ -177,6 +181,13 @@ func (fw *FileWriter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { } fw.RollSizeMB = int(math.Ceil(float64(size) / humanize.MiByte)) + case "roll_uncompressed": + var f bool + fw.RollCompress = &f + if d.NextArg() { + return d.ArgErr() + } + case "roll_keep": var keepStr string if !d.AllArgs(&keepStr) { From 2e46c2ac1d860230a80de5fd7bc85c8c9429d3f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=94=D0=B5=D0=BD=D0=B8=D1=81=20=D0=A2=D0=B5=D0=BB=D1=8E?= =?UTF-8?q?=D1=85?= Date: Wed, 5 Jan 2022 02:14:18 +0700 Subject: [PATCH 4/7] admin, reverseproxy: Stop timers if canceled to avoid goroutine leak (#4482) --- caddy.go | 7 ++++++- modules/caddyhttp/reverseproxy/reverseproxy.go | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/caddy.go b/caddy.go index bee4274c7d3b..f990d362cfab 100644 --- a/caddy.go +++ b/caddy.go @@ -494,9 +494,10 @@ func finishSettingUp(ctx Context, cfg *Config) error { if cfg.Admin.Config.LoadInterval > 0 { go func() { for { + timer := time.NewTimer(time.Duration(cfg.Admin.Config.LoadInterval)) select { // if LoadInterval is positive, will wait for the interval and then run with new config - case <-time.After(time.Duration(cfg.Admin.Config.LoadInterval)): + case <-timer.C: loadedConfig, err := val.(ConfigLoader).LoadConfig(ctx) if err != nil { Log().Error("loading dynamic config failed", zap.Error(err)) @@ -504,6 +505,10 @@ func finishSettingUp(ctx Context, cfg *Config) error { } runLoadedConfig(loadedConfig) case <-ctx.Done(): + if !timer.Stop() { + // if the timer has been stopped then read from the channel + <-timer.C + } Log().Info("stopping config load interval") return } diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index b4189535fd92..eaa7cbf928ea 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -792,10 +792,15 @@ func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, proxyErr er } // otherwise, wait and try the next available host + timer := time.NewTimer(time.Duration(lb.TryInterval)) select { - case <-time.After(time.Duration(lb.TryInterval)): + case <-timer.C: return true case <-ctx.Done(): + if !timer.Stop() { + // if the timer has been stopped then read from the channel + <-timer.C + } return false } } From 6cadb60fa2308f24a20a32c3f8d5e3a521c277ff Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 5 Jan 2022 13:59:59 -0700 Subject: [PATCH 5/7] templates: Document .OriginalReq Close caddyserver/website#91 --- modules/caddyhttp/templates/templates.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/caddyhttp/templates/templates.go b/modules/caddyhttp/templates/templates.go index 80efded45305..f7332a9ac5df 100644 --- a/modules/caddyhttp/templates/templates.go +++ b/modules/caddyhttp/templates/templates.go @@ -153,6 +153,10 @@ func init() { // {{.Req.Header.Get "User-Agent"}} // ``` // +// ##### `.OriginalReq` +// +// Like .Req, except it accesses the original HTTP request before rewrites or other internal modifications. +// // ##### `.RespHeader.Add` // // Adds a header field to the HTTP response. From b4bfa29be2191ffacfa4ed747bd5cdce8da8917f Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 5 Jan 2022 17:55:09 -0700 Subject: [PATCH 6/7] admin: Require identity for remote (fix #4478) --- admin.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/admin.go b/admin.go index 6960de52ab31..0a7b93307988 100644 --- a/admin.go +++ b/admin.go @@ -466,6 +466,9 @@ func replaceRemoteAdminServer(ctx Context, cfg *Config) error { } // create TLS config that will enforce mutual authentication + if identityCertCache == nil { + return fmt.Errorf("cannot enable remote admin without a certificate cache; configure identity management to initialize a certificate cache") + } cmCfg := cfg.Admin.Identity.certmagicConfig(remoteLogger, false) tlsConfig := cmCfg.TLSConfig() tlsConfig.NextProtos = nil // this server does not solve ACME challenges From 80d7a356b3443e0a994e5d6abfa6082ba3d5e6e7 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Wed, 5 Jan 2022 20:01:15 -0500 Subject: [PATCH 7/7] caddyhttp: Redirect HTTP requests on the HTTPS port to https:// (#4313) * caddyhttp: Redirect HTTP requests on the HTTPS port to https:// * Apply suggestions from code review Co-authored-by: Matt Holt Co-authored-by: Matt Holt --- modules/caddyhttp/app.go | 5 + modules/caddyhttp/httpredirectlistener.go | 114 ++++++++++++++++++++++ 2 files changed, 119 insertions(+) create mode 100644 modules/caddyhttp/httpredirectlistener.go diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 64cc5401b914..67f9d1d68fcf 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -343,6 +343,11 @@ func (app *App) Start() error { // enable TLS if there is a policy and if this is not the HTTP port useTLS := len(srv.TLSConnPolicies) > 0 && int(listenAddr.StartPort+portOffset) != app.httpPort() if useTLS { + // create HTTP redirect wrapper, which detects if + // the request had HTTP bytes on the HTTPS port, and + // triggers a redirect if so. + ln = &httpRedirectListener{Listener: ln} + // create TLS listener tlsCfg := srv.TLSConnPolicies.TLSConfig(app.ctx) ln = tls.NewListener(ln, tlsCfg) diff --git a/modules/caddyhttp/httpredirectlistener.go b/modules/caddyhttp/httpredirectlistener.go new file mode 100644 index 000000000000..38225a3d262d --- /dev/null +++ b/modules/caddyhttp/httpredirectlistener.go @@ -0,0 +1,114 @@ +// Copyright 2015 Matthew Holt and The Caddy Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// 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 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package caddyhttp + +import ( + "bufio" + "fmt" + "net" + "net/http" + "sync" +) + +// httpRedirectListener is listener that checks the first few bytes +// of the request when the server is intended to accept HTTPS requests, +// to respond to an HTTP request with a redirect. +type httpRedirectListener struct { + net.Listener +} + +// Accept waits for and returns the next connection to the listener, +// wrapping it with a httpRedirectConn. +func (l *httpRedirectListener) Accept() (net.Conn, error) { + c, err := l.Listener.Accept() + if err != nil { + return nil, err + } + + return &httpRedirectConn{ + Conn: c, + r: bufio.NewReader(c), + }, nil +} + +type httpRedirectConn struct { + net.Conn + once sync.Once + r *bufio.Reader +} + +// Read tries to peek at the first few bytes of the request, and if we get +// an error reading the headers, and that error was due to the bytes looking +// like an HTTP request, then we perform a HTTP->HTTPS redirect on the same +// port as the original connection. +func (c *httpRedirectConn) Read(p []byte) (int, error) { + var errReturn error + c.once.Do(func() { + firstBytes, err := c.r.Peek(5) + if err != nil { + return + } + + // If the request doesn't look like HTTP, then it's probably + // TLS bytes and we don't need to do anything. + if !firstBytesLookLikeHTTP(firstBytes) { + return + } + + // Parse the HTTP request, so we can get the Host and URL to redirect to. + req, err := http.ReadRequest(c.r) + if err != nil { + return + } + + // Build the redirect response, using the same Host and URL, + // but replacing the scheme with https. + headers := make(http.Header) + headers.Add("Location", "https://"+req.Host+req.URL.String()) + resp := &http.Response{ + Proto: "HTTP/1.0", + Status: "308 Permanent Redirect", + StatusCode: 308, + ProtoMajor: 1, + ProtoMinor: 0, + Header: headers, + } + + err = resp.Write(c.Conn) + if err != nil { + errReturn = fmt.Errorf("couldn't write HTTP->HTTPS redirect") + return + } + + errReturn = fmt.Errorf("redirected HTTP request on HTTPS port") + c.Conn.Close() + }) + + if errReturn != nil { + return 0, errReturn + } + + return c.r.Read(p) +} + +// firstBytesLookLikeHTTP reports whether a TLS record header +// looks like it might've been a misdirected plaintext HTTP request. +func firstBytesLookLikeHTTP(hdr []byte) bool { + switch string(hdr[:5]) { + case "GET /", "HEAD ", "POST ", "PUT /", "OPTIO": + return true + } + return false +}