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

Commercial tests #18

Open
wants to merge 12 commits into
base: master_upstream
Choose a base branch
from
Open

Commercial tests #18

wants to merge 12 commits into from

Conversation

Kidev
Copy link
Owner

@Kidev Kidev commented Jan 9, 2025

No description provided.

aqt/installer.py Outdated
try:
os.chdir(working_dir)
self.logger.debug(f"Executing installer with command: {' '.join(safe_args)}")
subprocess.run(safe_args, shell=False, check=True, cwd=working_dir)

Choose a reason for hiding this comment

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

Codacy found a critical Security issue: Detected subprocess function 'run' without a static string.

The issue identified by the Semgrep linter pertains to the way the subprocess.run function is being called. Specifically, it highlights that the command being executed is constructed from a list of arguments (safe_args), which may not be static strings. This could potentially lead to command injection vulnerabilities if any of the arguments come from untrusted sources.

To address this issue, we can ensure that the command is executed in a safer manner by explicitly passing the command and its arguments as a sequence, which is already being done in the current code. However, the warning suggests that we should ensure that the command itself is a static string.

The current implementation is already quite secure due to the use of shlex.quote for argument quoting, but we can improve the clarity of the command execution by explicitly defining the command as a list of strings rather than relying on a variable that could potentially be modified elsewhere.

Here's the suggested change, which explicitly separates the command from its arguments:

Suggested change
subprocess.run(safe_args, shell=False, check=True, cwd=working_dir)
subprocess.run([str(installer_path.absolute())] + cmd[1:], shell=False, check=True, cwd=working_dir)

This modification directly constructs the command list, ensuring clarity and maintaining security best practices.


This comment was generated by an experimental AI tool.

aqt/installer.py Outdated
f.write(chunk)

if self.os_name != "windows":
os.chmod(target_path, 0o700) # Read/execute only for owner

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Security issue: These permissions 0o700 are widely permissive and grant access to more people than may be necessary.

The issue identified by the Semgrep linter is that setting the file permissions to 0o700 grants read, write, and execute permissions to the owner, while denying access to everyone else. This may be considered overly permissive in some security contexts, as it allows the owner of the file to execute it, which might not be necessary depending on the use case. A more restrictive permission setting would limit access appropriately.

To address this, we can change the permissions to 0o500, which grants read and execute permissions to the owner while denying all permissions to group and others. This is typically sufficient for executable files that do not need to be modified after download.

Here’s the suggested code change:

Suggested change
os.chmod(target_path, 0o700) # Read/execute only for owner
os.chmod(target_path, 0o500) # Read/execute only for owner

This comment was generated by an experimental AI tool.

aqt/installer.py Outdated

with tempfile.TemporaryDirectory(prefix="qt_install_") as temp_dir:
temp_path = Path(temp_dir)
os.chmod(temp_dir, 0o700)

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Security issue: These permissions 0o700 are widely permissive and grant access to more people than may be necessary.

The issue identified by the Semgrep linter is that setting the permissions of the temporary directory to 0o700 (read, write, and execute permissions for the owner only) may be considered overly permissive in this context. While 0o700 restricts access to the owner, it may still be unnecessary depending on the application's security requirements. If the temporary directory is used to store sensitive information, it would be prudent to restrict permissions further.

A more secure option would be to set the permissions to 0o600, which grants read and write permissions only to the owner, preventing any access by others.

Here’s the code suggestion to fix the issue:

Suggested change
os.chmod(temp_dir, 0o700)
os.chmod(temp_dir, 0o600)

This comment was generated by an experimental AI tool.

aqt/installer.py Outdated
try:
os.chdir(working_dir)
self.logger.debug(f"Executing installer with command: {' '.join(safe_args)}")
subprocess.run(safe_args, shell=False, check=True, cwd=working_dir)

Choose a reason for hiding this comment

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

⚠️ Codacy found a medium Security issue: subprocess call - check for execution of untrusted input.

The issue identified by the Bandit linter pertains to the potential execution of untrusted input when using subprocess.run(). Although shell=False is set, which is a good practice as it avoids shell injection vulnerabilities, the command being executed (safe_args) still includes user-provided input (from cmd[1:]). If any of those arguments are untrusted, they could lead to security vulnerabilities.

To mitigate this issue, it is essential to ensure that the arguments being passed to subprocess.run() are sanitized and validated. One way to achieve this is by using a safer method to build the command, ensuring that only trusted inputs are included.

Here's a single line change that ensures the command is built from a known list of trusted arguments:

Suggested change
subprocess.run(safe_args, shell=False, check=True, cwd=working_dir)
safe_args.extend([arg for arg in cmd[1:] if is_trusted(arg)]) # Replace is_trusted with actual validation logic

In this suggestion, is_trusted(arg) would be a placeholder for a validation function that checks whether the argument is safe to use. You would need to implement is_trusted to fit your security requirements.


This comment was generated by an experimental AI tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant