From c6419883d2fd6f9791ded327a1b3c03c7198acd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Fri, 6 May 2022 17:35:18 +0200 Subject: [PATCH 1/4] Extend safeTemplateFilepath to cover more cases. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - template name ../tmpfile, stored into /tmp dir - empty template name - template name being just "." Signed-off-by: Peter Štibraný --- pkg/alertmanager/multitenant.go | 28 ++++++++++++++++++- pkg/alertmanager/multitenant_test.go | 42 ++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/pkg/alertmanager/multitenant.go b/pkg/alertmanager/multitenant.go index bf2edd6b2cd..33df9bc210f 100644 --- a/pkg/alertmanager/multitenant.go +++ b/pkg/alertmanager/multitenant.go @@ -1262,16 +1262,42 @@ func safeTemplateFilepath(dir, templateName string) (string, error) { return "", err } + // If actualPath is same as containerDir, it's likely that actualPath was empty, or just ".". + if containerDir == actualPath { + return "", fmt.Errorf("invalid template name %q", templateName) + } + // Ensure the actual path of the template is within the expected directory. // This check is a counter-measure to make sure the tenant is not trying to // escape its own directory on disk. - if !strings.HasPrefix(actualPath, containerDir) { + if !isFilePathInsideDirectory(containerDir, actualPath) { return "", fmt.Errorf("invalid template name %q: the template filepath is escaping the per-tenant local directory", templateName) } return actualPath, nil } +// Returns true if file described by filePath is inside given directory. (Doesn't check for symbolic links) +// Directory argument should not end with separator, ie. it must be filepath.Clean-ed. +func isFilePathInsideDirectory(absDir string, absFilePath string) bool { + if absFilePath == absDir { + // If filePath refers to dir itself, it's not a "file" path. + return false + } + + d := filepath.Dir(absFilePath) + if d == absDir { + return true + } + + // If path was empty, its Dir is ".". + // If path was "/", its Dir is just "/". + if d == "." || d == "/" { + return false + } + return isFilePathInsideDirectory(absDir, d) +} + // storeTemplateFile stores template file at the given templateFilepath. // Returns true, if file content has changed (new or updated file), false if file with the same name // and content was already stored locally. diff --git a/pkg/alertmanager/multitenant_test.go b/pkg/alertmanager/multitenant_test.go index a81e87d6864..7b500a6bbad 100644 --- a/pkg/alertmanager/multitenant_test.go +++ b/pkg/alertmanager/multitenant_test.go @@ -1969,6 +1969,27 @@ func TestSafeTemplateFilepath(t *testing.T) { template: "../test.tmpl", expectedErr: errors.New(`invalid template name "../test.tmpl": the template filepath is escaping the per-tenant local directory`), }, + "template name starting with /": { + dir: "/tmp", + template: "/file", + expectedErr: nil, + expectedPath: "/tmp/file", + }, + "escaping template name that has prefix of dir (tmp is prefix of tmpfile)": { + dir: "/sub/tmp", + template: "../tmpfile", + expectedErr: errors.New(`invalid template name "../tmpfile": the template filepath is escaping the per-tenant local directory`), + }, + "empty template name": { + dir: "/tmp", + template: "", + expectedErr: errors.New(`invalid template name ""`), + }, + "dot template name": { + dir: "/tmp", + template: ".", + expectedErr: errors.New(`invalid template name "."`), + }, } for testName, testData := range tests { @@ -1980,6 +2001,27 @@ func TestSafeTemplateFilepath(t *testing.T) { } } +func TestIsFilePathInsideDirectory(t *testing.T) { + assert.False(t, isFilePathInsideDirectory("/", "/")) + assert.False(t, isFilePathInsideDirectory("/", "")) + assert.True(t, isFilePathInsideDirectory("/", "/test")) + assert.False(t, isFilePathInsideDirectory("/", "random")) + + assert.False(t, isFilePathInsideDirectory("/tmp", "/")) + assert.False(t, isFilePathInsideDirectory("/tmp", "//")) + assert.False(t, isFilePathInsideDirectory("/tmp", "")) + assert.False(t, isFilePathInsideDirectory("/tmp", "/tmp")) + assert.False(t, isFilePathInsideDirectory("/tmp", "/tmpfile")) + assert.True(t, isFilePathInsideDirectory("/tmp", "/tmp/test")) + assert.True(t, isFilePathInsideDirectory("/tmp", "/tmp/inner/dir/file")) + // Dir ending with / doesn't work. + assert.False(t, isFilePathInsideDirectory("/tmp/", "/tmp/inner/dir/file")) + + assert.True(t, isFilePathInsideDirectory(".", "./test")) + // Dir ending with / doesn't work. + assert.False(t, isFilePathInsideDirectory("./", "./test")) +} + func TestStoreTemplateFile(t *testing.T) { tempDir := t.TempDir() testTemplateDir := filepath.Join(tempDir, templatesDir) From 354a94919f8fc32afba8b3e330fa5bea07e4e880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Fri, 6 May 2022 17:37:52 +0200 Subject: [PATCH 2/4] CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Peter Štibraný --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12d4fac2208..b418d2399d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ * [BUGFIX] Multikv: Fix panic when using using runtime config to set primary KV store used by `multi` KV. #1587 * [BUGFIX] Multikv: Fix watching for runtime config changes in `multi` KV store in ruler and querier. #1665 * [BUGFIX] Memcached: allow to use CNAME DNS records for the memcached backend addresses. #1654 +* [BUGFIX] Alertmanager: prevent more file traversal cases related to template names. #1833 ### Mixin From 532e987f179bc676793527da736b2ab6313a6f56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Mon, 16 May 2022 09:48:16 +0200 Subject: [PATCH 3/4] Simplify check. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Peter Štibraný --- pkg/alertmanager/multitenant.go | 27 +++++------------------- pkg/alertmanager/multitenant_test.go | 31 +++++++++------------------- 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/pkg/alertmanager/multitenant.go b/pkg/alertmanager/multitenant.go index 33df9bc210f..8eb4b10f949 100644 --- a/pkg/alertmanager/multitenant.go +++ b/pkg/alertmanager/multitenant.go @@ -1267,37 +1267,20 @@ func safeTemplateFilepath(dir, templateName string) (string, error) { return "", fmt.Errorf("invalid template name %q", templateName) } + if !strings.HasSuffix(containerDir, string(os.PathSeparator)) { + containerDir = containerDir + string(os.PathSeparator) + } + // Ensure the actual path of the template is within the expected directory. // This check is a counter-measure to make sure the tenant is not trying to // escape its own directory on disk. - if !isFilePathInsideDirectory(containerDir, actualPath) { + if !strings.HasPrefix(actualPath, containerDir) { return "", fmt.Errorf("invalid template name %q: the template filepath is escaping the per-tenant local directory", templateName) } return actualPath, nil } -// Returns true if file described by filePath is inside given directory. (Doesn't check for symbolic links) -// Directory argument should not end with separator, ie. it must be filepath.Clean-ed. -func isFilePathInsideDirectory(absDir string, absFilePath string) bool { - if absFilePath == absDir { - // If filePath refers to dir itself, it's not a "file" path. - return false - } - - d := filepath.Dir(absFilePath) - if d == absDir { - return true - } - - // If path was empty, its Dir is ".". - // If path was "/", its Dir is just "/". - if d == "." || d == "/" { - return false - } - return isFilePathInsideDirectory(absDir, d) -} - // storeTemplateFile stores template file at the given templateFilepath. // Returns true, if file content has changed (new or updated file), false if file with the same name // and content was already stored locally. diff --git a/pkg/alertmanager/multitenant_test.go b/pkg/alertmanager/multitenant_test.go index 7b500a6bbad..2bae3020686 100644 --- a/pkg/alertmanager/multitenant_test.go +++ b/pkg/alertmanager/multitenant_test.go @@ -1990,6 +1990,16 @@ func TestSafeTemplateFilepath(t *testing.T) { template: ".", expectedErr: errors.New(`invalid template name "."`), }, + "root dir": { + dir: "/", + template: "file", + expectedPath: "/file", + }, + "root dir 2": { + dir: "/", + template: "/subdir/file", + expectedPath: "/subdir/file", + }, } for testName, testData := range tests { @@ -2001,27 +2011,6 @@ func TestSafeTemplateFilepath(t *testing.T) { } } -func TestIsFilePathInsideDirectory(t *testing.T) { - assert.False(t, isFilePathInsideDirectory("/", "/")) - assert.False(t, isFilePathInsideDirectory("/", "")) - assert.True(t, isFilePathInsideDirectory("/", "/test")) - assert.False(t, isFilePathInsideDirectory("/", "random")) - - assert.False(t, isFilePathInsideDirectory("/tmp", "/")) - assert.False(t, isFilePathInsideDirectory("/tmp", "//")) - assert.False(t, isFilePathInsideDirectory("/tmp", "")) - assert.False(t, isFilePathInsideDirectory("/tmp", "/tmp")) - assert.False(t, isFilePathInsideDirectory("/tmp", "/tmpfile")) - assert.True(t, isFilePathInsideDirectory("/tmp", "/tmp/test")) - assert.True(t, isFilePathInsideDirectory("/tmp", "/tmp/inner/dir/file")) - // Dir ending with / doesn't work. - assert.False(t, isFilePathInsideDirectory("/tmp/", "/tmp/inner/dir/file")) - - assert.True(t, isFilePathInsideDirectory(".", "./test")) - // Dir ending with / doesn't work. - assert.False(t, isFilePathInsideDirectory("./", "./test")) -} - func TestStoreTemplateFile(t *testing.T) { tempDir := t.TempDir() testTemplateDir := filepath.Join(tempDir, templatesDir) From 2466466b4c68c0ff5d2147c9638a4b770478167a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Mon, 16 May 2022 09:59:23 +0200 Subject: [PATCH 4/4] Fix unit test after changing error message. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Peter Štibraný --- pkg/alertmanager/api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/alertmanager/api_test.go b/pkg/alertmanager/api_test.go index 44a3704e4c1..7b8b8f7eb12 100644 --- a/pkg/alertmanager/api_test.go +++ b/pkg/alertmanager/api_test.go @@ -155,7 +155,7 @@ template_files: "good.tpl": "good-templ" ".": "bad-template" `, - err: fmt.Errorf("error validating Alertmanager config: unable to store template file '.'"), + err: fmt.Errorf("error validating Alertmanager config: invalid template name \".\""), }, { name: "Should return error if the referenced template contains the root /",