bridgecrewio / checkov

Prevent cloud misconfigurations and find vulnerabilities during build-time in infrastructure as code, container images and open source packages with Checkov by Bridgecrew.
https://www.checkov.io/
Apache License 2.0
7.02k stars 1.1k forks source link

Incorrect logic applied to security group checks when `cidr_blocks` & `self = true` are defined. #6073

Open jmcmillan1873 opened 6 months ago

jmcmillan1873 commented 6 months ago

Describe the issue Currently checkov skips checking an ingress rules if that rule contains self=true . This is true even if cidr_blocks = ["0.0.0.0/0"] or ipv6_cidr_blocks = ["::/0"] is present in the same rule.

i.e. If an ingress contains both self and one of cidr_blocks or ipv6_cidr_blocks - the current checkov logic skips checking the ingress rule due to the presence of self. The result is that potentially insecure ingress rules are flagged as passed/OK - resulting in an apparent clean bill of health for the SG.

known affected Check ID's: CKV_AWS_24 CKV_AWS_25 CKV_AWS_260

Examples Consider the security group below. I expect it to trigger various objections to the ingress rule, but it does not. This security group passes checkov tests. If you remove self = true from the code snippet below, checkov correctly identifies the insecure rules.

resource "aws_security_group" "common" {
  name        = "Common SG for all instances"
  description = "Base SG for all isntances"
  vpc_id      = module.vpc.vpc_id

  ### Ingress rules:
  # Create an ingress rule to allow ssh from self
  ingress {
    from_port   = 22
    to_port     = 3389
    protocol    = "tcp"
    self        = true
    cidr_blocks = ["0.0.0.0/0"]
    description = "Allow SSH access from this SG."
  }

  ### Egress rules:
  # Allow all outbound traffic. 
  egress {
    from_port   = 0
    to_port     = 0
    protocol    = "-1"
    cidr_blocks = ["0.0.0.0/0"]
    description = "Allow all outbound traffic."
  }
}

Version (please complete the following information):

Additional context As far as I can tell the issue lies with the logic in terraform/checks/resource/aws/AbsSecurityGroupUnrestrictedIngress.py.

If I comment out the check_self test from the evaluation logic in AbsSecurityGroupUnrestrictedIngress.py, a subsequent checkov run correctly highlights the insecure ingress rules in my SG definition.

        if 'ingress' in conf:  # This means it's an SG resource with ingress block(s)
            ingress_conf = conf['ingress']
            for ingress_rule in ingress_conf:
                for rule in force_list(ingress_rule):
                    if isinstance(rule, dict):
#                        if self.check_self(rule):
#                           return CheckResult.PASSED
                        if self.contains_violation(rule):
                            self.evaluated_keys = [
                                f'ingress/[{ingress_conf.index(ingress_rule)}]/from_port',
                                f'ingress/[{ingress_conf.index(ingress_rule)}]/to_port',
                                f'ingress/[{ingress_conf.index(ingress_rule)}]/cidr_blocks',
                                f'ingress/[{ingress_conf.index(ingress_rule)}]/ipv6_cidr_blocks',
                            ]
                            return CheckResult.FAILED

I understand the reason for skipping self and not flagging that as insecure, but could the check_self definition be modified to return False if self is paired with cidr_blocks or ipv6_cidr_blocks? (Sorry - I'd have a go myself, but I don't yet have the python knowledge to suggest a fix in a reasonable time frame.)

stale[bot] commented 3 weeks ago

Thanks for contributing to Checkov! We've automatically marked this issue as stale to keep our issues list tidy, because it has not had any activity for 6 months. It will be closed in 14 days if no further activity occurs. Commenting on this issue will remove the stale tag. If you want to talk through the issue or help us understand the priority and context, feel free to add a comment or join us in the Checkov slack channel at codifiedsecurity.slack.com Thanks!