Verify write permissions on settings policy folder#1624
Merged
Conversation
On Linux, check that only root (uid 0) owns the /etc/dsc folder and that no group or other write bits are set. On Windows, enumerate the DACL on the ProgramData\dsc folder and verify that only SYSTEM and Administrators have write access. If the folder is not secure, emit a localized error message and exit with code 1. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
When the dsc policy folder has insecure permissions, emit a warning indicating the settings file will not be used and return an empty path so execution continues without the policy settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds dsc_settings_policy_security.tests.ps1 that validates on both Windows and Linux that an insecure dsc policy folder emits a warning about the settings file not being used, and does not cause a non-zero exit code. Also fixes minor issue with the t! macro call passing path to the Windows-specific message. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens loading of the system-wide DSC settings policy (dsc.settings.json) by validating that the policy directory is not writable by unprivileged users, and skipping policy loading (with a warning) when the directory is deemed insecure.
Changes:
- Added platform-specific policy-folder permission checks (Linux mode/uid validation; Windows DACL validation) and warnings when insecure.
- Added i18n strings for the new warnings.
- Added a new Pester test to validate warning emission and non-failing execution when the policy folder is made insecure.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/dsc-lib/src/util.rs | Adds Windows ACL validation + Linux ownership/mode validation before using the policy settings file path. |
| lib/dsc-lib/locales/en-us.toml | Adds localized warning strings describing insecure policy folder requirements. |
| lib/dsc-lib/Cargo.toml | Adds windows dependency for Windows-only ACL inspection. |
| dsc/tests/dsc_settings_policy_security.tests.ps1 | Adds Pester coverage for insecure policy folder detection/warning behavior. |
| Cargo.toml | Enables additional windows crate features needed for ACL inspection APIs. |
| Cargo.lock | Locks the added windows crate dependency version. |
- Fix clippy explicit-auto-deref: &(*ace).SidStart -> &ace.SidStart - Fail closed: return false (insecure) when GetNamedSecurityInfoW, GetAce, or fs::metadata fails instead of assuming secure - Treat NULL DACL as insecure (full access to everyone) - Add GENERIC_WRITE and GENERIC_ALL to the write mask - Skip inherit-only ACEs that don't apply to the folder itself - On Linux, emit warning and return empty on metadata failure - Tests: save/restore original ACL via Get-Acl/Set-Acl on Windows - Tests: capture original mode with stat on Linux instead of hardcoding 755 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
😐 Code Coverage ReportChanged Code Coverage75% (70%+ coverage)
🔵 Full Codebase Coverage81% (good)
|
- Handle non-standard allow ACE types (callback, object ACEs): fail closed for any ACE type we cannot reliably parse the SID from - Skip deny ACE types (1, 6, 10) as they restrict rather than grant - Remove /T from icacls in test to avoid recursively modifying children - Gate Linux test context on \False to avoid macOS failures - Extract verify_linux_permissions helper for testability - Add Rust unit tests for Linux permission checks covering: nonexistent path, non-root owned dir, and missing /etc/dsc scenarios Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The 'Confirm settings override priorities' test creates ProgramData\dsc which on CI inherits broad write access from the parent. The new ACL validation now rejects that folder. Fix by explicitly setting a secure ACL (only SYSTEM, Administrators write; Users read) after creating it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The
dscsettings policy file (dsc.settings.json) is loaded from a system-wide location (/etc/dscon Linux,%ProgramData%\dscon Windows). If an unprivileged user can write to that folder, they could inject arbitrary policy settings -- a potential privilege escalation vector. This PR adds a security check so that an insecure folder is detected and its settings are simply not loaded rather than silently trusted.Approach
get_settings_policy_file_pathnow validates folder permissions before returning the path:/etc/dscfolder is owned by root (uid 0) and has no group or other write bits set.ProgramData\dscfolder and verifies that only SYSTEM and Administrators have write access (FILE_WRITE_DATA, FILE_APPEND_DATA, WRITE_DAC, WRITE_OWNER).If the check fails on either platform, a warning is emitted via
tracing::warn!indicating the settings file will not be used, and an empty path is returned so execution continues without the policy settings. The process does not exit -- this is a graceful degradation.Notable details
Win32_SecurityandWin32_Security_Authorizationfeatures to the workspacewindowscrate and addedwindowsas a cfg(windows) dependency fordsc-lib.verify_windows_aclhelper returns a bool so the caller can decide what to do on failure, keeping the ACL logic self-contained.en-us.tomlunder[util]for the warning messages.dsc_settings_policy_security.tests.ps1) validates the warning is emitted and exit code remains 0 on both Windows (usingicaclsto grant Everyone write) and Linux (usingchmod 777). Both contexts require elevation and restore original state inAfterAll.