From 4f1cbe3043f07a66d367ba37ca7e2e996f445c20 Mon Sep 17 00:00:00 2001 From: dduzgun-security Date: Thu, 3 Oct 2024 13:35:04 -0400 Subject: [PATCH] alloc fs: use case-insensitive check for reads of secret/private dir When using the Client FS APIs, we check to ensure that reads don't traverse into the allocation's secret dir and private dir. But this check can be bypassed on case-insensitive file systems (ex. Windows, macOS, and Linux with obscure ext4 options enabled). This allows a user with `read-fs` permissions but not `alloc-exec` permissions to read from the secrets dir. This changeset updates the check so that it's case-insensitive. This risks false positives for escape (see linked Go issue), but only if a task without filesystem isolation deliberately writes into the task working directory to do so, which is a fail-safe failure mode. Ref: https://github.com/golang/go/issues/18358 --- .changelog/24125.txt | 3 ++ .github/workflows/test-windows.yml | 1 + client/allocdir/alloc_dir.go | 9 +++- client/allocdir/alloc_dir_nonlinux_test.go | 60 ++++++++++++++++++++++ client/allocdir/alloc_dir_test.go | 3 ++ client/allocdir/fs_linux_test.go | 3 ++ client/allocdir/task_dir_test.go | 3 ++ 7 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 .changelog/24125.txt create mode 100644 client/allocdir/alloc_dir_nonlinux_test.go diff --git a/.changelog/24125.txt b/.changelog/24125.txt new file mode 100644 index 000000000000..a61ef2ea4f92 --- /dev/null +++ b/.changelog/24125.txt @@ -0,0 +1,3 @@ +```release-note:security +security: Fixed a bug in client FS API where the check to prevent reads from the secrets dir could be bypassed on case-insensitive file systems +``` diff --git a/.github/workflows/test-windows.yml b/.github/workflows/test-windows.yml index d1b00f780635..0d1eb43fc5df 100644 --- a/.github/workflows/test-windows.yml +++ b/.github/workflows/test-windows.yml @@ -90,6 +90,7 @@ jobs: github.com/hashicorp/nomad/client/lib/fifo \ github.com/hashicorp/nomad/client/logmon \ github.com/hashicorp/nomad/client/allocrunner/taskrunner/template \ + github.com/hashicorp/nomad/client/allocdir \ github.com/hashicorp/nomad/plugins/base - uses: actions/upload-artifact@50769540e7f4bd5e21e526ee35c689e35e0d6874 # v4.4.0 with: diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index 724e59da4542..65248460871a 100644 --- a/client/allocdir/alloc_dir.go +++ b/client/allocdir/alloc_dir.go @@ -471,11 +471,11 @@ func (d *AllocDir) ReadAt(path string, offset int64) (io.ReadCloser, error) { // Check if it is trying to read into a secret directory d.mu.RLock() for _, dir := range d.TaskDirs { - if filepath.HasPrefix(p, dir.SecretsDir) { + if caseInsensitiveHasPrefix(p, dir.SecretsDir) { d.mu.RUnlock() return nil, fmt.Errorf("Reading secret file prohibited: %s", path) } - if filepath.HasPrefix(p, dir.PrivateDir) { + if caseInsensitiveHasPrefix(p, dir.PrivateDir) { d.mu.RUnlock() return nil, fmt.Errorf("Reading private file prohibited: %s", path) } @@ -492,6 +492,11 @@ func (d *AllocDir) ReadAt(path string, offset int64) (io.ReadCloser, error) { return f, nil } +// CaseInsensitiveHasPrefix checks if the prefix is a case-insensitive prefix. +func caseInsensitiveHasPrefix(s, prefix string) bool { + return strings.HasPrefix(strings.ToLower(s), strings.ToLower(prefix)) +} + // BlockUntilExists blocks until the passed file relative the allocation // directory exists. The block can be cancelled with the passed context. func (d *AllocDir) BlockUntilExists(ctx context.Context, path string) (chan error, error) { diff --git a/client/allocdir/alloc_dir_nonlinux_test.go b/client/allocdir/alloc_dir_nonlinux_test.go new file mode 100644 index 000000000000..b1882e553294 --- /dev/null +++ b/client/allocdir/alloc_dir_nonlinux_test.go @@ -0,0 +1,60 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +//go:build !linux +// +build !linux + +package allocdir + +import ( + "os" + "path/filepath" + "testing" + + "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/testlog" + "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/plugins/drivers/fsisolation" + "github.com/shoenig/test/must" +) + +func TestAllocDir_ReadAt_CaseInsensitiveSecretDir(t *testing.T) { + ci.Parallel(t) + + // On macOS, os.TempDir returns a symlinked path under /var which + // is outside of the directories shared into the VM used for Docker. + // Expand the symlink to get the real path in /private, which is ok. + tmp, err := filepath.EvalSymlinks(t.TempDir()) + must.NoError(t, err) + + d := NewAllocDir(testlog.HCLogger(t), tmp, tmp, "test") + must.NoError(t, d.Build()) + defer func() { _ = d.Destroy() }() + + td := d.NewTaskDir(t1Windows) + must.NoError(t, td.Build(fsisolation.None, nil, "nobody")) + + target := filepath.Join(t1Windows.Name, TaskSecrets, "test_file") + + full := filepath.Join(d.AllocDir, target) + must.NoError(t, os.WriteFile(full, []byte("hi"), 0o600)) + + targetCaseInsensitive := filepath.Join(t1Windows.Name, "sEcReTs", "test_file") + + _, err = d.ReadAt(targetCaseInsensitive, 0) + must.EqError(t, err, "Reading secret file prohibited: "+targetCaseInsensitive) +} + +var ( + t1Windows = &structs.Task{ + Name: "web", + Driver: "exec", + Config: map[string]interface{}{ + "command": "/bin/date", + "args": "+%s", + }, + Resources: &structs.Resources{ + DiskMB: 1, + }, + } +) diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index 22f2e04d3249..69f9e699598a 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/client/allocdir/alloc_dir_test.go @@ -1,6 +1,9 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: BUSL-1.1 +//go:build !windows +// +build !windows + package allocdir import ( diff --git a/client/allocdir/fs_linux_test.go b/client/allocdir/fs_linux_test.go index 073849d003d4..c6e6387c9334 100644 --- a/client/allocdir/fs_linux_test.go +++ b/client/allocdir/fs_linux_test.go @@ -1,6 +1,9 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: BUSL-1.1 +//go:build !windows +// +build !windows + package allocdir import ( diff --git a/client/allocdir/task_dir_test.go b/client/allocdir/task_dir_test.go index 592aab2e5a95..f965b70d67ee 100644 --- a/client/allocdir/task_dir_test.go +++ b/client/allocdir/task_dir_test.go @@ -1,6 +1,9 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: BUSL-1.1 +//go:build !windows +// +build !windows + package allocdir import (