feat(internet-exposed): Improve publicly accessible checks to include targets of ELBs #3985

abant07 · 2024-05-13T17:06:29Z

Context

Currently, we are checking if resources are internet facing and then flagging it as a failed test to the user, however, there is possibility that the user has configured security groups for their resources but have forgotten to configure for their load balancers. This can potentially be a security threat as anyone from the internet can access their load balancer and have the ability to hack their resources.

Description

No dependencies have been added, however, I have added 2 checks for EC2, 1 check for Lambda, and 1 check for ECS to make sure that ELBs and ELBv2s are either internal or if they are internet facing they should have security groups.

New checks:

awslambda_function_not_directly_publicly_accessible_via_elbv2
ecs_container_not_directly_publicly_accessible_via_elbv2
ec2_instance_not_directly_publicly_accessible_via_elb
ec2_instance_not_directly_publicly_accessible_via_elbv2

To-Do:

Verify ALB/ELB and Instance/Lambda security group and ports
Include the target groups in the ALB/ELB object

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…-cloud#3237 Wrote checks for EC2, Lambda, and ECS to make sure they are not behind any public facing ALBs and ELBs in VPCs that have no security groups

puchy22

Hi @abant07 thanks for contributing with Prowler 🚀.

The main problem that I found in the PR is that you are not checking the Security Groups of the affected resources and ELBs and it could induce to false positives.

Please ensure that when you mark that a resource is public is because is actual public. For example, we can have a EC2 instance behind of public ELB but if the EC2 have a Security Group that is cutting down the public connections this instance in reality is not public. I know that is a stranger case but we have to consider it to reduce false positives.

I only write a review for the awslambda check but it is applicable for the other 3. For whatever doubt I am here to solve it.

puchy22 · 2024-05-21T10:34:31Z

...blicly_accessible_via_elbv2/awslambda_function_not_directly_publicly_accessible_via_elbv2.py

+                report.resource_arn = function.arn
+                report.resource_tags = function.tags
+                report.status = "PASS"
+                report.status_extended = f"Lambda function {function.name} is not behind an Internet facing Load Balancer."


Suggested change

report.status_extended = f"Lambda function {function.name} is not behind an Internet facing Load Balancer."

report.status_extended = f"Lambda function {function.name} is not publicly accesible through an Internet facing Load Balancer."

Make the same for the rest of the status_extended in other checks too. I think it is more clear.

puchy22 · 2024-05-21T10:38:12Z

...blicly_accessible_via_elbv2/awslambda_function_not_directly_publicly_accessible_via_elbv2.py

+    def execute(self):
+        findings = []
+
+        if awslambda_client.functions:


Suggested change

if awslambda_client.functions:

The if is not necessary here, if there is not functions it is not going to enter in the for loop and findings is gonna be empty

puchy22 · 2024-05-21T10:47:49Z

prowler/providers/aws/services/elb/elb_service.py

+                        for id in elb["Instances"]:
+                            instance_ids.append(id["InstanceId"])


Suggested change

for id in elb["Instances"]:

instance_ids.append(id["InstanceId"])

for instance_attached in elb["Instances"]:

instance_ids.append(instance_attached["InstanceId"])

puchy22 · 2024-05-21T10:50:05Z

prowler/providers/aws/services/elb/elb_service.py

@@ -45,6 +50,8 @@ def __describe_load_balancers__(self, regional_client):
                                region=regional_client.region,
                                scheme=elb["Scheme"],
                                listeners=listeners,
+                                security_groups=elb["SecurityGroups"],
+                                instances=instance_ids


Suggested change

instances=instance_ids

instances_ids=instance_ids

puchy22 · 2024-05-21T10:50:17Z

prowler/providers/aws/services/elb/elb_service.py

@@ -98,3 +105,5 @@ class LoadBalancer(BaseModel):
    access_logs: Optional[bool]
    listeners: list[Listener]
    tags: Optional[list] = []
+    security_groups: list[str]
+    instances: list[str]


Suggested change

instances: list[str]

instances_ids: list[str]

puchy22 · 2024-05-21T10:50:49Z

prowler/providers/aws/services/elbv2/elbv2_service.py

+                f"{regional_client.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}"
+            )
+
+    def __describe_target_groups__(self, regional_client):


This method is repeated.

puchy22 · 2024-05-21T10:51:29Z

prowler/providers/aws/services/elbv2/elbv2_service.py

+                    if (
+                        lb.scheme == "internet-facing"
+                        and lb.type == "application"
+                        and len(lb.security_groups) > 0
+                    ):


Why are you only getting in count public ELBs?

puchy22 · 2024-05-21T10:52:08Z

prowler/providers/aws/services/elbv2/elbv2_service.py

+                                            if lb.scheme == "internet-facing"
+                                            and lb.type == "application"
+                                            and len(lb.security_groups) > 0
+                                            else False


This is never going to be false due to first if of the for

puchy22 · 2024-05-21T10:53:07Z

prowler/providers/aws/services/elbv2/elbv2_service.py

+    target_type: str
+    target: str
+    public: bool
+    lbdns: Optional[str]


Why you need the DNS of the load balancer in each target group?

done. removed it

abant07 · 2024-05-21T15:29:30Z

Sure! Thanks for the feedback. Is there anything else that is incorrect that i need to fix?

Thanks

abant07 · 2024-05-22T00:00:02Z

Ok @puchy22

I believe I have corrected everything, let me know if there is something I missed.

abant07 · 2024-05-22T08:43:57Z

@puchy22 Is there a way I can run a linting script to fix the linting error?

puchy22 · 2024-05-22T09:40:46Z

Hi @abant07, thanks for your amazing work and for responding so quickly. Now I check the new changes introduced. For linters we recommend using pre-commit which is a tool that is configured for the repo.

Check our developer documentation to see how to install the project with everything you need to develop: https://docs.prowler.com/projects/prowler-open-source/en/latest/developer-guide/introduction/#contributing-with-your-code-or-fixes-to-prowler

abant07 · 2024-05-22T09:48:06Z

Yes @puchy22 I have linted the code, and it seems to be passing the checks better. thanks for the help

codecov · 2024-05-22T10:30:27Z

Codecov Report

Attention: Patch coverage is 87.19512% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 86.33%. Comparing base (eb7f56f) to head (258262c).
Report is 37 commits behind head on master.

❗ Current head 258262c differs from pull request most recent head b6a3f06

Please upload reports for the commit b6a3f06 to get more accurate results.

Files	Patch %	Lines
prowler/providers/aws/services/ecs/ecs_service.py	45.16%	17 Missing ⚠️
...wler/providers/aws/services/elbv2/elbv2_service.py	86.66%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3985      +/-   ##
==========================================
+ Coverage   86.27%   86.33%   +0.06%     
==========================================
  Files         790      787       -3     
  Lines       24729    24762      +33     
==========================================
+ Hits        21335    21379      +44     
+ Misses       3394     3383      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

puchy22

Hi @abant07, thanks for the effort reviewing my comments and improving the code. But the main problem persist, still giving false positives due to not checking the respective security group of each type of resource.

If you need any help to continue with the checks let me know.

Thank you for contributing wit Prowler and make cloud safer ☁️

puchy22 · 2024-05-22T11:07:42Z

prowler/providers/aws/services/ec2/ec2_service.py

@@ -489,6 +490,7 @@ class Instance(BaseModel):
    monitoring_state: str
    instance_profile: Optional[dict]
    tags: Optional[list] = []
+    security_groups: int


This is not necessary, security groups are added yet to the service.

I think we would need to have an additional attribute stating the security groups attached to an instance, because I think it will be easier to map which security groups are attached to which instances. So I will just make that attribute

security_groups: list - where this will store all the security group ids of a given instance

puchy22 · 2024-05-22T11:18:16Z

...blicly_accessible_via_elbv2/awslambda_function_not_directly_publicly_accessible_via_elbv2.py

+
+        public_lambda_functions = {}
+        for target_group in elbv2_client.target_groups:
+            if target_group.public and target_group.target_type == "lambda":


Should add a verification to ensure that the function has a policy that makes really public. Please see awslambda_function_not_publicly_accessible check to add the same verification before adding to public_lambda_function

puchy22 · 2024-05-22T11:19:17Z

...irectly_publicly_accessible_via_elb/ec2_instance_not_directly_publicly_accessible_via_elb.py

+                report.status = "PASS"
+                report.status_extended = f"EC2 Instance {instance.id} is not publicly accesible through an Internet facing Classic Load Balancer."
+
+                if instance.id in public_instances and instance.security_groups <= 1:


The security group must not be used in this way, here you are only checking that it exits but not if it has any inbound rule

puchy22 · 2024-05-22T11:23:08Z

...irectly_publicly_accessible_via_elb/ec2_instance_not_directly_publicly_accessible_via_elb.py

+        for lb in elb_client.loadbalancers:
+            if lb.public:
+                for instance in lb.instances_ids:
+                    public_instances[instance] = lb


Here you should add more verifications before adding an instance to public, first you should ensure that exists a security group attached to the instance that has some inbound rules that make the instance public.

Please review the ec2_securitygroup_allow_ingress_from_internet_to_all_ports and ec2_securitygroup_allow_ingress_from_internet_to_any_port to ensure that the instance is really open to Internet and avoid false positives

puchy22 · 2024-05-22T11:23:57Z

...tly_publicly_accessible_via_elbv2/ec2_instance_not_directly_publicly_accessible_via_elbv2.py

+from prowler.providers.aws.services.elbv2.elbv2_client import elbv2_client
+
+
+class ec2_instance_not_directly_publicly_accessible_via_elbv2(Check):


Apply the same review as elbv1 check

puchy22 · 2024-05-22T11:28:20Z

...ly_publicly_accessible_via_elbv2/ecs_container_not_directly_publicly_accessible_via_elbv2.py

+        for tg in elbv2_client.target_groups:
+            if tg.public and tg.target_type == "ip":
+                public_containers[tg.target] = tg.arn


You should check the firewall (Security Group) of the container before adding it to public containers to ensure that is really public through Internet. This is not added yet in the ECS service, so if you need help to add it please let me know and I can help you adding it to your PR as soon as possible.

Yeah sure, any help would be greatly appreciated

puchy22 · 2024-05-22T11:33:02Z

prowler/providers/aws/services/elbv2/elbv2_service.py

+                                    name=target_group["TargetGroupName"],
+                                    arn=target_group["TargetGroupArn"],
+                                    target_type=target_group["TargetType"],
+                                    target=target_health["Target"]["Id"],


It is a best practice to use get function in dictionaries instead of keys because if an key is not required it could make fail the creation of the object. Look at this doc to watch how to use it and define default values in case of fail.

Not sure if this is needed since no other files are using .get to access values from dictionaries from the boto3 api. I think it looks better if I just follow what others have done. But if you strongly insist I change it, I can do that.

abant07 · 2024-05-23T01:35:02Z

Hey @puchy22

I fixed all the edits from your feedback, the only thing that I am uncertain how to do and I might need assistance from you is checking the firewalls of ECS

Thanks

…t07/master

abant07 and others added 11 commits April 17, 2024 08:48

debugging lambda test for albs

0b55244

debugging awslambda test case for public alb check

860f1d0

chore: general fixes

006fbf7

fix: tests

63d0529

review PR

da6d821

solve comment

69c6d59

fix(elbv2): Add public attribute to TargetGroups

7c8727f

Improve publicly accessible checks to include targets of ELBs prowler…

25e2b70

…-cloud#3237 Wrote checks for EC2, Lambda, and ECS to make sure they are not behind any public facing ALBs and ELBs in VPCs that have no security groups

fixed tests

213053f

Merge branch 'prowler-cloud:master' into master

0c9747d

Merge branch 'new-public-exposed-checks' into master

150623b

abant07 requested review from a team as code owners May 13, 2024 17:06

github-actions bot added the provider/aws Issues/PRs related with the AWS provider label May 13, 2024

jfagoagas mentioned this pull request May 17, 2024

feat(internet-exposed): New public exposed checks #3801

Closed

2 tasks

jfagoagas added the no-merge Please, DO NOT MERGE this PR. label May 17, 2024

puchy22 suggested changes May 21, 2024

View reviewed changes

abant07 and others added 2 commits May 21, 2024 10:19

Merge branch 'prowler-cloud:master' into master

2810c95

fixed problems

79b4edf

fixes updated for EC2

7a785c2

abant07 and others added 3 commits May 22, 2024 02:18

Merge branch 'prowler-cloud:master' into master

51c2bf1

lint

f191535

lint agian

2d14623

abant07 added 2 commits May 22, 2024 02:43

update

39cf555

updated again

258262c

puchy22 suggested changes May 22, 2024

View reviewed changes

updated with fixes except for ECS

3e1ad05

abant07 and others added 16 commits May 22, 2024 23:39

linting update again

f0bbc1a

lint

7253a84

Update ecs_service.py

c70fade

fix(elbv2): Change target group to include multiple targets

5e8b9e0

chore(awslambda): Change check logic to not duplicate loops

5fa85bd

Merge branch 'prowler-cloud:master' into master

2f823e0

Merge branch 'master' of https://github.com/abant07/prowler into aban…

214d113

…t07/master

chore(awslambda): Add security group comprobation for the elb

c57df43

chore(awslambda): Improve status extended to not repeat

be58644

chore(awslabmda): Add support for many tgs in an action

1e4eec1

chore(awslambda): Checks all SGs before add report

7b108e6

feat(ecs): Add service support

7051ec5

chore(alb): Fix some specific false positives

af4e923

chore(ecs): Change check logic to avoid false positives

7053dc4

feat(elb): Add port and protocol to listeners

799dd43

chore(ec2): Change check logic to avoid false positives and repeat loops

b6a3f06

	report.status_extended = f"Lambda function {function.name} is not behind an Internet facing Load Balancer."
	report.status_extended = f"Lambda function {function.name} is not publicly accesible through an Internet facing Load Balancer."

		for id in elb["Instances"]:
		instance_ids.append(id["InstanceId"])

		from prowler.providers.aws.services.elbv2.elbv2_client import elbv2_client


		class ec2_instance_not_directly_publicly_accessible_via_elbv2(Check):

feat(internet-exposed): Improve publicly accessible checks to include targets of ELBs #3985

Are you sure you want to change the base?

feat(internet-exposed): Improve publicly accessible checks to include targets of ELBs #3985

Conversation

abant07 commented May 13, 2024 • edited

Context

Description

License

puchy22 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abant07 commented May 21, 2024

abant07 commented May 22, 2024

abant07 commented May 22, 2024

puchy22 commented May 22, 2024

abant07 commented May 22, 2024

codecov bot commented May 22, 2024 • edited

Codecov Report

puchy22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abant07 commented May 23, 2024

abant07 commented May 13, 2024 •

edited

puchy22 left a comment •

edited

codecov bot commented May 22, 2024 •

edited