prowler-cloud / prowler

Prowler is an Open Source Security tool for AWS, Azure, GCP and Kubernetes to do security assessments, audits, incident response, compliance, continuous monitoring, hardening and forensics readiness. Includes CIS, NIST 800, NIST CSF, CISA, FedRAMP, PCI-DSS, GDPR, HIPAA, FFIEC, SOC2, GXP, Well-Architected Security, ENS and more
https://prowler.com
Apache License 2.0
10.54k stars 1.51k forks source link

Many false positives for check awslambda_function_not_publicly_accessible #4870

Open pr3l14t0r opened 2 weeks ago

pr3l14t0r commented 2 weeks ago

Steps to Reproduce

Ahoy! I am running prowler periodically on AWS. Since the latest version (Prowler 4.3.5) i am getting a lot of false positives on check awslambda_function_not_publicly_accessible.

From my understanding, the code is missing a check whether a condition is set in the resource based policy.

Here's a redacted example policy that i have in place for a lambda-script:

{
  "Version": "2012-10-17",
  "Id": "default",
  "Statement": [
    {
      "Sid": "awslambda-myLambdaScript-LambdaInvokePermission",
      "Effect": "Allow",
      "Principal": {
        "Service": "ses.amazonaws.com"
      },
      "Action": "lambda:InvokeFunction",
      "Resource": "arn:aws:lambda:<REGION>:<ACCOUNT-ID>:function:myLambdaScript",
      "Condition": {
        "StringEquals": {
          "AWS:SourceAccount": "<ACCOUNT-ID>"
        }
      }
    }
  ]
}

Notice that there's a condition set to mitigate access from public. This condition allows SES to only invoke this function from within the account.

But still this policy would get flagged as failed check, stating:

Lambda function myLambdaFunction has a policy resource-based policy with public access.

Neither am i a python expert, nor a prowler-developer, but shouldn't a condition statement get taken into consideration to determine whether a function is publicly accessible or not?

Anyway - I have multiple of such Resource-based policy documents and all of these get flagged as publicly accessible, when they are in fact not.

Could you double check and help me out verifying whether it's a false positive // the check's code is wrong?

Cheers. :)

Expected behavior

The check awslambda_function_not_publicly_accessible should not fail when the Resource-based policy document contains a Condition which restricts access, like:

      "Condition": {
        "StringEquals": {
          "AWS:SourceAccount": "<ACCOUNT-ID>"
        }
      }

Actual Result with Screenshots or Logs

When using a Condition, the check would still fail and state:

Lambda function myLambdaFunction has a policy resource-based policy with public access.

How did you install Prowler?

From pip package (pip install prowler)

Environment Resource

  1. Fargate Task

OS used

Container source image: docker.io/library/python:3.12-slim

cat /etc/os-release

PRETTY_NAME="Debian GNU/Linux 12 (bookworm)"
NAME="Debian GNU/Linux"
VERSION_ID="12"
VERSION="12 (bookworm)"

Prowler version

4.3.5

Pip version

24.2

Context

No response

pedrooot commented 2 weeks ago

Hey! @pr3l14t0r thanks for the report, we are evaluating this issue with the team and we will reach you with a solution!

pr3l14t0r commented 2 weeks ago

Small and delayed addendum that would've fit in Context:

I have upgraded from Version 4.2.4 to 4.3.5 yesterday, and after that run with 4.3.5 i noticed the false positives for that check. Maybe that information helps.

puchy22 commented 2 weeks ago

Hi @pr3l14t0r,

Thank you for bringing this to our attention. We were aware that the check feature might occasionally result in false positives, which is why we added a note about this in the metadata, exactly in the Note field. The introduction of this feature was actually in response to an issue we identified earlier false negatives.

The root of the problem isn't with the Condition itself, but rather with how the check now evaluates whether a function can be invoked by other resources, as in your case. We flagged this as a potential exposure risk because if a function can be invoked by an ALB or API Gateway, there's a possibility it could be publicly exposed. This is why the check currently marks such cases as a FAIL. However, the challenge is that we can't definitively determine if the resource allowed to invoke the function is indeed exposing it publicly.

After further discussion, weโ€™ve decided to implement the following improvements:

  1. Update the status_extended field: We'll modify the messaging to more clearly reflect the nature of the issue.
  2. Adjust the severity level: For cases like this, we'll lower the severity of the check to MEDIUM.

If, after analyzing your specific case, you find that the result isnโ€™t applicable, you have the option to mute the finding. You can find more details on how to do this in our documentation. Iโ€™ll notify you once the PR with these changes is available.

Thanks again for your report and for using Prowler! ๐Ÿš€

pr3l14t0r commented 2 weeks ago

Heyho @puchy22 , thanks for taking care and for your thorough analysis of my request, highly appreciated! :)

I don't know whether adjusting the severity from Critical to Medium sounds like a good approach. As a user and in my context: I have set up a daily scan of multiple AWS accounts. I would get a notification for all new and critical failed scans reported by prowler. Decreasing the severity would mean that i won't get notified on new public lambda scripts.

If the solution you came up with does not mitigate false positives given my policy-example (where the lambda script definitely is not public, but public in the same source aws account though), i would have to mute the whole scan which is bad. I can't exclude lambda scripts by name because there are tons of thousands that i would have to check.

I really value that check and it made a lot of sense, until the false positives started. ๐Ÿ˜…

I'll definitely check out the new variant once it became available, until then i will stay on version 4.2.4.

Thanks again for your report and for using Prowler! ๐Ÿš€

Don't thank me for using prowler. You are the heroes here for actively maintaining an awesome tool and keeping it open-source, making the world a more secure place. Thanks so much for your work and efforts! :)

You guys bow to no-one!

kagahd commented 2 weeks ago

We have exactly the same problem and therefore disabled the check, hopefully only temporarily. We consider to customize this check to take into account appropriate condition statements in the resource-based policy document. However, we'd prefer to use prowler's standard checks instead customized ones.

christiandavilakoobin commented 2 weeks ago

Same here. The only solutions seems to be mute the alert, which is a lot worse because there may be a public lambda and we didn't notice.

Thanks!

puchy22 commented 2 weeks ago

Hello again @pr3l14t0r @kagahd @chrisdlangton,

I would like to clarify a few points from my previous response that I believe may not have been entirely clear.

  1. The check will remain CRITICAL, but in this specific case where it is believed to be potentially exposed like it works before, it will dynamically change to MEDIUM only in the cases where the function could be exposed by other AWS service. I am aware that this solution is not perfect, as it does not address the true issue, which is having a false positive. However, controlling the false positive problem is not currently feasible due to the complexity of variables that can affect it. If I see that a function can be invoked by a service from a certain account, it is very difficult or impossible for Prowler to determine whether the service allowing the invocation is properly secured. This uncertainty is why we want to generate a finding, and it's also why I believe the severity could be lowered dynamically like happens in ec2_instance_port_*_exposed_to_internet. To classify something as CRITICAL, you need to be very certain that there is indeed a problem.
  2. The issue is not with the Condition in the policy. In the specific case referenced in this issue, the problem is that the Principal is a Service and the Action is InvokeFunction. The logic behind the check is: if the SES service can invoke the function, then itโ€™s possible that this service could be publicly exposing my Lambda function. Probably other solution/feature to add could be to make a list of services that we do not want that can invoke the function because it could be exposing the function like ELBs, APIGateways, etc. and let service like SES not giving this findings because is not common that SES will expose the function.
  3. The check has always produced false positives, but now instead of PASS, they are resulting in FAILs. In my particular case, this was included because a function was connected to an ALB in another account, which was publicly exposing the function, but the check was reporting a PASS. The same applied to API Gateways or services that redirect HTTP traffic externally.

With all of this clarified, I want to mention that I am discussing with the team whether to maintain these changes, where the check would dynamically change severity (meaning it would function exactly as before, but MEDIUM findings would belong to this feature of being exposed by another insecure resource), or to revert the check to its previous behavior based on the feedback we've received. You are right that the mute list could become complex if you don't want to see the finding, even if it's MEDIUM.

