client: add extra checks to nomad alloc fs - #28468
Conversation
Juanadelacuesta
changed the title
nomad alloc fs
076b906 to
cacbf62
Compare
cacbf62 to
0cb4066
Compare
0cb4066 to
24c8f12
Compare
Juanadelacuesta
added
backport/ent/1.10.x+ent
tgross
left a comment
tgross
left a comment
There was a problem hiding this comment.
I've left a few comments but otherwise LGTM. We should link to the Jira somewhere in the PR description and/or commit message too.
| }{ | ||
| { | ||
| name: "empty path", | ||
| base: allocDir, |
There was a problem hiding this comment.
It looks like all the test cases have the same base field value, so we can probably drop that and just use allocDir directly. (Although we're also shadowing the allocDir variable in the test case scope by assigning AllocDir to it)
| // Check it does not access the secrets or private directories | ||
| for _, taskDir := range a.TaskDirs { | ||
|
|
||
| if err := escapingfs.ChildEscapesParentDir(taskDir.SecretsDir, requestedPath); err == nil { | ||
| return "", fmt.Errorf("Reading secret file prohibited: %s", path) | ||
| } | ||
|
|
||
| if err := escapingfs.ChildEscapesParentDir(taskDir.PrivateDir, requestedPath); err == nil { | ||
| return "", fmt.Errorf("Reading secret file prohibited: %s", path) | ||
| } | ||
| } |
There was a problem hiding this comment.
The original code does a case-insensitive prefix check. Does ChildEscapesParentDir handle that correctly on macOS, which has the awful case-preserving-but-insensitive file system?
There was a problem hiding this comment.
What do you mean?
There was a problem hiding this comment.
requestedPath is partially attacker-controlled and we resolve it to an absolute file path. So an attacker could give us a path to directory like https://go.dev/play/p/sj2v20RAdeD that the OS would treat as inside the secrets dir for purposes of reading the file. So my question is whether ChildEscapesParentDir protects us even if they've done so. I don't have a case-preserving-but-case-insensitive file system to test with.
| a.mu.RLock() | ||
| defer a.mu.RUnlock() |
There was a problem hiding this comment.
I think this mutex only needs to guard the TaskDirs field, which has the mutex because we can add task dirs as part of task start later on.
There was a problem hiding this comment.
we are reading the TaskDirs when checking for access to secrets and private
There was a problem hiding this comment.
Right, but that's in the same block as the loop over TaskDirs on line 424 so we could move this down if we want. (Not really required but we do a syscall in ChildEscapesParentDir so it might be nice not to hold a mutex over the syscall)
tgross
left a comment
tgross
left a comment
There was a problem hiding this comment.
We discussed offline smoke-testing the behavior of case-insensitive file systems (macOS) but other than that, nothing in my comments is a blocker here so LGTM
…ths and reads into secrets or private directories
Co-authored-by: Tim Gross <tim@0x74696d.com>
Co-authored-by: Tim Gross <tim@0x74696d.com>
4a00794 to
873913c
Compare
4705019 to
ef2a48f
Compare
ef2a48f to
2ddafda
Compare
2ddafda to
7aefa7e
Compare
* func: update alloc dir functions to use os.Root to detect escaping paths and reads into secrets or private directories * fix: update the tests to make the sanitizePath function a method of AllocDir * fix: update the error messages * style: add changelog * Apply suggestion from @tgross Co-authored-by: Tim Gross <tim@0x74696d.com> * Apply suggestion from @tgross Co-authored-by: Tim Gross <tim@0x74696d.com> * fix: always use paths in lower case to avoid escapes on OSs that are case sensitive --------- Co-authored-by: Tim Gross <tim@0x74696d.com>
This branch was successfully deployed
2 participants
This PR addresses 3 bugs:
On mac, to keep compatibility, directories like /vars and /tmp are actually symlinks to
/private/varsand/private/tmp, If the data directory of the client is in one of this redirected directories , when usingnomad alloc fs <alloc id> .the alloc path resolves to/private/var/<data-dir>/..., but the command path does not resolve the symlink, resulting on the requested path being/var/<data-dir>/...which escapes/private/var/<data-dir>/...and the command fails.When a symlink is created from the local task directory into the secrets directory, the security checks are bypassed, and the contents of the secrets directory are visible using
nomad alloc fs.The check for unauthorised access to the the
secretsandprivatedirectories is done by a simple string prefix search and it is not actually preventing any access.This PR updates the escaping path verification methods to use the
ospackage, which uses both lexical check and resolves any possible symlink.Description
Testing & Reproduction steps
Links
Contributor Checklist
changelog entry using the
make clcommand.ensure regressions will be caught.
and job configuration, please update the Nomad product documentation, which is stored in the
web-unified-docsrepo. Refer to theweb-unified-docscontributor guide for docs guidelines.Please also consider whether the change requires notes within the upgrade
guide. If you would like help with the docs, tag the
nomad-docsteam in this PR.and followed the AI usage guide.
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.