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 f7d8f8d08b84..872d59e988b5 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 (