Skip to content

Commit

Permalink
backport of commit b7595c6
Browse files Browse the repository at this point in the history
  • Loading branch information
tgross authored Oct 3, 2024
1 parent 0d73064 commit 7cc5e35
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 7cc5e35

Please sign in to comment.