-
Notifications
You must be signed in to change notification settings - Fork 404
security: fix path traversal in BaseTuner directory/project_name/tuner_id #1058
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
|
|
||
|
|
||
| import copy | ||
| import ntpath | ||
| import os | ||
| import traceback | ||
| import warnings | ||
|
|
@@ -115,13 +116,15 @@ def __init__( | |
| # Ops and metadata | ||
| self.directory = directory or "." | ||
| self.project_name = project_name or "untitled_project" | ||
| self._validate_project_path(self.directory, self.project_name) | ||
| self.oracle._set_project_dir(self.directory, self.project_name) | ||
|
|
||
| if overwrite and backend.io.exists(self.project_dir): | ||
| backend.io.rmtree(self.project_dir) | ||
|
|
||
| # To support tuning distribution. | ||
| self.tuner_id = os.environ.get("KERASTUNER_TUNER_ID", "tuner0") | ||
| self._validate_tuner_id(self.tuner_id) | ||
|
|
||
| # Reloading state. | ||
| if not overwrite and backend.io.exists(self._get_tuner_fname()): | ||
|
|
@@ -471,5 +474,43 @@ def get_trial_dir(self, trial_id): | |
| utils.create_directory(dirname) | ||
| return dirname | ||
|
|
||
| @staticmethod | ||
| def _validate_project_path(directory, project_name): | ||
| """Validates that directory and project_name do not contain path traversal. | ||
|
|
||
| Raises: | ||
| ValueError: If path traversal sequences or absolute paths are detected. | ||
| """ | ||
| for name, segment in (("directory", str(directory)), ("project_name", str(project_name))): | ||
| if ".." in segment: | ||
| raise ValueError( | ||
| f"Path traversal is not allowed in {name}. Received: {segment!r}" | ||
| ) | ||
| if ( | ||
| name == "project_name" | ||
| and ( | ||
| os.path.isabs(segment) | ||
| or ntpath.isabs(segment) | ||
| or ntpath.splitdrive(segment)[0] | ||
| ) | ||
| ): | ||
| raise ValueError( | ||
| f"Absolute paths are not allowed in {name}. Received: {segment!r}" | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _validate_tuner_id(tuner_id): | ||
| """Validates that tuner_id does not contain path traversal sequences. | ||
|
|
||
| Raises: | ||
| ValueError: If path traversal sequences or path separators are detected. | ||
| """ | ||
| tuner_id_str = str(tuner_id) | ||
| if ".." in tuner_id_str or "/" in tuner_id_str or "\\" in tuner_id_str: | ||
| raise ValueError( | ||
| f"tuner_id cannot contain path separators or traversal sequences. " | ||
| f"Received: {tuner_id_str!r}" | ||
| ) | ||
|
Comment on lines
+502
to
+513
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Vulnerability: Windows Path Separator Bypass in
|
||
|
|
||
| def _get_tuner_fname(self): | ||
| return os.path.join(str(self.project_dir), f"{str(self.tuner_id)}.json") | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Vulnerability: Absolute Path Bypass in
project_name\n\nWhile checking for".." in segmentprevents directory traversal using relative path segments, it does not preventproject_namefrom being an absolute path (e.g.,/etc/passwdorC:\\Windows).\n\nIn Python,os.path.join(directory, project_name)will discard thedirectoryprefix entirely ifproject_nameis an absolute path. Whenoverwrite=Trueis set, this can lead to arbitrary directory deletion or unauthorized file access outside the intended workspace.\n\nTo resolve this, we should explicitly validate thatproject_nameis neither an absolute path nor a drive-relative path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the current PR head
dbc21de._validate_project_path()now rejects POSIX absolute paths, Windows rooted paths, and Windows drive-prefixed paths viantpath, with regression coverage forC:\Windows,C:Windows, and\Windowsproject names.