Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VAULT-12299 Use file.Stat when checking file permissions #19311

Merged
merged 6 commits into from
Feb 23, 2023

Conversation

miagilepner
Copy link
Contributor

VAULT_ENABLE_FILE_PERMISSIONS_CHECK can be bypassed if the file is modified in between the time its ownership is checked, and the time it is opened. This PR opens the config files first, then uses the file pointers to perform the ownership checks.

There isn't a test for the actual attack, because any test requires multiple OS users, which is difficult to do from Go test code. To reliably test the time of check vs time of use difference, you'd also need to be able to make a sigaction() syscall to perform the ownership change. It doesn't seem worth it to create an enos scenario for a scenario this simple.

@miagilepner miagilepner added this to the 1.13.0 milestone Feb 23, 2023
@miagilepner miagilepner modified the milestones: 1.13.0, 1.13.1 Feb 23, 2023
@miagilepner miagilepner requested review from mpalmi, ncabatoff and a team February 23, 2023 16:02
@miagilepner miagilepner marked this pull request as ready for review February 23, 2023 16:02
@finnigja
Copy link
Contributor

finnigja commented Mar 9, 2023

Note that this was in response to a report by an external researcher to security@hashicorp.com. After triage / investigation, we decided not to issue a CVE for this issue, as code execution on the underlying host is not within Vault’s security model.

An adversary with code execution or write privileges on the host is not something Vault can effectively defend against as a standalone Golang binary. This opt-in feature represents a best effort to reduce the likelihood of misconfigurations, and we decided to make the suggested changes nonetheless as to adhere to file handling best practices.

Thanks for Giuseppe Cocomazzi and the Sysdig team for the report.

@miagilepner miagilepner deleted the miagilepner/VAULT-12299-filechecks branch April 25, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants