P0: Fix Machine-scope security for ProgramData bin (prevent Users write)#30
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts Windows installer directory ACLs under ProgramData for CloudSQLCTL to restrict Users from modifying binaries while retaining write access only for logs, tightening machine-scope security. Flow diagram for installer setting tightened ACLs on ProgramData directoriesflowchart TD
A[Start installer] --> B[Create CloudSQLCTL root directory]
B --> C[Set ACL: admins-full, system-full, users-readexec]
C --> D[Create logs directory]
D --> E[Set ACL: admins-full, system-full, users-modify]
E --> F[Create bin directory]
F --> G[Set ACL: admins-full, system-full, users-readexec]
G --> H[Create secrets directory]
H --> I[Set ACL: admins-full, system-full]
I --> J[Continue rest of installation]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing the directory ACLs in the installer will only affect newly created directories, so if you need to fix permissions on existing installations you may want to add upgrade logic (e.g., a custom action or script) that explicitly resets ACLs on the existing ProgramData\CloudSQLCTL tree.
- Double‑check whether non‑admin processes need to drop or update binaries under ProgramData\CloudSQLCTL\bin (e.g., auto‑updaters or self‑updating tools); if so, you may need a more targeted mechanism than removing Users modify rights entirely on that directory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing the directory ACLs in the installer will only affect newly created directories, so if you need to fix permissions on existing installations you may want to add upgrade logic (e.g., a custom action or script) that explicitly resets ACLs on the existing ProgramData\CloudSQLCTL tree.
- Double‑check whether non‑admin processes need to drop or update binaries under ProgramData\CloudSQLCTL\bin (e.g., auto‑updaters or self‑updating tools); if so, you may need a more targeted mechanism than removing Users modify rights entirely on that directory.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: admins-full system-full users-readexec | ||
| Name: "{commonappdata}\CloudSQLCTL\logs"; Permissions: admins-full system-full users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-readexec |
There was a problem hiding this comment.
These ACL changes make {commonappdata}\CloudSQLCTL and \bin read/exec-only for standard users, but the installer still sets machine-scope env vars (CLOUDSQLCTL_HOME/CLOUDSQLCTL_PROXY_PATH) so the runtime resolves PATHS to the ProgramData location and then writes config.json, temp, gcloud, and proxy.pid under that directory (see src/core/config.ts, src/core/gcloud-installer.ts, src/core/proxy.ts). With read/exec only on the home/bin, non-admin users will now fail to write config or PID files after a machine install, which is a regression from the prior users-modify ACLs. This will surface anytime a non-admin runs the CLI on a machine-wide install.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request addresses a security vulnerability by tightening directory permissions for the CloudSQLCTL installation in ProgramData. The changes restrict Users group from having modify permissions on the root directory and bin directory (where binaries are stored), while maintaining write access to the logs directory. This prevents unprivileged users from tampering with executable files in a machine-wide installation.
Key changes:
- Restricted root CloudSQLCTL directory from
users-modifytousers-readexec - Restricted bin directory from
users-modifytousers-readexecto protect binaries - Maintained
users-modifyon logs directory to allow non-admin users to write logs - Added explicit
admins-full system-fullpermissions to all directories for clarity
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: admins-full system-full users-readexec | ||
| Name: "{commonappdata}\CloudSQLCTL\logs"; Permissions: admins-full system-full users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-readexec |
There was a problem hiding this comment.
The restrictive permissions on the bin directory (users-readexec) will prevent non-admin users from updating or installing the proxy binary. After the installer runs and sets CLOUDSQLCTL_PROXY_PATH environment variable to point to this machine-scope location, the application will resolve PATHS to use SYSTEM_PATHS. When non-admin users run commands like 'cloudsqlctl install', 'cloudsqlctl update', or 'cloudsqlctl setup', these commands call downloadProxy which writes to PATHS.PROXY_EXE in the bin directory. This write operation will fail with permission denied errors.
Consider one of these approaches:
- Add admin privilege checks before attempting to download to machine-scope paths, and gracefully fall back to user-scope paths for non-admin users
- Make the bin directory writable by a specific group (e.g., a CloudSQLCTL Users group) that can be managed separately
- Document that proxy updates require admin privileges when installed machine-wide
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-readexec | |
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-modify |
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: admins-full system-full users-readexec | ||
| Name: "{commonappdata}\CloudSQLCTL\logs"; Permissions: admins-full system-full users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-readexec |
There was a problem hiding this comment.
The scripts subdirectory is not defined in the [Dirs] section but is used by the application (see src/system/ps1.ts line 33). When generateScripts is called, it will create this directory with inherited permissions from the parent CloudSQLCTL directory. With the new users-readexec permissions on the parent, non-admin users won't be able to write scripts to this location. The selfHeal function calls generateScripts without checking admin status, which could cause failures.
Add an explicit [Dirs] entry for the scripts directory with appropriate permissions, similar to how the logs directory is configured to allow writes.
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-readexec | |
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: admins-full system-full users-readexec | |
| Name: "{commonappdata}\CloudSQLCTL\scripts"; Permissions: admins-full system-full users-modify |
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL\logs"; Permissions: users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL\bin"; Permissions: users-modify | ||
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: admins-full system-full users-readexec |
There was a problem hiding this comment.
The root CloudSQLCTL directory now has users-readexec permissions, which prevents non-admin users from creating new subdirectories or files. While this improves security for the bin and secrets directories, it may break functionality for other operations. For example:
- Creating PID files (proxy.pid is written to HOME, see src/core/proxy.ts line 63)
- Creating config files (config.json in HOME, see src/system/paths.ts line 14)
- Creating temp directories (see src/system/paths.ts line 15)
These paths are resolved to SYSTEM_PATHS.HOME (ProgramData) after installation, and the writes will fail for non-admin users. Consider adding explicit directory entries for directories that need write access, or ensure the application properly handles permission errors and falls back to user-scope paths.
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: admins-full system-full users-readexec | |
| Name: "{commonappdata}\CloudSQLCTL"; Permissions: admins-full system-full users-modify |
1 participant
Closes #13
Summary:
Tests:
Rollback:
Summary by Sourcery
Enhancements: