-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(internet-exposed): Improve publicly accessible checks to include targets of ELBs #3985
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the same for the rest of the status_extended in other checks too. I think it is more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
def execute(self): | ||
findings = [] | ||
|
||
if awslambda_client.functions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if awslambda_client.functions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
for id in elb["Instances"]: | ||
instance_ids.append(id["InstanceId"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for id in elb["Instances"]: | |
instance_ids.append(id["InstanceId"]) | |
for instance_attached in elb["Instances"]: | |
instance_ids.append(instance_attached["InstanceId"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instances=instance_ids | |
instances_ids=instance_ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -98,3 +105,5 @@ class LoadBalancer(BaseModel): | |||
access_logs: Optional[bool] | |||
listeners: list[Listener] | |||
tags: Optional[list] = [] | |||
security_groups: list[str] | |||
instances: list[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instances: list[str] | |
instances_ids: list[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
f"{regional_client.region} -- {error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}" | ||
) | ||
|
||
def __describe_target_groups__(self, regional_client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is repeated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ffixed
if ( | ||
lb.scheme == "internet-facing" | ||
and lb.type == "application" | ||
and len(lb.security_groups) > 0 | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you only getting in count public ELBs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if lb.scheme == "internet-facing" | ||
and lb.type == "application" | ||
and len(lb.security_groups) > 0 | ||
else False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never going to be false due to first if of the for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
target_type: str | ||
target: str | ||
public: bool | ||
lbdns: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you need the DNS of the load balancer in each target group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. removed it
Sure! Thanks for the feedback. Is there anything else that is incorrect that i need to fix? Thanks |
Ok @puchy22 I believe I have corrected everything, let me know if there is something I missed. |
@puchy22 Is there a way I can run a linting script to fix the linting error? |
Hi @abant07, thanks for your amazing work and for responding so quickly. Now I check the new changes introduced. For linters we recommend using 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 |
Yes @puchy22 I have linted the code, and it seems to be passing the checks better. thanks for the help |
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ☁️
@@ -489,6 +490,7 @@ class Instance(BaseModel): | |||
monitoring_state: str | |||
instance_profile: Optional[dict] | |||
tags: Optional[list] = [] | |||
security_groups: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, security groups are added yet to the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
public_lambda_functions = {} | ||
for target_group in elbv2_client.target_groups: | ||
if target_group.public and target_group.target_type == "lambda": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for lb in elb_client.loadbalancers: | ||
if lb.public: | ||
for instance in lb.instances_ids: | ||
public_instances[instance] = lb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
from prowler.providers.aws.services.elbv2.elbv2_client import elbv2_client | ||
|
||
|
||
class ec2_instance_not_directly_publicly_accessible_via_elbv2(Check): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply the same review as elbv1 check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for tg in elbv2_client.target_groups: | ||
if tg.public and tg.target_type == "ip": | ||
public_containers[tg.target] = tg.arn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure, any help would be greatly appreciated
name=target_group["TargetGroupName"], | ||
arn=target_group["TargetGroupArn"], | ||
target_type=target_group["TargetType"], | ||
target=target_health["Target"]["Id"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
After much internal testing, we have found that these checks are very sensitive to false positives/negatives. Therefore, we will use all this PR work to improve the existing public exposed checks. The checks that are gonna be modified to improve this are:
Anyway, thanks for all your work, it will help us to improve our checks in the near future. Thank you for your help with Prowler, we will let you know when these new features are available. 🚀 🚀 |
Context
Fixes #3237
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:
To-Do:
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.