Conversation
Updates the requirements on [django](https://github.com/django/django) to permit the latest version. - [Release notes](https://github.com/django/django/releases) - [Commits](django/django@1.0...4.1.4) --- updated-dependencies: - dependency-name: django dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
troubleshooting rsync wrapper
fix ssh key upload bug
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if not paths_contain(settings.LINK_TO_DIRECTORIES,path): | ||
| raise forms.ValidationError('Path not allowed.') | ||
| raise forms.ValidationError('Path not whitelisted. Contact the site admin if you believe this to be an error.') | ||
| if not os.path.isdir(path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
The problem is accessing a filesystem location based on user input without robust normalization and confinement. The safest general approach is to normalize the user-supplied path using os.path.normpath (or os.path.realpath if symlinks are of concern), then check that the normalized path remains within a set of allowed (safe) root directories. This prevents path traversal attacks, even if the input passes regex or whitelist checks.
To implement the fix:
- Before performing
os.path.isdir(path), normalizepathwithos.path.normpathand join it to its root parent (the parent as determined by the whitelisting/validated path). - Verify that the final normalized absolute path is still inside one of the whitelisted directories (from
settings.LINK_TO_DIRECTORIESor similar). - Use the normalized path instead of the raw user input for any filesystem access.
- Minimal changes are required within the method; add normalization code within
clean_link_to_path, and usefull_path(the result) in place ofpathforos.path.isdir().
No dependencies are needed; only Python's standard library (os.path) is used.
| @@ -57,10 +57,14 @@ | ||
| self.fp = FilePath.objects.get(path=parent_path) | ||
| if not self.fp.is_valid(path): | ||
| raise forms.ValidationError('Path not allowed. Must match begin with {} and match one of the expressions: {}'.format(self.fp.path,', '.join(['"'+r+'"' for r in self.fp.regexes]))) | ||
| if not paths_contain(settings.LINK_TO_DIRECTORIES,path): | ||
| # Normalize and join path with parent_path to prevent traversal attacks | ||
| full_path = os.path.normpath(os.path.join(parent_path, os.path.relpath(path, start=parent_path))) | ||
| # Ensure normalized path remains inside any of the allowed directories | ||
| allowed = any(os.path.commonpath([full_path, allowed_dir]) == allowed_dir for allowed_dir in settings.LINK_TO_DIRECTORIES) | ||
| if not allowed: | ||
| raise forms.ValidationError('Path not whitelisted. Contact the site admin if you believe this to be an error.') | ||
| if not os.path.isdir(path): | ||
| raise forms.ValidationError('Path: Directory "%s" does not exist'%path) | ||
| if not os.path.isdir(full_path): | ||
| raise forms.ValidationError('Path: Directory "%s" does not exist' % full_path) | ||
| if self.the_instance: | ||
| if self.the_instance.link_to_path and not path: | ||
| raise forms.ValidationError('It is not possible to change a linked share to a regular share.') |
| raise forms.ValidationError('Path not allowed. Must match begin with {} and match one of the expressions: {}'.format(self.fp.path,', '.join(['"'+r+'"' for r in self.fp.regexes]))) | ||
| if not paths_contain(settings.LINK_TO_DIRECTORIES,path): | ||
| raise forms.ValidationError('Path not whitelisted. Contact the site admin if you believe this to be an error.') | ||
| if not os.path.isdir(path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
The most direct and secure way to fix the problem is to ensure that after any path manipulations based on user input, the code normalizes the computed path (using os.path.normpath or os.path.realpath) and checks that it is strictly inside an approved "root" directory (such as one of the allowed file_paths or paths from settings.LINK_TO_DIRECTORIES). This check should be performed immediately before using the path in a file operation like os.path.isdir.
Specifically:
- After all existing validation in
clean_target, and just before usingos.path.isdir(path), normalize (realpathpreferred to resolve symlinks) both the candidatepathand each allowed root, then check if the normalized candidate path starts with any of the allowed root paths. - If not, raise a
ValidationError. - This will require importing
os.path(already present). - Since
file_pathsmay be many andsettings.LINK_TO_DIRECTORIESa list, check the normalizedpathagainst all permitted roots.
Edits required:
- Insert normalization and containment check after all prior validation but before any use of
os.path.isdir(path), i.e., just before line 201 in theclean_targetmethod ofSymlinkForm(insidebioshareX/forms.py). - No additional imports needed (already imported
os). - No changes to other files necessary.
| @@ -198,7 +198,13 @@ | ||
| raise forms.ValidationError('Path not allowed. Must match begin with {} and match one of the expressions: {}'.format(self.fp.path,', '.join(['"'+r+'"' for r in self.fp.regexes]))) | ||
| if not paths_contain(settings.LINK_TO_DIRECTORIES,path): | ||
| raise forms.ValidationError('Path not whitelisted. Contact the site admin if you believe this to be an error.') | ||
| if not os.path.isdir(path): | ||
| # Normalize and check that path is within allowed roots | ||
| normalized_path = os.path.realpath(path) | ||
| allowed_roots = [os.path.realpath(fp_path) for fp_path in [fp.path for fp in self.file_paths]] | ||
| allowed_roots += [os.path.realpath(link_root) for link_root in settings.LINK_TO_DIRECTORIES] | ||
| if not any(normalized_path.startswith(root + os.sep) or normalized_path == root for root in allowed_roots): | ||
| raise forms.ValidationError('Normalized path not allowed. Path traversal or access outside allowed roots detected.') | ||
| if not os.path.isdir(normalized_path): | ||
| raise forms.ValidationError('Path: Directory "%s" does not exist'%path) | ||
| try: | ||
| check_symlinks_dfs(path)# search_illegal_symlinks(path) #recursively check for illegal paths |
| # Make sure that everything leading up to the link is a regular old directory. If it doesn't exist, one will be made. | ||
| while base_path: | ||
| print('base_path', base_path) | ||
| if os.path.exists(base_path) and (not os.path.isdir(base_path) or os.path.islink(base_path)): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix the uncontrolled path error:
- Normalize and resolve the combined path (using
os.path.normpathand/oros.path.realpath) resulting from the user'snameinput. - Ensure the resolved path is strictly contained within the intended root directory (i.e.,
self.base_directory, which is set as eitherself.share.get_path()oros.path.join(self.share.get_path(), self.subdir)). - If the normalized path escapes the intended root, raise a validation error.
- Apply this check within
clean_name()(lines 212–229), right after calculatingself.link_path, but before operating onbase_pathor potentially creating/changing directories. - This fix does not require any third-party libraries, only Python's standard
os.path.
| @@ -214,6 +214,11 @@ | ||
| else: | ||
| self.base_directory = self.share.get_path() | ||
| self.link_path = os.path.join(self.base_directory, name) | ||
| # Normalize and check containment | ||
| normalized_link_path = os.path.normpath(self.link_path) | ||
| base_directory_norm = os.path.normpath(self.base_directory) | ||
| if not normalized_link_path.startswith(base_directory_norm + os.sep): | ||
| raise forms.ValidationError('The final link path escapes the allowed directory.') | ||
| base_path, link_name = os.path.split(self.link_path) | ||
| # Make sure that everything leading up to the link is a regular old directory. If it doesn't exist, one will be made. | ||
| while base_path: |
| if new_path == base_path: | ||
| break | ||
| base_path = new_path | ||
| if os.path.exists(self.link_path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this, we must ensure that the computed path (self.link_path) is always inside a designated safe root directory -- namely, the share's path (with optional subdir). After joining user-supplied name to the base path, we should normalize or resolve the final path and check that it starts with the expected root directory. This prevents path traversal and ensures users cannot create or target unintended filesystem locations, regardless of user input (even with allowed slashes or weird path segments).
Detailed fix:
- After construction of
self.link_path(inclean_name), normalize the path (usingos.path.normpath). - Check that the normalized path starts with the intended base directory.
- If not, raise a validation error.
- Do not allow the user to escape the base dir by using nested
../or similar constructs.
Changes needed:
- In
clean_name, after computingself.link_path, normalize and enforce its location with respect toself.base_directory. - Imports are sufficient as
os.pathis already imported.
| @@ -214,6 +214,11 @@ | ||
| else: | ||
| self.base_directory = self.share.get_path() | ||
| self.link_path = os.path.join(self.base_directory, name) | ||
| # Normalize and ensure link_path stays within base_directory | ||
| self.link_path = os.path.normpath(self.link_path) | ||
| base_directory_norm = os.path.normpath(self.base_directory) | ||
| if not self.link_path.startswith(base_directory_norm + os.sep) and self.link_path != base_directory_norm: | ||
| raise forms.ValidationError('The link path escapes the intended directory.') | ||
| base_path, link_name = os.path.split(self.link_path) | ||
| # Make sure that everything leading up to the link is a regular old directory. If it doesn't exist, one will be made. | ||
| while base_path: |
| for i in range(len(path_components)): | ||
| subpath = os.sep.join(path_components[:i + 1]) | ||
| full_path = os.path.join(self.base_directory, subpath) | ||
| if not os.path.isdir(full_path) and not os.path.exists(base_path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix the issue, we should ensure that any directory created by the user (in create_subdirectories) will be within the intended base directory (self.base_directory). We do this by normalizing the final directory path (full_path), then confirming it starts with (is contained within) the base directory. If it is not, we raise a validation error or skip creation. This requires adding a check with os.path.normpath() or os.path.abspath(), and ensuring the base path is absolute as well. Only proceed with os.mkdir() if full_path is a subdirectory of self.base_directory. Necessary changes should be made within the create_subdirectories method in bioshareX/forms.py. No external libraries are required, only the built-in Python os and perhaps os.path.
| @@ -236,7 +236,12 @@ | ||
| for i in range(len(path_components)): | ||
| subpath = os.sep.join(path_components[:i + 1]) | ||
| full_path = os.path.join(self.base_directory, subpath) | ||
| if not os.path.isdir(full_path) and not os.path.exists(base_path): | ||
| # Ensure full_path is contained within self.base_directory | ||
| abs_base = os.path.abspath(self.base_directory) | ||
| abs_full = os.path.abspath(full_path) | ||
| if not abs_full.startswith(abs_base + os.sep): | ||
| raise forms.ValidationError(f"Illegal directory creation attempt: '{subpath}' is outside of base directory.") | ||
| if not os.path.isdir(full_path) and not os.path.exists(full_path): | ||
| os.mkdir(full_path) | ||
| self.directories_created.append(subpath) | ||
| def create_link(self): |
| for i in range(len(path_components)): | ||
| subpath = os.sep.join(path_components[:i + 1]) | ||
| full_path = os.path.join(self.base_directory, subpath) | ||
| if not os.path.isdir(full_path) and not os.path.exists(base_path): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
The best fix is to ensure that any directories or symlinks created by SymlinkForm are strictly within the intended base directory (the one corresponding to the constructed share/subdir path). The fix should normalize the final target path after joining user input with the base directory and then validate that the normalized path starts with the base directory path, preventing directory traversal attacks. Specifically:
- In the methods handling user input for path creation (
clean_name,create_subdirectories, andcreate_link), after combining user input with the base directory, always normalize the resulting path. - Check that the normalized path begins with the base directory path. If it does not, raise a validation error and refuse to create the directory or symlink.
- Make minimal additional changes: just normalization and containment checks before any filesystem operation involving a user-derived path.
This requires importing os.path if not already imported, using os.path.normpath and possibly os.path.realpath for extra strictness, and for the check, using os.path.commonpath or a startswith comparison between paths.
| @@ -213,7 +213,11 @@ | ||
| self.base_directory = os.path.join(self.share.get_path(), self.subdir) | ||
| else: | ||
| self.base_directory = self.share.get_path() | ||
| self.link_path = os.path.join(self.base_directory, name) | ||
| self.link_path = os.path.normpath(os.path.join(self.base_directory, name)) | ||
| # Ensure link_path is within base_directory | ||
| base_dir_norm = os.path.normpath(self.base_directory) | ||
| if not self.link_path.startswith(base_dir_norm + os.sep): | ||
| raise forms.ValidationError('Symlink path escapes the share directory.') | ||
| base_path, link_name = os.path.split(self.link_path) | ||
| # Make sure that everything leading up to the link is a regular old directory. If it doesn't exist, one will be made. | ||
| while base_path: | ||
| @@ -235,8 +239,11 @@ | ||
| path_components = normalized_path.split(os.sep) | ||
| for i in range(len(path_components)): | ||
| subpath = os.sep.join(path_components[:i + 1]) | ||
| full_path = os.path.join(self.base_directory, subpath) | ||
| if not os.path.isdir(full_path) and not os.path.exists(base_path): | ||
| full_path = os.path.normpath(os.path.join(self.base_directory, subpath)) | ||
| base_dir_norm = os.path.normpath(self.base_directory) | ||
| if not full_path.startswith(base_dir_norm + os.sep): | ||
| raise forms.ValidationError('Subdirectory path escapes the share directory.') | ||
| if not os.path.isdir(full_path) and not os.path.exists(full_path): | ||
| os.mkdir(full_path) | ||
| self.directories_created.append(subpath) | ||
| def create_link(self): | ||
| @@ -246,6 +253,10 @@ | ||
| # if not os.path.isdir(base_path) and not os.path.exists(base_path): | ||
| # os.makedirs(base_path) | ||
| # self.directories_created = True | ||
| # Validate again that link_path is within allowed base directory | ||
| base_dir_norm = os.path.normpath(self.base_directory) | ||
| if not self.link_path.startswith(base_dir_norm + os.sep): | ||
| raise forms.ValidationError('Symlink path escapes the share directory.') | ||
| os.symlink(self.cleaned_data['target'], self.link_path) | ||
| return self.link_path | ||
|
|
| subpath = os.sep.join(path_components[:i + 1]) | ||
| full_path = os.path.join(self.base_directory, subpath) | ||
| if not os.path.isdir(full_path) and not os.path.exists(base_path): | ||
| os.mkdir(full_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix the issue, ensure that all paths created under create_subdirectories are guaranteed to remain within the expected root directory (self.base_directory). After joining and normalizing the path, add a check so that the normalized absolute path starts with the absolute version of self.base_directory. If the check fails, raise a validation error or skip directory creation.
Specifically:
- In
create_subdirectories, after computingfull_path, normalize and make it absolute, then check it starts with the absolute base directory path followed by a path separator ("os.path.join(self.base_directory, '')" or using "os.path.commonpath"). - If the check fails, raise a
ValidationErrororException. - Only proceed to create the directory if the path is valid.
You only need to edit the code in bioshareX/forms.py within the region implementing SymlinkForm.create_subdirectories.
| @@ -236,8 +236,13 @@ | ||
| for i in range(len(path_components)): | ||
| subpath = os.sep.join(path_components[:i + 1]) | ||
| full_path = os.path.join(self.base_directory, subpath) | ||
| if not os.path.isdir(full_path) and not os.path.exists(base_path): | ||
| os.mkdir(full_path) | ||
| # Ensure the directory to be created is within base_directory | ||
| normalized_full_path = os.path.realpath(full_path) | ||
| base_directory_real = os.path.realpath(self.base_directory) | ||
| if not normalized_full_path.startswith(base_directory_real + os.sep) and normalized_full_path != base_directory_real: | ||
| raise forms.ValidationError('Attempted directory creation outside of base directory.') | ||
| if not os.path.isdir(full_path) and not os.path.exists(full_path): | ||
| os.mkdir(normalized_full_path) | ||
| self.directories_created.append(subpath) | ||
| def create_link(self): | ||
| self.create_subdirectories() |
| # if not os.path.isdir(base_path) and not os.path.exists(base_path): | ||
| # os.makedirs(base_path) | ||
| # self.directories_created = True | ||
| os.symlink(self.cleaned_data['target'], self.link_path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To resolve the vulnerability, the target path should be validated before being used to create a symlink. At minimum, ensure it is not an absolute path and does not contain any path traversal (..) elements. The recommended strategy is to normalize the user-supplied path using os.path.normpath, reject absolute paths, and, if you wish to limit symlinking to contents in a particular directory, verify that the final target path resides inside a trusted base directory.
Implementation steps:
- In the
clean_targetmethod, add checks to normalize the target path, refuse any absolute paths, and refuse path traversals. - Optionally, check that the target exists or meets any further business rules.
- Raise a
forms.ValidationErrorif validation fails. - No need for new imports since only the standard
os.pathfunctions are used.
All edits are within bioshareX/forms.py, specifically inside the SymlinkForm class.
| @@ -178,6 +178,21 @@ | ||
| name = forms.RegexField(regex=r'^[\w\d\ \-_\/]+$',error_messages={'invalid':'Illegal character in folder name'}) | ||
| target = forms.CharField() | ||
| def clean_target(self): | ||
| target = self.cleaned_data['target'] | ||
| # Normalize the target path | ||
| normalized_target = os.path.normpath(target) | ||
| # Prevent absolute paths | ||
| if os.path.isabs(normalized_target): | ||
| raise forms.ValidationError('Illegal absolute symlink target path (must be relative).') | ||
| # Prevent path traversal | ||
| if normalized_target.startswith('..') or '..' in normalized_target.split(os.sep): | ||
| raise forms.ValidationError('Illegal path traversal in symlink target.') | ||
| # Optionally, further check: ensure the target exists, or complies with share restrictions | ||
| # base_directory = self.share.get_path() if not self.subdir else os.path.join(self.share.get_path(), self.subdir) | ||
| # target_path = os.path.normpath(os.path.join(base_directory, normalized_target)) | ||
| # if not target_path.startswith(base_directory): | ||
| # raise forms.ValidationError('Symlink target must be within the allowed directory.') | ||
| return normalized_target | ||
| path = self.cleaned_data['target'] | ||
| file_paths = [fp.path for fp in self.file_paths] | ||
| if not self.user.can_link: | ||
| @@ -246,7 +261,7 @@ | ||
| # if not os.path.isdir(base_path) and not os.path.exists(base_path): | ||
| # os.makedirs(base_path) | ||
| # self.directories_created = True | ||
| os.symlink(self.cleaned_data['target'], self.link_path) | ||
| os.symlink(self.cleaned_data['target'], self.link_path) # self.cleaned_data['target'] is already normalized & validated | ||
| return self.link_path | ||
|
|
||
|
|
2 participants
catch up django 3 branch with master changes