Skip to content

Commit 9d4ed3a

Browse files
mholtmholt
authored
caddyhttp: Refactor and export SanitizedPathJoin for use in fastcgi (#4207)
1 parent fbd6560 commit 9d4ed3a

File tree

8 files changed

+131
-134
lines changed

8 files changed

+131
-134
lines changed

‎modules/caddyhttp/caddyhttp.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ import (
2020
"io"
2121
"net"
2222
"net/http"
23+
"path/filepath"
2324
"strconv"
25+
"strings"
2426

2527
"github.com/caddyserver/caddy/v2"
2628
"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
@@ -217,6 +219,31 @@ func StatusCodeMatches(actual, configured int) bool {
217219
return false
218220
}
219221

222+
// SanitizedPathJoin performs filepath.Join(root, reqPath) that
223+
// is safe against directory traversal attacks. It uses logic
224+
// similar to that in the Go standard library, specifically
225+
// in the implementation of http.Dir. The root is assumed to
226+
// be a trusted path, but reqPath is not; and the output will
227+
// never be outside of root. The resulting path can be used
228+
// with the local file system.
229+
func SanitizedPathJoin(root, reqPath string) string {
230+
if root == "" {
231+
root = "."
232+
}
233+
234+
path := filepath.Join(root, filepath.Clean("/"+reqPath))
235+
236+
// filepath.Join also cleans the path, and cleaning strips
237+
// the trailing slash, so we need to re-add it afterwards.
238+
// if the length is 1, then it's a path to the root,
239+
// and that should return ".", so we don't append the separator.
240+
if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
241+
path += separator
242+
}
243+
244+
return path
245+
}
246+
220247
// tlsPlaceholderWrapper is a no-op listener wrapper that marks
221248
// where the TLS listener should be in a chain of listener wrappers.
222249
// It should only be used if another listener wrapper must be placed
@@ -242,6 +269,8 @@ const (
242269
DefaultHTTPSPort = 443
243270
)
244271

272+
const separator = string(filepath.Separator)
273+
245274
// Interface guard
246275
var _ caddy.ListenerWrapper = (*tlsPlaceholderWrapper)(nil)
247276
var _ caddyfile.Unmarshaler = (*tlsPlaceholderWrapper)(nil)

‎modules/caddyhttp/caddyhttp_test.go‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
package caddyhttp
2+
3+
import (
4+
"net/url"
5+
"path/filepath"
6+
"testing"
7+
)
8+
9+
func TestSanitizedPathJoin(t *testing.T) {
10+
// For reference:
11+
// %2e = .
12+
// %2f = /
13+
// %5c = \
14+
for i, tc := range []struct {
15+
inputRoot string
16+
inputPath string
17+
expect string
18+
}{
19+
{
20+
inputPath: "",
21+
expect: ".",
22+
},
23+
{
24+
inputPath: "/",
25+
expect: ".",
26+
},
27+
{
28+
inputPath: "/foo",
29+
expect: "foo",
30+
},
31+
{
32+
inputPath: "/foo/",
33+
expect: "foo" + separator,
34+
},
35+
{
36+
inputPath: "/foo/bar",
37+
expect: filepath.Join("foo", "bar"),
38+
},
39+
{
40+
inputRoot: "/a",
41+
inputPath: "/foo/bar",
42+
expect: filepath.Join("/", "a", "foo", "bar"),
43+
},
44+
{
45+
inputPath: "/foo/../bar",
46+
expect: "bar",
47+
},
48+
{
49+
inputRoot: "/a/b",
50+
inputPath: "/foo/../bar",
51+
expect: filepath.Join("/", "a", "b", "bar"),
52+
},
53+
{
54+
inputRoot: "/a/b",
55+
inputPath: "/..%2fbar",
56+
expect: filepath.Join("/", "a", "b", "bar"),
57+
},
58+
{
59+
inputRoot: "/a/b",
60+
inputPath: "/%2e%2e%2fbar",
61+
expect: filepath.Join("/", "a", "b", "bar"),
62+
},
63+
{
64+
inputRoot: "/a/b",
65+
inputPath: "/%2e%2e%2f%2e%2e%2f",
66+
expect: filepath.Join("/", "a", "b") + separator,
67+
},
68+
{
69+
inputRoot: "C:\\www",
70+
inputPath: "/foo/bar",
71+
expect: filepath.Join("C:\\www", "foo", "bar"),
72+
},
73+
{
74+
inputRoot: "C:\\www",
75+
inputPath: "/D:\\foo\\bar",
76+
expect: filepath.Join("C:\\www", "D:\\foo\\bar"),
77+
},
78+
} {
79+
// we don't *need* to use an actual parsed URL, but it
80+
// adds some authenticity to the tests since real-world
81+
// values will be coming in from URLs; thus, the test
82+
// corpus can contain paths as encoded by clients, which
83+
// more closely emulates the actual attack vector
84+
u, err := url.Parse("http://test:9999" + tc.inputPath)
85+
if err != nil {
86+
t.Fatalf("Test %d: invalid URL: %v", i, err)
87+
}
88+
actual := SanitizedPathJoin(tc.inputRoot, u.Path)
89+
if actual != tc.expect {
90+
t.Errorf("Test %d: SanitizedPathJoin('%s', '%s') => %s (expected '%s')",
91+
i, tc.inputRoot, tc.inputPath, actual, tc.expect)
92+
}
93+
}
94+
}

‎modules/caddyhttp/fileserver/browse.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ func isSymlinkTargetDir(f os.FileInfo, root, urlPath string) bool {
207207
if !isSymlink(f) {
208208
return false
209209
}
210-
target := sanitizedPathJoin(root, path.Join(urlPath, f.Name()))
210+
target := caddyhttp.SanitizedPathJoin(root, path.Join(urlPath, f.Name()))
211211
targetInfo, err := os.Stat(target)
212212
if err != nil {
213213
return false

‎modules/caddyhttp/fileserver/matcher.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) {
185185
if strings.HasSuffix(file, "/") {
186186
suffix += "/"
187187
}
188-
fullpath = sanitizedPathJoin(root, suffix)
188+
fullpath = caddyhttp.SanitizedPathJoin(root, suffix)
189189
return
190190
}
191191

‎modules/caddyhttp/fileserver/matcher_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ func TestFileMatcher(t *testing.T) {
9494
t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
9595
}
9696

97-
fileType, ok := repl.Get("http.matchers.file.type")
97+
fileType, _ := repl.Get("http.matchers.file.type")
9898
if fileType != tc.expectedType {
9999
t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
100100
}
@@ -197,7 +197,7 @@ func TestPHPFileMatcher(t *testing.T) {
197197
t.Fatalf("Test %d: actual path: %v, expected: %v", i, rel, tc.expectedPath)
198198
}
199199

200-
fileType, ok := repl.Get("http.matchers.file.type")
200+
fileType, _ := repl.Get("http.matchers.file.type")
201201
if fileType != tc.expectedType {
202202
t.Fatalf("Test %d: actual file type: %v, expected: %v", i, fileType, tc.expectedType)
203203
}

‎modules/caddyhttp/fileserver/staticfiles.go‎

Lines changed: 2 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
161161
filesToHide := fsrv.transformHidePaths(repl)
162162

163163
root := repl.ReplaceAll(fsrv.Root, ".")
164-
filename := sanitizedPathJoin(root, r.URL.Path)
164+
filename := caddyhttp.SanitizedPathJoin(root, r.URL.Path)
165165

166166
fsrv.logger.Debug("sanitized path join",
167167
zap.String("site_root", root),
@@ -185,7 +185,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, next c
185185
var implicitIndexFile bool
186186
if info.IsDir() && len(fsrv.IndexNames) > 0 {
187187
for _, indexPage := range fsrv.IndexNames {
188-
indexPath := sanitizedPathJoin(filename, indexPage)
188+
indexPath := caddyhttp.SanitizedPathJoin(filename, indexPage)
189189
if fileHidden(indexPath, filesToHide) {
190190
// pretend this file doesn't exist
191191
fsrv.logger.Debug("hiding index file",
@@ -435,42 +435,6 @@ func (fsrv *FileServer) transformHidePaths(repl *caddy.Replacer) []string {
435435
return hide
436436
}
437437

438-
// sanitizedPathJoin performs filepath.Join(root, reqPath) that
439-
// is safe against directory traversal attacks. It uses logic
440-
// similar to that in the Go standard library, specifically
441-
// in the implementation of http.Dir. The root is assumed to
442-
// be a trusted path, but reqPath is not.
443-
func sanitizedPathJoin(root, reqPath string) string {
444-
// TODO: Caddy 1 uses this:
445-
// prevent absolute path access on Windows, e.g. http://localhost:5000/C:\Windows\notepad.exe
446-
// if runtime.GOOS == "windows" && len(reqPath) > 0 && filepath.IsAbs(reqPath[1:]) {
447-
// TODO.
448-
// }
449-
450-
// TODO: whereas std lib's http.Dir.Open() uses this:
451-
// if filepath.Separator != '/' && strings.ContainsRune(name, filepath.Separator) {
452-
// return nil, errors.New("http: invalid character in file path")
453-
// }
454-
455-
// TODO: see https://play.golang.org/p/oh77BiVQFti for another thing to consider
456-
457-
if root == "" {
458-
root = "."
459-
}
460-
461-
path := filepath.Join(root, filepath.Clean("/"+reqPath))
462-
463-
// filepath.Join also cleans the path, and cleaning strips
464-
// the trailing slash, so we need to re-add it afterwards.
465-
// if the length is 1, then it's a path to the root,
466-
// and that should return ".", so we don't append the separator.
467-
if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
468-
path += separator
469-
}
470-
471-
return path
472-
}
473-
474438
// fileHidden returns true if filename is hidden according to the hide list.
475439
// filename must be a relative or absolute file system path, not a request
476440
// URI path. It is expected that all the paths in the hide list are absolute

‎modules/caddyhttp/fileserver/staticfiles_test.go‎

Lines changed: 0 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -15,96 +15,12 @@
1515
package fileserver
1616

1717
import (
18-
"net/url"
1918
"path/filepath"
2019
"runtime"
2120
"strings"
2221
"testing"
2322
)
2423

25-
func TestSanitizedPathJoin(t *testing.T) {
26-
// For easy reference:
27-
// %2e = .
28-
// %2f = /
29-
// %5c = \
30-
for i, tc := range []struct {
31-
inputRoot string
32-
inputPath string
33-
expect string
34-
}{
35-
{
36-
inputPath: "",
37-
expect: ".",
38-
},
39-
{
40-
inputPath: "/",
41-
expect: ".",
42-
},
43-
{
44-
inputPath: "/foo",
45-
expect: "foo",
46-
},
47-
{
48-
inputPath: "/foo/",
49-
expect: "foo" + separator,
50-
},
51-
{
52-
inputPath: "/foo/bar",
53-
expect: filepath.Join("foo", "bar"),
54-
},
55-
{
56-
inputRoot: "/a",
57-
inputPath: "/foo/bar",
58-
expect: filepath.Join("/", "a", "foo", "bar"),
59-
},
60-
{
61-
inputPath: "/foo/../bar",
62-
expect: "bar",
63-
},
64-
{
65-
inputRoot: "/a/b",
66-
inputPath: "/foo/../bar",
67-
expect: filepath.Join("/", "a", "b", "bar"),
68-
},
69-
{
70-
inputRoot: "/a/b",
71-
inputPath: "/..%2fbar",
72-
expect: filepath.Join("/", "a", "b", "bar"),
73-
},
74-
{
75-
inputRoot: "/a/b",
76-
inputPath: "/%2e%2e%2fbar",
77-
expect: filepath.Join("/", "a", "b", "bar"),
78-
},
79-
{
80-
inputRoot: "/a/b",
81-
inputPath: "/%2e%2e%2f%2e%2e%2f",
82-
expect: filepath.Join("/", "a", "b") + separator,
83-
},
84-
{
85-
inputRoot: "C:\\www",
86-
inputPath: "/foo/bar",
87-
expect: filepath.Join("C:\\www", "foo", "bar"),
88-
},
89-
// TODO: test more windows paths... on windows... sigh.
90-
} {
91-
// we don't *need* to use an actual parsed URL, but it
92-
// adds some authenticity to the tests since real-world
93-
// values will be coming in from URLs; thus, the test
94-
// corpus can contain paths as encoded by clients, which
95-
// more closely emulates the actual attack vector
96-
u, err := url.Parse("http://test:9999" + tc.inputPath)
97-
if err != nil {
98-
t.Fatalf("Test %d: invalid URL: %v", i, err)
99-
}
100-
actual := sanitizedPathJoin(tc.inputRoot, u.Path)
101-
if actual != tc.expect {
102-
t.Errorf("Test %d: [%s %s] => %s (expected %s)",
103-
i, tc.inputRoot, tc.inputPath, actual, tc.expect)
104-
}
105-
}
106-
}
107-
10824
func TestFileHidden(t *testing.T) {
10925
for i, tc := range []struct {
11026
inputHide []string

‎modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"fmt"
2121
"net"
2222
"net/http"
23-
"path"
2423
"path/filepath"
2524
"strconv"
2625
"strings"
@@ -218,12 +217,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
218217
}
219218

220219
// SCRIPT_FILENAME is the absolute path of SCRIPT_NAME
221-
scriptFilename := filepath.Join(root, scriptName)
222-
223-
// Add vhost path prefix to scriptName. Otherwise, some PHP software will
224-
// have difficulty discovering its URL.
225-
pathPrefix, _ := r.Context().Value(caddy.CtxKey("path_prefix")).(string)
226-
scriptName = path.Join(pathPrefix, scriptName)
220+
scriptFilename := caddyhttp.SanitizedPathJoin(root, scriptName)
227221

228222
// Ensure the SCRIPT_NAME has a leading slash for compliance with RFC3875
229223
// Info: https://tools.ietf.org/html/rfc3875#section-4.1.13
@@ -288,7 +282,7 @@ func (t Transport) buildEnv(r *http.Request) (map[string]string, error) {
288282
// PATH_TRANSLATED should only exist if PATH_INFO is defined.
289283
// Info: https://www.ietf.org/rfc/rfc3875 Page 14
290284
if env["PATH_INFO"] != "" {
291-
env["PATH_TRANSLATED"] = filepath.Join(root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
285+
env["PATH_TRANSLATED"] = caddyhttp.SanitizedPathJoin(root, pathInfo) // Info: http://www.oreilly.com/openbook/cgi/ch02_04.html
292286
}
293287

294288
// compliance with the CGI specification requires that

0 commit comments

Comments
 (0)