diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 90b31d1..8ce2d73 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -64,7 +64,7 @@ jobs: OS: "${{ matrix.os }}" steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: "Log in" shell: bash @@ -96,7 +96,7 @@ jobs: OS: "linux" steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Log in run: infra/login.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2ceed60..b82f49e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -46,7 +46,7 @@ jobs: timeout-minutes: 1 steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - run: infra/release-check.sh build-artifacts: @@ -76,7 +76,7 @@ jobs: OS: ${{ matrix.os }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - name: Log in run: infra/login.sh @@ -87,7 +87,7 @@ jobs: run: infra/release-build.sh shell: bash - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 if: success() with: name: ${{ matrix.os }} @@ -107,8 +107,8 @@ jobs: - build-artifacts steps: - - uses: actions/checkout@v4 - - uses: actions/download-artifact@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 with: path: _out merge-multiple: true diff --git a/internal/oauthtoken/BUILD b/internal/oauthtoken/BUILD index da10a7f..f943226 100644 --- a/internal/oauthtoken/BUILD +++ b/internal/oauthtoken/BUILD @@ -6,6 +6,8 @@ go_library( "debug.go", "fake.go", "file.go", + "file_other.go", + "file_unix.go", "keyring.go", "load_storer.go", ], @@ -21,6 +23,7 @@ go_library( go_test( name = "oauthtoken_test", srcs = [ + "file_test.go", "keyring_test.go", "load_storer_test.go", ], diff --git a/internal/oauthtoken/file.go b/internal/oauthtoken/file.go index d1fb723..50a14e2 100644 --- a/internal/oauthtoken/file.go +++ b/internal/oauthtoken/file.go @@ -21,6 +21,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "golang.org/x/oauth2" ) @@ -36,15 +37,58 @@ type FileStore struct { var _ LoadStorer = (*FileStore)(nil) func NewFileTokenStore(dir string) (*FileStore, error) { - if err := os.MkdirAll(filepath.Dir(dir), 0777); err != nil { + // Create parent directories with restrictive permissions (0700) rather + // than 0777. Go applies these bits before the process umask, so a + // permissive umask with 0777 could leave parents writable by other local + // users, enabling path-replacement attacks on a shared host. + if err := os.MkdirAll(filepath.Dir(dir), 0700); err != nil { return nil, fmt.Errorf("creating parent of token directory: %w", err) } - if err := os.Mkdir(dir, 0700); err != nil && !errors.Is(err, fs.ErrExist) { - return nil, fmt.Errorf("creating token directory: %w", err) + if err := os.Mkdir(dir, 0700); err != nil { + if !errors.Is(err, fs.ErrExist) { + return nil, fmt.Errorf("creating token directory: %w", err) + } + // The path already exists. Do not blindly trust it: another local + // user could have pre-created a symbolic link or other object at this + // path to redirect or observe subsequent token-file operations. + // Verify it is a real directory owned by us and not accessible to + // group or other users. + if err := checkTokenDir(dir); err != nil { + return nil, fmt.Errorf("validating existing token directory: %w", err) + } } return &FileStore{dir: dir}, nil } +// checkTokenDir verifies that the token directory at dir is a regular +// directory (not a symbolic link or other filesystem object), is owned by the +// current user, and is not writable by group or other users. Group/other +// write access is what would allow another local user to replace the +// directory or its contents; token files themselves are written with mode +// 0600. +func checkTokenDir(dir string) error { + // Use Lstat so that a symbolic link is reported as a symlink rather than + // followed to its target. + info, err := os.Lstat(dir) + if err != nil { + return err + } + mode := info.Mode() + if mode&fs.ModeSymlink != 0 { + return fmt.Errorf("%q is a symbolic link, not a directory", dir) + } + if !mode.IsDir() { + return fmt.Errorf("%q is not a directory", dir) + } + if perm := mode.Perm(); perm&0022 != 0 { + return fmt.Errorf("%q is writable by group or other users (mode %o)", dir, perm) + } + if err := checkOwnedByCurrentUser(dir, info); err != nil { + return err + } + return nil +} + func (f *FileStore) Load(cluster string) (_ *oauth2.Token, err error) { defer func() { if err != nil { @@ -52,7 +96,17 @@ func (f *FileStore) Load(cluster string) (_ *oauth2.Token, err error) { } }() - data, err := os.ReadFile(f.tokenFilePath(cluster)) + name, err := tokenFileName(cluster) + if err != nil { + return nil, err + } + root, err := os.OpenRoot(f.dir) + if err != nil { + return nil, err + } + defer root.Close() + + data, err := root.ReadFile(name) if err != nil { return nil, err } @@ -70,15 +124,35 @@ func (f *FileStore) Store(cluster string, token *oauth2.Token) (err error) { } }() + name, err := tokenFileName(cluster) + if err != nil { + return err + } data, err := json.Marshal(token) if err != nil { return err } - return os.WriteFile(f.tokenFilePath(cluster), data, 0600) + root, err := os.OpenRoot(f.dir) + if err != nil { + return err + } + defer root.Close() + + return root.WriteFile(name, data, 0600) } func (f *FileStore) Delete(cluster string) error { - if err := os.Remove(f.tokenFilePath(cluster)); errors.Is(err, fs.ErrNotExist) { + name, err := tokenFileName(cluster) + if err != nil { + return err + } + root, err := os.OpenRoot(f.dir) + if err != nil { + return err + } + defer root.Close() + + if err := root.Remove(name); errors.Is(err, fs.ErrNotExist) { return err } else if err != nil { return fmt.Errorf("removing token for cluster %q: %w", cluster, err) @@ -86,6 +160,27 @@ func (f *FileStore) Delete(cluster string) error { return nil } -func (f *FileStore) tokenFilePath(cluster string) string { - return filepath.Join(f.dir, cluster) +// tokenFileName validates that a cluster key can be used directly as the name +// of a token file within the store directory, and returns that name. +// +// Token files have always been named after the (bare host) cluster key, one +// file per cluster directly inside the store directory. A legitimate host — +// with or without a port — is a single path element and passes unchanged, so +// existing token files remain readable. Any key that would resolve to a +// different directory (because it contains a path separator, a ".." segment, +// is absolute, or is otherwise not a single "clean" path element) is rejected. +// +// This is the first line of defense; os.Root additionally enforces containment +// at the operating-system level for every read, write, and delete, so a key +// that slipped past this check still cannot escape the store directory. +func tokenFileName(cluster string) (string, error) { + if cluster == "" || + cluster == "." || + cluster == ".." || + strings.ContainsRune(cluster, '/') || + strings.ContainsRune(cluster, filepath.Separator) || + filepath.Clean(cluster) != cluster { + return "", fmt.Errorf("invalid cluster identifier %q: must be a single path element", cluster) + } + return cluster, nil } diff --git a/internal/oauthtoken/file_other.go b/internal/oauthtoken/file_other.go new file mode 100644 index 0000000..44e48bf --- /dev/null +++ b/internal/oauthtoken/file_other.go @@ -0,0 +1,26 @@ +// Copyright 2024 EngFlow Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build !unix + +package oauthtoken + +import "io/fs" + +// checkOwnedByCurrentUser is a no-op on platforms where POSIX ownership +// semantics do not apply (e.g. Windows). The directory-type and permission +// checks in checkTokenDir still apply. +func checkOwnedByCurrentUser(dir string, info fs.FileInfo) error { + return nil +} diff --git a/internal/oauthtoken/file_test.go b/internal/oauthtoken/file_test.go new file mode 100644 index 0000000..8248408 --- /dev/null +++ b/internal/oauthtoken/file_test.go @@ -0,0 +1,153 @@ +// Copyright 2024 EngFlow Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package oauthtoken + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" +) + +func TestNewFileTokenStore(t *testing.T) { + t.Run("creates directory tree", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "a", "b", "tokens") + _, err := NewFileTokenStore(dir) + require.NoError(t, err) + + info, err := os.Stat(dir) + require.NoError(t, err) + assert.True(t, info.IsDir()) + if runtime.GOOS != "windows" { + assert.Equal(t, os.FileMode(0700), info.Mode().Perm()) + } + }) + + t.Run("accepts existing well-formed directory", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "tokens") + require.NoError(t, os.Mkdir(dir, 0700)) + _, err := NewFileTokenStore(dir) + require.NoError(t, err) + }) + + t.Run("rejects symlink at token directory path", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is restricted on Windows") + } + base := t.TempDir() + target := filepath.Join(base, "target") + require.NoError(t, os.Mkdir(target, 0700)) + link := filepath.Join(base, "tokens") + require.NoError(t, os.Symlink(target, link)) + + _, err := NewFileTokenStore(link) + require.Error(t, err) + assert.Contains(t, err.Error(), "symbolic link") + }) + + t.Run("rejects non-directory at token directory path", func(t *testing.T) { + base := t.TempDir() + path := filepath.Join(base, "tokens") + require.NoError(t, os.WriteFile(path, []byte("not a dir"), 0600)) + + _, err := NewFileTokenStore(path) + require.Error(t, err) + }) + + t.Run("rejects group- or other-writable directory", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits do not apply on Windows") + } + dir := filepath.Join(t.TempDir(), "tokens") + require.NoError(t, os.Mkdir(dir, 0777)) + // os.Mkdir applies the umask, so force the loose bits explicitly. + require.NoError(t, os.Chmod(dir, 0777)) + + _, err := NewFileTokenStore(dir) + require.Error(t, err) + assert.Contains(t, err.Error(), "writable") + }) +} + +// traversalKeys are cluster identifiers that must never be allowed to resolve +// to a path outside the token store directory. These mirror the values a +// malicious cluster URL or import blob can produce (see sanitizedURL, which +// moves a bare relative value into the URL host). +var traversalKeys = []string{ + "../../../../../escaped/token", + "..", + ".", + "", + "sub/token", + "a/../../b", + "/etc/passwd", + "/absolute/path", +} + +func TestFileStoreRejectsPathTraversal(t *testing.T) { + // A sentinel file living next to the store directory stands in for any + // user-accessible file an attacker might try to read, overwrite, or delete. + base := t.TempDir() + dir := filepath.Join(base, "tokens") + store, err := NewFileTokenStore(dir) + require.NoError(t, err) + + sentinelRel := filepath.Join("..", "sentinel") + sentinelPath := filepath.Join(dir, sentinelRel) + const sentinelContent = "important-user-data" + + for _, key := range traversalKeys { + t.Run(key, func(t *testing.T) { + require.NoError(t, os.WriteFile(sentinelPath, []byte(sentinelContent), 0600)) + + _, loadErr := store.Load(key) + require.Error(t, loadErr, "Load must reject traversal key") + + storeErr := store.Store(key, &oauth2.Token{AccessToken: "attacker-controlled"}) + require.Error(t, storeErr, "Store must reject traversal key") + + deleteErr := store.Delete(key) + require.Error(t, deleteErr, "Delete must reject traversal key") + + // The sentinel file must be untouched by any of the operations. + got, err := os.ReadFile(sentinelPath) + require.NoError(t, err, "sentinel file must still exist") + assert.Equal(t, sentinelContent, string(got), "sentinel file must be unmodified") + }) + } +} + +func TestTokenFileName(t *testing.T) { + // Legitimate cluster hosts (with or without a port) are single path + // elements and must pass through unchanged for backwards compatibility. + for _, valid := range []string{ + "nakatomiplaza.cluster.engflow.com", + "cluster.example.com", + "cluster.example.com:8080", + } { + name, err := tokenFileName(valid) + require.NoErrorf(t, err, "expected %q to be accepted", valid) + assert.Equal(t, valid, name) + } + + for _, invalid := range traversalKeys { + _, err := tokenFileName(invalid) + assert.Errorf(t, err, "expected %q to be rejected", invalid) + } +} diff --git a/internal/oauthtoken/file_unix.go b/internal/oauthtoken/file_unix.go new file mode 100644 index 0000000..2c1d591 --- /dev/null +++ b/internal/oauthtoken/file_unix.go @@ -0,0 +1,37 @@ +// Copyright 2024 EngFlow Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build unix + +package oauthtoken + +import ( + "fmt" + "io/fs" + "os" + "syscall" +) + +// checkOwnedByCurrentUser verifies that the filesystem object described by +// info at path dir is owned by the user ID of the current process. +func checkOwnedByCurrentUser(dir string, info fs.FileInfo) error { + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("cannot determine owner of %q", dir) + } + if uid := os.Getuid(); uint64(stat.Uid) != uint64(uid) { + return fmt.Errorf("%q is owned by uid %d, not the current user (uid %d)", dir, stat.Uid, uid) + } + return nil +}