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(query): Show privilege_escalation_allowed k8s alert also in case no securityContext is defined #4885

Merged

Conversation

Churro
Copy link
Contributor

@Churro Churro commented Feb 27, 2022

Problem

The privilege_escalation_allowed rule is rated high and, indeed, represents a very security-relevant setting. Currently, the rule does not trigger an alert in case no securityContext is defined with a container. There is a separate rule to check whether securityContext exists but the severity rating is low. Hence, alerts for a rule with high severity are only shown if a user acts upon a low severity rule.

Therefore, I suggest to change the rule to always show the alert, regardless of the existence of securityContext. This is important since a report otherwise does not provide a full picture of issues that demand for action.

Proposed Changes

  • Make the definition of securityContext optional, such that an alert is also shown in case container.securityContext is undefined

I submit this contribution under the Apache-2.0 license.

@kicsbot
Copy link
Contributor

kicsbot commented Feb 27, 2022

Scan submitted to Checkmarx

Copy link
Contributor

@rafaela-soares rafaela-soares left a comment

Choose a reason for hiding this comment

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

Another great observation and refactor, @Churro 🚀 Thank you!

I have two request changes:

@kicsbot
Copy link
Contributor

kicsbot commented Mar 2, 2022

Scan not submitted to Checkmarx due to existing Active scan for the same project.

1 similar comment
@kicsbot
Copy link
Contributor

kicsbot commented Mar 2, 2022

Scan not submitted to Checkmarx due to existing Active scan for the same project.

@Churro
Copy link
Contributor Author

Churro commented Mar 2, 2022

Suggestions applied, thank you for the thorough review @rafaela-soares! 👍

@kicsbot
Copy link
Contributor

kicsbot commented Mar 2, 2022

Logo
Checkmarx SAST - Scan Summary & Details

Cx-SAST Summary

Total of 5 vulnerabilities
High 0 High
Medium 0 Medium
Low 5 Low
Info 0 Info

Violation Summary

No policy violation found

@rafaela-soares rafaela-soares added query New query feature community Community contribution labels Mar 15, 2022
Copy link
Collaborator

@cx-joao-reigota cx-joao-reigota left a comment

Choose a reason for hiding this comment

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

Hi @Churro, everything looks good to me, the only thing I would like to request here is the use of the searchLine

Thank you for your great contributions

@Churro
Copy link
Contributor Author

Churro commented Mar 15, 2022

Hi @joaoReigota1, thank you for the review. I've now added searchLine and another positive testcase related to that

Copy link
Collaborator

@cx-joao-reigota cx-joao-reigota left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@rafaela-soares rafaela-soares left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@cx-joao-reigota cx-joao-reigota merged commit 6cab947 into Checkmarx:master Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution query New query feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants