🔒 Restrict System storage path permissions to 0700#278
Conversation
The `System::createStoragePath()` was using `0755` to create user-specific configuration directories via `mkdir`. This is changed to `0700` so only the owner has access. Co-authored-by: segin <480709+segin@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR tightens POSIX filesystem permissions for the application’s user storage/config directory created by System::createStoragePath(), addressing potential exposure of sensitive user data in multi-user environments.
Changes:
- Change POSIX
mkdirpermissions for the storage directory from0755to0700.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // mkdir returns 0 on success. | ||
| return mkdir(path.toCString(true), 0755) == 0 || (errno == EEXIST); | ||
| return mkdir(path.toCString(true), 0700) == 0 || (errno == EEXIST); | ||
| #endif |
There was a problem hiding this comment.
On POSIX, treating errno == EEXIST as success can mask cases where the path already exists but is not a directory (mkdir returns EEXIST for any existing path). Also, if the directory already exists with overly permissive mode (e.g., 0755 from previous versions), this code returns true without tightening permissions, so the reported vulnerability remains for existing installs. Consider stat() to confirm S_ISDIR, and on success/exists ensure permissions are restricted (e.g., chmod(path, 0700) or equivalent) while handling errors explicitly.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the directory creation permissions in src/system.cpp from 0755 to 0700 to enhance security. The review feedback correctly identifies that this change only affects new directories and suggests using chmod to ensure consistent permissions for existing directories, while also pointing out a potential logic error if the path already exists as a file.
| #else | ||
| // mkdir returns 0 on success. | ||
| return mkdir(path.toCString(true), 0755) == 0 || (errno == EEXIST); | ||
| return mkdir(path.toCString(true), 0700) == 0 || (errno == EEXIST); |
There was a problem hiding this comment.
While restricting permissions to 0700 is a significant security improvement, this change only affects newly created directories. If the storage directory already exists (e.g., from a previous version of the application), its permissions will remain unchanged (likely 0755), leaving the vulnerability unaddressed for existing users. Additionally, if the path exists but is a regular file, mkdir will fail with EEXIST and the function will incorrectly return true. Consider explicitly setting the permissions using chmod if the directory already exists to ensure the security fix is applied consistently.
return mkdir(path.toCString(true), 0700) == 0 || (errno == EEXIST && chmod(path.toCString(true), 0700) == 0);
🎯 What: The vulnerability fixed is that
⚠️ Risk: By setting permissions to
System::createStoragePath()was invokingmkdirwith0755permissions for user configuration and storage directories.0755, other users on the system could potentially read the contents of the configuration directories, putting sensitive user data or configuration at risk of unauthorized access.🛡️ Solution: The permissions argument passed to
mkdirhas been restricted to0700, ensuring the directory is only readable, writable, and executable by the owner.PR created automatically by Jules for task 15930402972452581726 started by @segin