Skip to content

Commit

Permalink
alloc fs: use case-insensitive check for reads of secret/private dir
Browse files Browse the repository at this point in the history
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: golang/go#18358
  • Loading branch information
dduzgun-security authored and tgross committed Oct 3, 2024
1 parent 7526c91 commit 4f1cbe3
Show file tree
Hide file tree
Showing 7 changed files with 80 additions and 2 deletions.
3 changes: 3 additions & 0 deletions .changelog/24125.txt
Original file line number Diff line number Diff line change
@@ -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
```
1 change: 1 addition & 0 deletions .github/workflows/test-windows.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
9 changes: 7 additions & 2 deletions client/allocdir/alloc_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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) {
Expand Down
60 changes: 60 additions & 0 deletions client/allocdir/alloc_dir_nonlinux_test.go
Original file line number Diff line number Diff line change
@@ -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,
},
}
)
3 changes: 3 additions & 0 deletions client/allocdir/alloc_dir_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

//go:build !windows
// +build !windows

package allocdir

import (
Expand Down
3 changes: 3 additions & 0 deletions client/allocdir/fs_linux_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

//go:build !windows
// +build !windows

package allocdir

import (
Expand Down
3 changes: 3 additions & 0 deletions client/allocdir/task_dir_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

//go:build !windows
// +build !windows

package allocdir

import (
Expand Down

0 comments on commit 4f1cbe3

Please sign in to comment.