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

fix: [M3-7081] - Quote variable in changeset shell command #9791

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Oct 16, 2023

Description 📝

We're getting a codeQL warning for "Shell command built from environment values" for not properly handling the variable in the shell command.

Dynamically constructing a shell command with values from the local environment, such as file paths, may inadvertently change the meaning of the shell command. Such changes can occur when an environment value contains characters that the shell interprets in a special way, for instance quotes and spaces. This can result in the shell command misbehaving, or even allowing a malicious user to execute arbitrary commands on the system.

Recommendation
If possible, use hard-coded string literals to specify the shell command to run, and provide the dynamic arguments to the shell command separately to avoid interpretation by the shell.

Changes 🔄

  • Add quotes to the git add shell command in the changeset job

Preview 📷

No visual change

How to test 🧪

If you have the codeQL extension on VSCode, you can set up a codeQL database for the manager repo and run the IncompleteSanitization.ql query (from the codeql repo - javascript >> ql >> src >> Security >> CWE-78 >> ShellCommandInjectionFromEnvironment.ql) on the manager database. Instructions for setting up the extension here: https://codeql.github.com/docs/codeql-for-visual-studio-code/setting-up-codeql-in-visual-studio-code/ (+ I also installed the CLI). guide

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

Commit message and pull request title format standards

Note: Remove this section before opening the pull request
Make sure your PR title and commit message on squash and merge are as shown below

<commit type>: [JIRA-ticket-number] - <description>

Commit Types:

  • feat: New feature for the user (not a part of the code, or ci, ...).
  • fix: Bugfix for the user (not a fix to build something, ...).
  • change: Modifying an existing visual UI instance. Such as a component or a feature.
  • refactor: Restructuring existing code without changing its external behavior or visual UI. Typically to improve readability, maintainability, and performance.
  • test: New tests or changes to existing tests. Does not change the production code.
  • upcoming: A new feature that is in progress, not visible to users yet, and usually behind a feature flag.

Example: feat: [M3-1234] - Allow user to view their login history


@abailly-akamai abailly-akamai self-assigned this Oct 16, 2023
@abailly-akamai abailly-akamai marked this pull request as ready for review October 16, 2023 13:04
@abailly-akamai abailly-akamai requested a review from a team as a code owner October 16, 2023 13:04
@abailly-akamai abailly-akamai requested review from coliu-akamai and tyler-akamai and removed request for a team October 16, 2023 13:04
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

🎉 Verified that the warning no longer appears when running the query!

Before After
image image

(to get the before image, I ran the ShellCommandInjectionFromEnvironment.ql query on the original changeset script file - the one without the quotes.)

@coliu-akamai coliu-akamai added the Add'tl Approval Needed Waiting on another approval! label Oct 16, 2023
@tyler-akamai
Copy link
Contributor

Verified that there are no warnings, great job!

Screenshot 2023-10-16 at 5 15 40 PM

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 16, 2023
@abailly-akamai abailly-akamai merged commit 31a89ac into linode:develop Oct 17, 2023
11 checks passed
abailly-akamai added a commit to abailly-akamai/manager that referenced this pull request Oct 17, 2023
* Quote variable in changeset shell

* Added changeset: Quote variable in changeset shell command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants