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

add check if there is actually a package manager in the run command #55

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Morl99
Copy link

@Morl99 Morl99 commented Jan 16, 2024

Simplify rule to use regex instead of splitting

closes https://github.com/aquasecurity/defsec/issues/1256

@Morl99 Morl99 requested a review from simar7 as a code owner January 16, 2024 09:40
@CLAassistant
Copy link

CLAassistant commented Jan 16, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Morl99
Copy link
Author

Morl99 commented Jan 16, 2024

The CLA checker does not seem to be aware, that I am a member of the https://github.com/dbsystel organization, and that our organization has signed a CLA.

@simar7
Copy link
Member

simar7 commented Jan 17, 2024

Thanks could you format the rego check with opa fmt -w?

add check if there is actually a package manager in the run command
@Morl99
Copy link
Author

Morl99 commented Jan 22, 2024

Thanks could you format the rego check with opa fmt -w?

Done

I have added a line about that to the CONTRIBUTING.md.


array_split[len - 1] == update[_]
is_valid_update(command) {
regex.match(update_regex, command)
Copy link
Contributor

Choose a reason for hiding this comment

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

When using regex, we will not be able to establish that the update command refers to the package manager. Example of a command that causes a false positive:apt-get build-dep && /bin/sh /scripts/someScript.sh update. Maybe we should split the command by && and check each part?

@nikpivkin
Copy link
Contributor

@simar7 I think we should also check the package manager along with the install command.
apt-get update && /bin/sh /scripts/someScript.sh install doesn't trigger a warning, but this apt-get update && /bin/sh /scripts/someScript.sh other-cmd does. Does it relate to this PR?

@Morl99
Copy link
Author

Morl99 commented Feb 13, 2024

@simar7 I think we should also check the package manager along with the install command.

apt-get update && /bin/sh /scripts/someScript.sh install doesn't trigger a warning, but this apt-get update && /bin/sh /scripts/someScript.sh other-cmd does. Does it relate to this PR?

True, that is the other side of the original issue, that we could fix here as well. I will rework the code to also cover this false negative.

@simar7
Copy link
Member

simar7 commented Feb 14, 2024

@simar7 I think we should also check the package manager along with the install command.
apt-get update && /bin/sh /scripts/someScript.sh install doesn't trigger a warning, but this apt-get update && /bin/sh /scripts/someScript.sh other-cmd does. Does it relate to this PR?

True, that is the other side of the original issue, that we could fix here as well. I will rework the code to also cover this false negative.

Sounds good to me.

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.

4 participants