From bc59cf84015dd006e67900d4b26dd343fffc5868 Mon Sep 17 00:00:00 2001 From: Tommy Situ Date: Sun, 25 Aug 2024 13:52:11 +0100 Subject: [PATCH 1/2] Enhance validation for body file path --- core/hoverfly_funcs.go | 9 +++++-- core/util/util.go | 33 ++++++++++++++++++----- core/util/util_test.go | 61 +++++++++++++++++++++++++++++++++++------- 3 files changed, 86 insertions(+), 17 deletions(-) diff --git a/core/hoverfly_funcs.go b/core/hoverfly_funcs.go index 181d2f135..776ab79fd 100644 --- a/core/hoverfly_funcs.go +++ b/core/hoverfly_funcs.go @@ -2,8 +2,8 @@ package hoverfly import ( "fmt" - "io/ioutil" "net/http" + "os" "path/filepath" "strings" @@ -188,7 +188,12 @@ func (hf *Hoverfly) readResponseBodyFile(filePath string) (string, error) { return "", fmt.Errorf("bodyFile contains absolute path (%s). only relative is supported", filePath) } - fileContents, err := ioutil.ReadFile(filepath.Join(hf.Cfg.ResponsesBodyFilesPath, filePath)) + resolvedPath, err := util.ResolveAndValidatePath(hf.Cfg.ResponsesBodyFilesPath, filePath) + if err != nil { + return "", err + } + + fileContents, err := os.ReadFile(resolvedPath) if err != nil { return "", err } diff --git a/core/util/util.go b/core/util/util.go index 5493c60d2..e5eb537da 100644 --- a/core/util/util.go +++ b/core/util/util.go @@ -17,6 +17,7 @@ import ( "math/rand" "net/http" "net/url" + "path/filepath" "reflect" "regexp" "sort" @@ -517,11 +518,31 @@ func NeedsEncoding(headers map[string][]string, body string) bool { return needsEncoding } -func IsJsonData(data string) bool { - var jsonData map[string]interface{} - if err := json.Unmarshal([]byte(data), &jsonData); err == nil { - return true - } else { - return false +// Resolves a relative path from basePath, and fails if the relative path starts with ".." +func ResolveAndValidatePath(basePath, relativePath string) (string, error) { + absBasePath, err := filepath.Abs(basePath) + if err != nil { + return "", fmt.Errorf("failed to get absolute base path: %v", err) } + + cleanRelativePath := filepath.Clean(relativePath) + + // Check if the relative path starts with ".." + if strings.HasPrefix(cleanRelativePath, "..") { + return "", fmt.Errorf("relative path is invalid as it attempts to backtrack") + } + + resolvedPath := filepath.Join(absBasePath, cleanRelativePath) + + // Verify that the resolved path is indeed a subpath of the base path + finalPath, err := filepath.Rel(absBasePath, resolvedPath) + if err != nil { + return "", fmt.Errorf("failed to get relative path: %v", err) + } + + if strings.HasPrefix(finalPath, "..") { + return "", fmt.Errorf("resolved path is outside the base path") + } + + return resolvedPath, nil } diff --git a/core/util/util_test.go b/core/util/util_test.go index 731b60326..ddc888014 100644 --- a/core/util/util_test.go +++ b/core/util/util_test.go @@ -2,8 +2,10 @@ package util import ( "bytes" - "io/ioutil" + "io" "net/http" + "os" + "path/filepath" "testing" . "github.com/onsi/gomega" @@ -13,7 +15,7 @@ func Test_GetRequestBody_GettingTheRequestBodyGetsTheCorrectData(t *testing.T) { RegisterTestingT(t) request := &http.Request{} - request.Body = ioutil.NopCloser(bytes.NewBuffer([]byte("test"))) + request.Body = io.NopCloser(bytes.NewBuffer([]byte("test"))) requestBody, err := GetRequestBody(request) Expect(err).To(BeNil()) @@ -25,12 +27,12 @@ func Test_GetRequestBody_GettingTheRequestBodySetsTheSameBodyAgain(t *testing.T) RegisterTestingT(t) request := &http.Request{} - request.Body = ioutil.NopCloser(bytes.NewBuffer([]byte("test-preserve"))) + request.Body = io.NopCloser(bytes.NewBuffer([]byte("test-preserve"))) _, err := GetRequestBody(request) Expect(err).To(BeNil()) - newRequestBody, err := ioutil.ReadAll(request.Body) + newRequestBody, err := io.ReadAll(request.Body) Expect(err).To(BeNil()) Expect(string(newRequestBody)).To(Equal("test-preserve")) @@ -46,12 +48,12 @@ func Test_GetRequestBody_DecompressGzipContent(t *testing.T) { compressedBody, err := CompressGzip([]byte(originalBody)) Expect(err).To(BeNil()) Expect(string(compressedBody)).To(Not(Equal(originalBody))) - request.Body = ioutil.NopCloser(bytes.NewBuffer(compressedBody)) + request.Body = io.NopCloser(bytes.NewBuffer(compressedBody)) _, err = GetRequestBody(request) Expect(err).To(BeNil()) - newRequestBody, err := ioutil.ReadAll(request.Body) + newRequestBody, err := io.ReadAll(request.Body) Expect(err).To(BeNil()) Expect(string(newRequestBody)).To(Equal(originalBody)) @@ -61,7 +63,7 @@ func Test_GetResponseBody_GettingTheResponseBodyGetsTheCorrectData(t *testing.T) RegisterTestingT(t) response := &http.Response{} - response.Body = ioutil.NopCloser(bytes.NewBuffer([]byte("test"))) + response.Body = io.NopCloser(bytes.NewBuffer([]byte("test"))) responseBody, err := GetResponseBody(response) Expect(err).To(BeNil()) @@ -74,12 +76,12 @@ func Test_GetResponseBody_GettingTheResponseBodySetsTheSameBodyAgain(t *testing. RegisterTestingT(t) response := &http.Response{} - response.Body = ioutil.NopCloser(bytes.NewBuffer([]byte("test-preserve"))) + response.Body = io.NopCloser(bytes.NewBuffer([]byte("test-preserve"))) _, err := GetResponseBody(response) Expect(err).To(BeNil()) - newResponseBody, err := ioutil.ReadAll(response.Body) + newResponseBody, err := io.ReadAll(response.Body) Expect(err).To(BeNil()) Expect(string(newResponseBody)).To(Equal("test-preserve")) @@ -352,3 +354,44 @@ func Test_ContainsOnly_ReturnFalseWithOneExtraValue(t *testing.T) { Expect(ContainsOnly(first[:], second[:])).To(BeFalse()) } + +func TestResolveAndValidatePath(t *testing.T) { + RegisterTestingT(t) + + cwd, err := os.Getwd() + Expect(err).NotTo(HaveOccurred()) + + tests := []struct { + basePath string + relativePath string + expected string + shouldErr bool + }{ + {"/home/user/project", "subdir/file.txt", "/home/user/project/subdir/file.txt", false}, + {"/home/user/project", "../subdir/file.txt", "", true}, + {"/home/user/project", "../../etc/passwd", "", true}, + {"/home/user/project", "./subdir/file.txt", "/home/user/project/subdir/file.txt", false}, + {"/home/user/project", "subdir/../file.txt", "/home/user/project/file.txt", false}, + {"/home/user/project", ".", "/home/user/project", false}, + {"", "subdir/file.txt", filepath.Join(cwd, "subdir/file.txt"), false}, + {"", "", cwd, false}, + {"", ".", cwd, false}, + {"home/user/project", "subdir/file.txt", filepath.Join(cwd, "home/user/project/subdir/file.txt"), false}, + {"./home/user/project", "subdir/file.txt", filepath.Join(cwd, "home/user/project/subdir/file.txt"), false}, + } + + for _, test := range tests { + t.Run(test.relativePath, func(t *testing.T) { + result, err := ResolveAndValidatePath(test.basePath, test.relativePath) + if test.shouldErr { + Expect(err).To(HaveOccurred()) + } else { + Expect(err).NotTo(HaveOccurred()) + // Convert paths to absolute to avoid issues with relative paths in tests. + expectedAbs, _ := filepath.Abs(test.expected) + resultAbs, _ := filepath.Abs(result) + Expect(resultAbs).To(Equal(expectedAbs)) + } + }) + } +} From 3ab56a5f7220195128ba78264cd56644aff03afe Mon Sep 17 00:00:00 2001 From: Tommy Situ Date: Sun, 25 Aug 2024 14:25:31 +0100 Subject: [PATCH 2/2] Add validation for response body files base path --- core/cmd/hoverfly/main.go | 23 +++++++++++++++++++++-- core/util/util.go | 8 ++------ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/core/cmd/hoverfly/main.go b/core/cmd/hoverfly/main.go index 9536d3aa4..99263f774 100644 --- a/core/cmd/hoverfly/main.go +++ b/core/cmd/hoverfly/main.go @@ -27,6 +27,8 @@ import ( "io" "io/ioutil" "os" + "path" + "path/filepath" "strconv" "strings" "time" @@ -207,7 +209,7 @@ func main() { flag.Var(&templatingDataSourceFlags, "templating-data-source", "Set template data source (i.e. '-templating-data-source \" \"')") flag.Var(&destinationFlags, "dest", "Specify which hosts to process (i.e. '-dest fooservice.org -dest barservice.org -dest catservice.org') - other hosts will be ignored will passthrough'") flag.Var(&logOutputFlags, "logs-output", "Specify locations for output logs, options are \"console\" and \"file\" (default \"console\")") - flag.StringVar(&responseBodyFilesPath, "response-body-files-path", "", "When a response contains a relative bodyFile, it will be resolved against this path (default is CWD)") + flag.StringVar(&responseBodyFilesPath, "response-body-files-path", "", "When a response contains a relative bodyFile, it will be resolved against this absolute path (default is CWD)") flag.Var(&responseBodyFilesAllowedOriginFlags, "response-body-files-allow-origin", "When a response contains a url in bodyFile, it will be loaded only if the origin is allowed") flag.Var(&journalIndexingKeyFlags, "journal-indexing-key", "Key to setup indexing on journal") @@ -433,7 +435,24 @@ func main() { cfg.Destination = *destination } - cfg.ResponsesBodyFilesPath = responseBodyFilesPath + + if len(responseBodyFilesPath) > 0 { + // Ensure file path is absolute and exists in the file system + if !path.IsAbs(responseBodyFilesPath) { + log.Fatal("Response body files path should be absolute") + } + absBasePath, err := filepath.Abs(responseBodyFilesPath) + if err != nil { + log.Fatal("Invalid response body files path") + } + if _, err := os.Stat(absBasePath); os.IsNotExist(err) { + log.Fatal("Response body files path does not exist") + } + + cfg.ResponsesBodyFilesPath = absBasePath + } + + for _, allowedOrigin := range responseBodyFilesAllowedOriginFlags { if !util.IsURL(allowedOrigin) { diff --git a/core/util/util.go b/core/util/util.go index e5eb537da..33bfe2aa3 100644 --- a/core/util/util.go +++ b/core/util/util.go @@ -518,12 +518,8 @@ func NeedsEncoding(headers map[string][]string, body string) bool { return needsEncoding } -// Resolves a relative path from basePath, and fails if the relative path starts with ".." -func ResolveAndValidatePath(basePath, relativePath string) (string, error) { - absBasePath, err := filepath.Abs(basePath) - if err != nil { - return "", fmt.Errorf("failed to get absolute base path: %v", err) - } +// Resolves a relative path from an absolute basePath, and fails if the relative path starts with ".." +func ResolveAndValidatePath(absBasePath, relativePath string) (string, error) { cleanRelativePath := filepath.Clean(relativePath)