Finally, thank you very much for all the feedback; it helps us improve the checks and everything related to Prowler ๐Ÿš€

chrisdlangton commented 2 weeks ago

โ“โ“โ“

jfagoagas commented 2 weeks ago

Hello @chrisdlangton @kagahd @christiandavilakoobin @pr3l14t0r @puchy22 I was carefully reviewing the issue and I think a good solution, as you mentioned before, would be to check if a condition is present in the Lambda policy. I think we can also improve the check using the is_condition_block_restrictive function but at least with checking the condition key we will reduce false positives.

It seems the good practice from AWS when you need to give access from a service is to always use the Condition key block https://docs.aws.amazon.com/lambda/latest/dg/access-control-resource-based.html

What do you think?

Thanks for your feedback to always take care of Prowler.

kagahd commented 2 weeks ago

It seems the good practice from AWS when you need to give access from a service is to always use the Condition key block

That's true. If the function is_condition_block_restrictive is used, the argument is_cross_account_allowed should explicitly set to True. This should reduce a lot of false positives. However, the concerns of @puchy22 to produce false negatives would then be still there because firstly the condition statement isn't evaluated (so it can be "malicious") and secondly because the service, e.g. ELB, ApiGateway, SES etc. could publicly expose the Lambda function. @puchy22's suggestion to make a list of services that we do not want that can invoke the function would indeed harden the check which reduces false negatives. So I think both approaches are good to improve this check.

Btw. why the argument is_cross_account_allowed of the function is_condition_block_restrictive is set to True only in both checks:

...while it's not in the other four checks?

chbiel commented 2 weeks ago

I appreciate your suggests, but in the end we would still see the false positives and would still need to disable the whole check. As far as I know there is right now no way to mute (e.g. via Mutelist) explicit severities, or am I wrong? Would it be possible, to extend the Mutelist to support "Ignore all Medium finding of this check"?

That would allow us to still get Critical finding, but mute more detailed. Maybe in general a feature, that would allow users to use the Mutelist more efficient?

jfagoagas commented 2 weeks ago

Reply to @kagahd

That's true. If the function is_condition_block_restrictive is used, the argument is_cross_account_allowed should explicitly set to True. This should reduce a lot of false positives. However, the concerns of @puchy22 to produce false negatives would then be still there because firstly the condition statement isn't evaluated (so it can be "malicious") and secondly because the service, e.g. ELB, ApiGateway, SES etc. could publicly expose the Lambda function. @puchy22's suggestion to make a list of services that we do not want that can invoke the function would indeed harden the check which reduces false negatives. So I think both approaches are good to improve this check.

So, you mean to add some configuration to allow services? I think just checking the condition clause will be more than enough since AWS strongly recommends to add a condition to restrict the caller origin, specially when using some AWS' service principal.

Btw. why the argument is_cross_account_allowed of the function is_condition_block_restrictive is set to True only in both checks:

  • sqs_queues_not_publicly_accessible
  • s3_bucket_public_access

...while it's not in the other four checks?

  • dynamodb_table_cross_account_access
  • iam_role_cross_service_confused_deputy_prevention
  • sns_topics_not_publicly_accessible
  • vpc_endpoint_connections_trust_boundaries

That's a question that I don't have the answer now, we will review it and let you know.

jfagoagas commented 2 weeks ago

Reply to @chbiel

I appreciate your suggests, but in the end we would still see the false positives and would still need to disable the whole check. As far as I know there is right now no way to mute (e.g. via Mutelist) explicit severities, or am I wrong? Would it be possible, to extend the Mutelist to support "Ignore all Medium finding of this check"?

That would allow us to still get Critical finding, but mute more detailed. Maybe in general a feature, that would allow users to use the Mutelist more efficient?

Currently there is no way to mute severities but we could think about it. What you can do actually is to use the --severity and --checks CLI argument to just run the checks you want but I think in this particular case we need to improve/fix the check's logic.

I think just checking the policy's Condition block would be more than enough to reduce false positives. Could you share, redacting private data, a policy for a Lambda that is being flagged as a FAIL?

The following policy will be flagged as a PASS if we introduce the Condition block check:

{
    "Version": "2012-10-17",
    "Id": "default",
    "Statement": [
        {
            "Sid": "lambda-allow-s3-my-function",
            "Effect": "Allow",
            "Principal": {
              "Service": "s3.amazonaws.com"
            },
            "Action": "lambda:InvokeFunction",
            "Resource":  "arn:aws:lambda:us-east-2:123456789012:function:my-function",
            "Condition": {
              "StringEquals": {
                "AWS:SourceAccount": "123456789012"
              },
              "ArnLike": {
                "AWS:SourceArn": "arn:aws:s3:::amzn-s3-demo-bucket"
              }
            }
        }
     ]
}
chbiel commented 2 weeks ago

Sure :)

Example with secretsmanager principal:

{
  "Version": "2012-10-17",
  "Id": "default",
  "Statement": [
    {
      "Sid": "AllowSecretsManagerInvoke",
      "Effect": "Allow",
      "Principal": {
        "Service": "secretsmanager.amazonaws.com"
      },
      "Action": "lambda:InvokeFunction",
      "Resource": "arn:aws:lambda:eu-central-1:000000000000:function:...",
      "Condition": {
        "StringEquals": {
          "AWS:SourceAccount": "000000000000"
        }
      }
    }
  ]
}

example with events principal:

{
  "Version": "2012-10-17",
  "Id": "default",
  "Statement": [
    {
      "Sid": "AllowEventsInvoke",
      "Effect": "Allow",
      "Principal": {
        "Service": "events.amazonaws.com"
      },
      "Action": "lambda:InvokeFunction",
      "Resource": "arn:aws:lambda:eu-central-1:000000000000:function:...",
      "Condition": {
        "ArnLike": {
          "AWS:SourceArn": "arn:aws:events:eu-central-1:000000000000:rule/..."
        }
      }
    }
  ]
}
kagahd commented 2 weeks ago

So, you mean to add some configuration to allow services?

No, I thought to deny services that may publicly expose the Lambda function but maybe I got @puchy22's suggestion wrong. On the other side, simply denying such services would be too harsh because for example an ALB could use only private IP's or an APIGateway could have a VPC-connection. Both are not public and should not be denied, means the prowler check should not FAIL in such cases. It would be quite difficult or even impossible for the prowler check to verify whether the service is public or not.

I think just checking the condition clause will be more than enough since AWS strongly recommends to add a condition to restrict the caller origin

But the condition clause could be too open, allowing public access, couldn't it?

That's a question that I don't have the answer now, we will review it and let you know.

Of course, I understand that. No rush.

Could you share, redacting private data, a policy for a Lambda that is being flagged as a FAIL?

Here you are. Here is an example to allow S3 invoke:

{
  "Version": "2012-10-17",
  "Id": "default",
  "Statement": [
    {
      "Sid": "AllowS3Invoke",
      "Effect": "Allow",
      "Principal": {
        "Service": "s3.amazonaws.com"
      },
      "Action": "lambda:InvokeFunction",
      "Resource": "arn:aws:lambda:eu-central-1:123456789012:function:mylambda",
      "Condition": {
        "ArnLike": {
          "AWS:SourceArn": "arn:aws:s3:::mybucket"
        }
      }
    }
  ]
}

Here is an example to allow execution from an API gateway:


{
  "Version": "2012-10-17",
  "Id": "default",
  "Statement": [
    {
      "Sid": "AllowExecutionFromAPIGateway",
      "Effect": "Allow",
      "Principal":{
               "Service": "apigateway.amazonaws.com"
      },
      "Action": "lambda:InvokeFunction",
      "Resource": "arn:aws:lambda:eu-central-1:123456789012:function:myfunction",
      "Condition": {
        "ArnLike":{
                   "AWS:SourceArn": "arn:aws:execute-api:eu-central-1:123456789012:abcdefghi/*/GET/\{proxy+}"
        }
      }
    }
  ]
}
jfagoagas commented 2 weeks ago

@kagahd @chbiel

