Skip to content

Potential fix for code scanning alert no. 1: Uncontrolled data used in path expression #2

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Potential fix for code scanning alert no. 1: Uncontrolled data used in path expression #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion netbbsd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5564,7 +5564,17 @@
if hasattr(self, 'logger'):
self.logger.warning(f"Remote file {filename} exceeds max size; skipped")
return
file_path = dir_path / filename
# Validate filename: must not contain path separators or be absolute
if os.path.sep in filename or (os.path.altsep and os.path.altsep in filename) or Path(filename).is_absolute():
if hasattr(self, 'logger'):
self.logger.warning(f"Rejected remote file with invalid filename: {filename}")
return
file_path = (dir_path / filename).resolve()

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.

Copilot Autofix

AI 4 months ago

To fix the problem, we should ensure that the constructed file path is safely contained within the intended uploads directory, regardless of the user-supplied filename. This involves normalizing the path after joining the directory and filename, and then checking that the normalized path is a subpath of the intended directory. The current code does this with .resolve() and a string prefix check, but the initial filename validation could be improved by using a stricter sanitization function, such as werkzeug.utils.secure_filename, or by explicitly rejecting any suspicious characters. Since we can only use well-known libraries and not assume the presence of werkzeug, we should further sanitize the filename by allowing only safe characters (e.g., alphanumerics, underscores, hyphens, and dots), and then perform the normalization and containment check as before. The changes should be made in the region around line 5567–5577 in netbbsd.py.

Required changes:

  • Add a stricter filename sanitization step before constructing the path.
  • Optionally, define a helper function to sanitize filenames.
  • Ensure the path normalization and containment check are robust.
Suggested changeset 1
netbbsd.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/netbbsd.py b/netbbsd.py
--- a/netbbsd.py
+++ b/netbbsd.py
@@ -5566,4 +5566,14 @@
                     return
-                # Validate filename: must not contain path separators or be absolute
-                if os.path.sep in filename or (os.path.altsep and os.path.altsep in filename) or Path(filename).is_absolute():
+                # Validate filename: must be a safe basename (no path separators, no absolute, only safe chars)
+                import re
+                def sanitize_filename(name: str) -> str:
+                    # Only allow alphanumerics, underscores, hyphens, and dots
+                    return re.sub(r'[^A-Za-z0-9._-]', '_', name)
+                safe_filename = sanitize_filename(filename)
+                if (
+                    os.path.sep in safe_filename or
+                    (os.path.altsep and os.path.altsep in safe_filename) or
+                    Path(safe_filename).is_absolute() or
+                    safe_filename in ('', '.', '..')
+                ):
                     if hasattr(self, 'logger'):
@@ -5571,3 +5581,3 @@
                     return
-                file_path = (dir_path / filename).resolve()
+                file_path = (dir_path / safe_filename).resolve()
                 # Ensure file_path is within dir_path
EOF
@@ -5566,4 +5566,14 @@
return
# Validate filename: must not contain path separators or be absolute
if os.path.sep in filename or (os.path.altsep and os.path.altsep in filename) or Path(filename).is_absolute():
# Validate filename: must be a safe basename (no path separators, no absolute, only safe chars)
import re
def sanitize_filename(name: str) -> str:
# Only allow alphanumerics, underscores, hyphens, and dots
return re.sub(r'[^A-Za-z0-9._-]', '_', name)
safe_filename = sanitize_filename(filename)
if (
os.path.sep in safe_filename or
(os.path.altsep and os.path.altsep in safe_filename) or
Path(safe_filename).is_absolute() or
safe_filename in ('', '.', '..')
):
if hasattr(self, 'logger'):
@@ -5571,3 +5581,3 @@
return
file_path = (dir_path / filename).resolve()
file_path = (dir_path / safe_filename).resolve()
# Ensure file_path is within dir_path
Copilot is powered by AI and may make mistakes. Always verify output.
# Ensure file_path is within dir_path
if not str(file_path).startswith(str(dir_path.resolve())):
if hasattr(self, 'logger'):
self.logger.warning(f"Rejected remote file with path traversal attempt: {filename}")
return
try:
with open(file_path, 'wb') as fh:
fh.write(data_bytes)
Expand Down