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
6.71k stars 1.07k forks source link

BaseModuleCheck: using locals with deeply nested arguments inside functions causes improper `conf` escaping #6380

Open nomeelnoj opened 1 month ago

nomeelnoj commented 1 month ago

Checkov Version: 3.2.116

Describe the issue

We have many custom checks written for our modules, because we want to validate against the content users submit, not necessarily the resulting resources.

In one such module, we use a deeply nested data structure to handle our AWS SSO / Identity Center permission sets. Because we have so many permission sets, we organize them into individual files as locals for ease of maintenance and readability.

When writing a recent check to validate against the actions in a policy, I found improper escaping in conf when specific values are passed in, and only when they are passed in as locals.

Problem

Below is a super simplified example of what we are seeing. I understand that the mappings argument is complex, and therefore might need to be escaped, but the actual issue we are seeing is that the policy_statements argument, which is relatively simple, is also escaped when the below conditions are met.

This only happens when all of the following are true:

  1. The test value in permission_sets is passed in from a local (vs passed directly into the module)
  2. The accounts value in mappings uses flatten or concat at the start
  3. One of the values being passed to flatten or concat is a lookup, e.g. v["account"]

If any one of the above is not true, the policy_statements are not escaped. This. makes it very difficult to write any custom checks as the unpredictability ensures we cannot guarantee the structure of the data that we are trying to parse and manipulate.

Example Value

locals {
  test = {
    policy_statements = {
      iam_pass_role = {
        sid    = "AllowIamPassRoleForLaunchingFromServices"
        effect = "Allow"
        actions = [
          "iam:PassRole",
        ]
        resources = [
          "*",
        ]
      }
    }
    mappings = concat(
      [
        for k, v in {
          DevDevelopers = {
            name    = "DevelopersDev"
            env     = "dev"
            account = "1234567890"
          }
        } :
        {
          group = v["name"]
          accounts = flatten( # Removing this flatten will also fix the issue
            [
              v["account"], # This is what causes the issue, but only when `flatten` or `concat` is present a few lines earlier
              "2345678901",
            ]
          )
        }
      ]
    )
  }
}

module "simple" {
  source = "../../modules/foo"
  permission_sets = {
    test = local.test # If you pass the value above in directly here versus calling it from a local. the issue disappears
  }
}

Example custom check, built for debugging purposes:

from checkov.common.models.enums import CheckResult, CheckCategories
from checkov.common.bridgecrew.severities import Severities
from checkov.terraform.checks.module.base_module_check import BaseModuleCheck

class Simple(BaseModuleCheck):
    def __init__(self):
        name = (
            "Ensure policy statement actions are valid."
        )
        id = "CKV_FOO_1"
        categories = [CheckCategories.IAM]
        super().__init__(name=name, id=id, categories=categories)
        self.severity = Severities["HIGH"]

    def scan_module_conf(self, conf):
        """
            All policy statements should contain valid actions
        :return: <CheckResult>
        """
        print(conf)
        return CheckResult.PASSED

check = Simple()

Output

Expected Output: (line breaks added for readability)

[ terraform framework ]:   0%|                    |[0/1], Current File Scanned=main.tf
{
  '__end_line__': 43,
  '__resolved__': [],
  '__start_line__': 38,
  'permission_sets': [
    {
      'test': {
        'mappings': '${concat([for k , v in {\'DevDevelopers\': {\'name\': \'DevelopersDev\', 
\'env\': \'dev\', \'account\': \'1234567890\'}} : {\'group\': \'${v["name"]}\', \'accounts\': \'${flatten([\\\'${v["account"]}\\\', 
\\\'2345678901\\\'])}\'}])}',
        'policy_statements': {
          'iam_pass_role': {
            'actions': ['iam:PassRole'],
            'effect': 'Allow',
            'resources': ['*'],
            'sid': 'AllowIamPassRoleForLaunchingFromServices'
          }
        }
      }
    }
  ],
  'source': ['../../modules/foo'],
  '__address__': 'simple'
}

Actual Output: (line breaks added for readability)

[ terraform framework ]:   0%|                    |[0/1], Current File Scanned=main.tf
{
  '__end_line__': 43,
  '__resolved__': [],
  '__start_line__': 38,
  'permission_sets': [
    {
      'test': '{\'mappings\': \'${concat([for k , v in {\\\'DevDevelopers\\\': {\\\'name\\\': 
\\\'DevelopersDev\\\', \\\'env\\\': \\\'dev\\\', \\\'account\\\': \\\'1234567890\\\'}} : {\\\'group\\\': \\\'${v["name"]}\\\', \\\'accounts\\\': 
\\\'${flatten([\\\\\'${v["account"]}\\\\\', \\\\\'2345678901\\\\\'])}\\\'}])}\', \'policy_statements\': {\'iam_pass_role\': {\'actions\': 
\'iam:PassRole\', \'effect\': \'Allow\', \'resources\': \'*\', \'sid\': \'AllowIamPassRoleForLaunchingFromServices\'}}}'
    }
  ],
  'source': ['../../modules/foo'],
  '__address__': 'simple'
}

As you can see from above, in the actual output, the policy_statements section of conf is escaped, when it should not be.