Skip to content
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

TP2000-1269 Resolve uncontrolled data used in path expression alert #1257

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dalecannon
Copy link
Contributor

@dalecannon dalecannon commented Jul 2, 2024

TP2000-1269 Resolve uncontrolled data used in path expression alert

Why

The filenames of TARIC files uploaded in the importer form could use some validation and sanitisation.

What

  • Adds two new functions, validate_filename and validate_filepath, that can be used to guard against malicious filenames and path traversal attacks
  • Validates the filename of uploaded TARIC files
  • Sanitises the name used to create an ImportBatch based on the uploaded TARIC file

@dalecannon dalecannon force-pushed the TP2000-1269--uncontrolled-data-path-expression-alert branch 12 times, most recently from 92eedc4 to 8ae72b4 Compare July 4, 2024 12:00
@dalecannon dalecannon force-pushed the TP2000-1269--uncontrolled-data-path-expression-alert branch from 8ae72b4 to adbe367 Compare July 10, 2024 13:36
@dalecannon dalecannon force-pushed the TP2000-1269--uncontrolled-data-path-expression-alert branch from adbe367 to 728afc5 Compare July 10, 2024 15:32
"File name must only include alphanumeric characters and special characters such as spaces, hyphens and underscores.",
)
validate_filename(uploaded_taric_file.name)
validate_filepath(uploaded_taric_file)

try:
xml_file = parse_xml(uploaded_taric_file)
Copy link
Contributor Author

@dalecannon dalecannon Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the current validation (and after various other validation attempts), one CodeQL alert warning that uploaded_taric_file is an uncontrolled user input still remains.

It may just be a false positive, where the scanner overlooks how we're having to access the name attribute on the InMemoryUploadedFile object to validate its filename, and not a str or other such object that can be directly acted upon to satisfy the scanner's expectations.

@dalecannon dalecannon marked this pull request as ready for review July 10, 2024 15:59
@dalecannon dalecannon requested a review from a team as a code owner July 10, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant