Address potential path injection issues in OFRAK server #664
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| os.mkdir(new_path) | ||
| self.projects_dir = new_path | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| if not os.path.exists(validated_path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix this issue, we need to ensure that the validated_path (the directory to be set as self.projects_dir) is restricted to being within an approved safe root directory (e.g., a dedicated directory for projects, such as /srv/ofrak/projects, or a configurable location loaded securely on server startup). We must ensure that the normalized user-supplied path starts with this safe root, or else refuse the operation.
The process is:
- Define a safe base directory (
self.default_projects_root), used as the base for all project paths. - When a new path comes from the user, normalize and convert it to absolute form.
- Check that the resulting path starts with the safe root directory (using
os.path.commonpathorstartswith). - If it does not, reject the request (raise an exception or return an error response).
- If it is valid, proceed as before.
This update should go in the method set_projects_path, ensuring that only safe subdirectories of the base projects directory can be selected.
We may need to:
- Add a definition for
self.default_projects_rootto the class (e.g., set in the constructor). - Import or use standard utilities for path comparison (e.g.,
os.path.commonpath). - Handle the error case gracefully, e.g., returning a
HTTPBadRequestresponse.
| @@ -1321,6 +1321,17 @@ | ||
| body = await request.json() | ||
| new_path = body["path"] | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| # Only allow subdirectories of the default projects root | ||
| try: | ||
| default_projects_root = self.default_projects_root | ||
| except AttributeError: | ||
| # Define a default safe directory if not set | ||
| default_projects_root = os.path.expanduser("~/ofrak_projects") | ||
| self.default_projects_root = default_projects_root | ||
| # Ensure both are absolute and normalized | ||
| safe_root = os.path.normpath(os.path.abspath(default_projects_root)) | ||
| if not os.path.commonpath([validated_path, safe_root]) == safe_root: | ||
| raise HTTPBadRequest(reason="Path must be within the allowed projects directory") | ||
| if not os.path.exists(validated_path): | ||
| os.mkdir(validated_path) | ||
| self.projects_dir = validated_path |
| self.projects_dir = new_path | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| if not os.path.exists(validated_path): | ||
| os.mkdir(validated_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
The best practice is to restrict any user-supplied path so that after normalization, it is still contained within an intended root directory controlled by the application (e.g., some configured projects root such as /my/ofrak/projects). This mitigates path traversal and arbitrary file system modification attacks. To do this, after normalizing and absolutizing the path from user input, we should validate that it starts with an expected base path prefix (for example, the current project's root or an application config constant ALLOWED_PROJECTS_BASE_PATH, which should itself be made absolute).
Specific changes:
- After normalizing and absolutizing
new_path, check that it starts with the intended root projects directory. - If it doesn't, reject the request (raise exception or return error response).
- This check should be made directly after
validated_pathis set.
Additional requirements:
- Determine the variable or constant for the allowed base directory. If not shown or set, you can introduce a default (such as self.projects_dir or a new module-level constant).
- Use the security check pattern from the recommendation:
(ideally, raise an HTTP error).
if not validated_path.startswith(base_path): raise Exception("not allowed")
- Ensure this logic appears immediately before any file system operation using the tainted path.
| @@ -1321,6 +1321,10 @@ | ||
| body = await request.json() | ||
| new_path = body["path"] | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| # Restrict the directory change to a known root directory to prevent path traversal | ||
| base_path = os.path.abspath(self.projects_dir) | ||
| if not validated_path.startswith(base_path): | ||
| raise HTTPBadRequest(text="Invalid path: outside allowed projects directory") | ||
| if not os.path.exists(validated_path): | ||
| os.mkdir(validated_path) | ||
| self.projects_dir = validated_path |
| os.makedirs(self.projects_dir) | ||
| for dir in os.listdir(self.projects_dir): | ||
| validated_projects_dir = os.path.normpath(os.path.abspath(self.projects_dir)) | ||
| if not os.path.exists(validated_projects_dir): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
The best way to fix this risk is to restrict the user-supplied project directory path (new_path) in set_projects_path to remain within a pre-defined "safe root" directory. This way, even if the client supplies a path like /etc or ../../.., the resulting path will never escape the bounds of a specific directory the application controls.
Steps:
- Define a constant (e.g.,
DEFAULT_PROJECTS_ROOT) pointing to the application's main project root directory (e.g.,~/.ofrak/projectsor equivalent in a temp or application directory). - When setting
self.projects_dir, join the user-supplied (new_path) to this root, then normalize, so that attempts to escape the directory are thwarted. - Before updating
self.projects_dir, validate that the normalized resolved absolute path still starts with the safe root. If not, reject the request. - Optionally, adjust
_slurp_projects_from_dirto also revalidateself.projects_dirbefore listing, to mitigate any toxic values being set elsewhere. - You may want to insert a reusable function
validate_safe_directory_path(base, user_path)as referenced elsewhere, or implement an inline check.
Implement these changes in ofrak_core/src/ofrak/gui/server.py, modifying only the affected lines in set_projects_path and validating use in _slurp_projects_from_dir.
| @@ -1320,10 +1320,16 @@ | ||
| async def set_projects_path(self, request: Request) -> Response: | ||
| body = await request.json() | ||
| new_path = body["path"] | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| if not os.path.exists(validated_path): | ||
| os.mkdir(validated_path) | ||
| self.projects_dir = validated_path | ||
| # Restrict project directories to a safe root | ||
| DEFAULT_PROJECTS_ROOT = os.path.expanduser("~/.ofrak/projects") | ||
| abs_default_root = os.path.abspath(DEFAULT_PROJECTS_ROOT) | ||
| requested_path = os.path.normpath(os.path.join(abs_default_root, new_path)) | ||
| # Ensure requested path is within allowed root | ||
| if not requested_path.startswith(abs_default_root): | ||
| raise HTTPBadRequest(reason="Project directory must be within the application root.") | ||
| if not os.path.exists(requested_path): | ||
| os.makedirs(requested_path, exist_ok=True) | ||
| self.projects_dir = requested_path | ||
| self.projects = self._slurp_projects_from_dir() | ||
| return json_response(self.projects_dir) | ||
|
|
| for dir in os.listdir(self.projects_dir): | ||
| validated_projects_dir = os.path.normpath(os.path.abspath(self.projects_dir)) | ||
| if not os.path.exists(validated_projects_dir): | ||
| os.makedirs(validated_projects_dir) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To address this vulnerability, the incoming user-supplied path used for self.projects_dir must be constrained within a safe, application-controlled root directory. The proper fix is to define a constant application root directory (e.g., /srv/ofrak/projects or similar, perhaps via configuration) and ensure that any user-provided path is interpreted strictly as a subdirectory or filename relative to this root. Before accepting and using a path, combine it with the root, normalize it, and confirm that it still begins with the root after normalization.
Specific changes:
- Add a
PROJECTS_ROOTattribute or constant as the root-sandbox directory. - In
set_projects_path, interpret the client-provided "path" only as a relative folder underPROJECTS_ROOT, sanitize and normalize the result, and enforce containment within the root. - In
_slurp_projects_from_dir, always operate withinPROJECTS_ROOT, not based on unchecked data. - Reject any paths that attempt path traversal (e.g., with "..") or would direct the directory outside of the intended root.
- If needed, log or raise an error if validation fails.
This may require defining PROJECTS_ROOT, modifying the assignment to projects_dir, and updating relevant path-construction code.
| @@ -1319,10 +1319,13 @@ | ||
| @exceptions_to_http(SerializedError) | ||
| async def set_projects_path(self, request: Request) -> Response: | ||
| body = await request.json() | ||
| new_path = body["path"] | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| user_path = body["path"] | ||
| try: | ||
| validated_path = get_safe_projects_dir(user_path) | ||
| except ValueError: | ||
| raise HTTPBadRequest(reason="Invalid project path: must be within allowed root") | ||
| if not os.path.exists(validated_path): | ||
| os.mkdir(validated_path) | ||
| os.makedirs(validated_path) | ||
| self.projects_dir = validated_path | ||
| self.projects = self._slurp_projects_from_dir() | ||
| return json_response(self.projects_dir) | ||
| @@ -1388,7 +1390,10 @@ | ||
|
|
||
| def _slurp_projects_from_dir(self) -> Set: | ||
| projects = set() | ||
| validated_projects_dir = os.path.normpath(os.path.abspath(self.projects_dir)) | ||
| try: | ||
| validated_projects_dir = get_safe_projects_dir(self.projects_dir[len(PROJECTS_ROOT):].lstrip(os.sep)) | ||
| except ValueError: | ||
| raise Exception("Current projects_dir is outside of allowed root directory") | ||
| if not os.path.exists(validated_projects_dir): | ||
| os.makedirs(validated_projects_dir) | ||
| for dir in os.listdir(validated_projects_dir): |
| validated_projects_dir = os.path.normpath(os.path.abspath(self.projects_dir)) | ||
| if not os.path.exists(validated_projects_dir): | ||
| os.makedirs(validated_projects_dir) | ||
| for dir in os.listdir(validated_projects_dir): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To safely handle user-provided directory paths, you need to restrict them so that they cannot escape a designated safe root (e.g., a dedicated projects directory). The standard way is to define an application-level root directory (e.g., /var/ofrak/projects/ or some such), then, when accepting a subdirectory name from the user, construct the full path by joining the root with the sanitized user input, and validating that the result is contained within the root directory—i.e., after normalization the joined path must start with the root.
Detailed fix steps:
- Define a constant or attribute to represent the safe root for all projects (
PROJECTS_ROOT_DIR). - In
set_projects_path, validate the proposed new projects path:- If you want to allow flexible (nested) directories, accept only subdirectories under the safe root.
- Use
os.path.normpath/os.path.abspathto normalize the joined path. - Reject inputs that would escape the root (using string prefix checks or
os.path.commonpath).
- Whenever you list or create directories using
self.projects_dir, ensure you operate only within the allowed safe root. - For any directory input, validate and reject absolute paths or attempts to navigate outside the root.
- Optionally, for extra safety, restrict allowed characters for project directory names.
Required methods/imports/definitions:
- Define a safe
PROJECTS_ROOT_DIRpath. - Add helper function, e.g.,
validate_safe_directory_path(root, user_dir)for validation. - Update logic in
set_projects_pathand_slurp_projects_from_dirto use only safe, validated paths.
| @@ -15,6 +15,7 @@ | ||
| import orjson | ||
| import inspect | ||
| import os | ||
| from werkzeug.utils import secure_filename | ||
| import webbrowser | ||
| from collections import defaultdict | ||
| from werkzeug.utils import secure_filename | ||
| @@ -102,7 +103,23 @@ | ||
| T = TypeVar("T") | ||
| LOGGER = logging.getLogger(__name__) | ||
|
|
||
| # Define a safe, fixed root path for projects | ||
| PROJECTS_ROOT_DIR = os.path.abspath(os.path.join(os.getcwd(), "user_projects")) | ||
|
|
||
| def validate_safe_directory_path(root_dir: str, user_dir: str) -> str: | ||
| """ | ||
| Validate that user_dir (project dir name) is a safe subdirectory of root_dir. | ||
| """ | ||
| # Sanitize the directory name and prevent path traversal | ||
| sanitized_dir = secure_filename(user_dir) | ||
| joined_path = os.path.normpath(os.path.join(root_dir, sanitized_dir)) | ||
| root_dir_norm = os.path.normpath(root_dir) | ||
| # Ensure containment (does not allow escaping root via path traversal) | ||
| if not joined_path.startswith(root_dir_norm): | ||
| raise ValueError("Directory path escapes permitted root directory") | ||
| return joined_path | ||
|
|
||
|
|
||
| def exceptions_to_http(error_class: Type[SerializedError]): | ||
| """ | ||
| Decorator for a server function that attempts to do some work, and | ||
| @@ -1319,10 +1335,13 @@ | ||
| @exceptions_to_http(SerializedError) | ||
| async def set_projects_path(self, request: Request) -> Response: | ||
| body = await request.json() | ||
| new_path = body["path"] | ||
| validated_path = os.path.normpath(os.path.abspath(new_path)) | ||
| user_dir = body["path"] # Now treated as a directory name, not a full path | ||
| try: | ||
| validated_path = validate_safe_directory_path(PROJECTS_ROOT_DIR, user_dir) | ||
| except Exception as e: | ||
| raise HTTPBadRequest(text=f"Invalid project directory: {str(e)}") | ||
| if not os.path.exists(validated_path): | ||
| os.mkdir(validated_path) | ||
| os.makedirs(validated_path) | ||
| self.projects_dir = validated_path | ||
| self.projects = self._slurp_projects_from_dir() | ||
| return json_response(self.projects_dir) | ||
| @@ -1388,7 +1406,11 @@ | ||
|
|
||
| def _slurp_projects_from_dir(self) -> Set: | ||
| projects = set() | ||
| # Use validated, normalized projects_dir | ||
| validated_projects_dir = os.path.normpath(os.path.abspath(self.projects_dir)) | ||
| if not validated_projects_dir.startswith(PROJECTS_ROOT_DIR): | ||
| logging.warning("projects_dir is outside the permitted root!") | ||
| return projects # Return empty set | ||
| if not os.path.exists(validated_projects_dir): | ||
| os.makedirs(validated_projects_dir) | ||
| for dir in os.listdir(validated_projects_dir): |
whyitfor
force-pushed
the
maintenance/server_file_sec
branch
from
c402e53 to
56eec9c
Compare
One sentence summary of this PR (This should go in the CHANGELOG!)
Adding path validation and sanitization using werkzeug.secure_filename and path normalization to OFRAK server.
Link to Related Issue(s)
Resolves 6 CodeQL path injection alerts from GitHub Code Scanning:
Please describe the changes in your request.
Implementation:
validate_safe_directory_pathto validate that user-provided paths are within base directorysanitize_repo_nameto extract and sanitize provided Git URLsclone_project_from_git,set_projects_path,_slurp_projects_from_dirto use these methodsTesting:
Anyone you think should look at this, specifically?