Regarding the policies you shared it will be quite easy to improve the check's behaviour as I was mentioning earlier. The key point here is to set that if the policy has a Condition block the AWS Lambda Function will not be flagged as a FAIL unless the policy has an *. So, for your policy examples:

Secrets Manager + Lambda

The following policy will flag the Lambda as PASS using the is_condition_block_restrictive function, either the audited account is set or another cross-account, in that case setting is_cross_account_allowed = True.

"Principal": {
        "Service": "secretsmanager.amazonaws.com"
},
"Condition": {
        "StringEquals": {
          "AWS:SourceAccount": "000000000000"
        }
}

CloudWatch Events + Lambda

The following policy will flag the Lambda as PASS using the is_condition_block_restrictive function, either the audited account is set or another cross-account, in that case setting is_cross_account_allowed = True.

"Principal": {
        "Service": "events.amazonaws.com"
},
"Condition": {
        "ArnLike": {
          "AWS:SourceArn": "arn:aws:events:eu-central-1:000000000000:rule/..."
        }
      }

S3 + Lambda

The following policy will flag the Lambda as PASS using the is_condition_block_restrictive function. In this case the is_condition_block_restrictive will need to handle the case where the ARN does not contains an AWS Account Number.

"Principal": {
        "Service": "s3.amazonaws.com"
},
"Condition": {
        "ArnLike": {
          "AWS:SourceArn": "arn:aws:s3:::mybucket"
        }
}

API Gateway + Lambda

The following policy will flag the Lambda as PASS using the is_condition_block_restrictive function, either the audited account is set or another cross-account, in that case setting is_cross_account_allowed = True.

"Principal": {
        "Service": "apigateway.amazonaws.com"
},
"Condition": {
        "ArnLike":{
                   "AWS:SourceArn": "arn:aws:execute-api:eu-central-1:123456789012:abcdefghi/*/GET/\{proxy+}"
        }
      }

Please, let us know what do you think about these use cases? The idea is to implement the fix and add the above as tests.

To-Do

kagahd commented 1 week ago

Hi @jfagoagas, I think your suggestion would remove a lot of false positives which would make the check useful for us again. However, as @puchy22 pointed out, the check may still produce false negatives (but not more than before) because of transitive dependencies which the check cannot verify. In my AllowExecutionFromAPIGateway example above, if the APIGateway is publicly accessible, then the Lambda function is also publicly accessible.
However, if a Lambda function must be publicly accessible, then it's common and good practice to put an APIGateway or an ALB etc. as a layer of defense in front of the Lambda to avoid making the Lambda directly publicly accessible. Furthermore, prowler would alert the publicly accessible APIGateway or ALB anyway as long as prowler is run against the account they are running on. So I think your suggestion is a good solution. I agree also with the two points in your To-Do block.

jfagoagas commented 1 week ago

Hi @jfagoagas, I think your suggestion would remove a lot of false positives which would make the check useful for us again. However, as @puchy22 pointed out, the check may still produce false negatives (but not more than before) because of transitive dependencies which the check cannot verify. In my AllowExecutionFromAPIGateway example above, if the APIGateway is publicly accessible, then the Lambda function is also publicly accessible. However, if a Lambda function must be publicly accessible, then it's common and good practice to put an APIGateway or an ALB etc. as a layer of defense in front of the Lambda to avoid making the Lambda directly publicly accessible. Furthermore, prowler would alert the publicly accessible APIGateway or ALB anyway as long as prowler is run against the account they are running on. So I think your suggestion is a good solution. I agree also with the two points in your To-Do block.

That's great! Thank you for the insight, we will work on this next week.

jchrisfarris commented 1 week ago

I think there is also a distinction between publicly accessible and publicly invokable. In many cases, and "serverless api" has publicly accessible Lambda functions, but they're properly invoked by the upsteam service (ALB, API GW). I'd want to address these at the upsteam service. Either with WAF, or asking "why is this on the internet to begin with?"

OTOH, a lambda URL with Auth NONE, or a lambda with Principal: AWS: * in the resource policy is publicly invokable, and a much larger concern.

One place where this check would be more useful is if I have a function that's publicly accessible as defined currently, and Lambda runtime is deprecated.