From b54ecb50bfe4ce60bc8afaaa09c63afc0decd9c0 Mon Sep 17 00:00:00 2001 From: Sergio Garcia <38561120+sergargar@users.noreply.github.com> Date: Tue, 30 Apr 2024 13:08:05 +0200 Subject: [PATCH] fix(efs): check all public conditions (#3872) --- .../efs_not_publicly_accessible.py | 28 ++-- .../aws/services/efs/lib/__init__.py | 0 prowler/providers/aws/services/efs/lib/lib.py | 45 ++++++ .../efs_not_publicly_accessible_test.py | 135 ++++++++++++++++-- .../aws/services/efs/lib/lib_test.py | 102 +++++++++++++ 5 files changed, 282 insertions(+), 28 deletions(-) create mode 100644 prowler/providers/aws/services/efs/lib/__init__.py create mode 100644 prowler/providers/aws/services/efs/lib/lib.py create mode 100644 tests/providers/aws/services/efs/lib/lib_test.py diff --git a/prowler/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible.py b/prowler/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible.py index 8cb33dfa41..cf8229c2d2 100644 --- a/prowler/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible.py +++ b/prowler/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible.py @@ -1,5 +1,6 @@ from prowler.lib.check.models import Check, Check_Report_AWS from prowler.providers.aws.services.efs.efs_client import efs_client +from prowler.providers.aws.services.efs.lib.lib import is_public_access_allowed class efs_not_publicly_accessible(Check): @@ -19,23 +20,14 @@ class efs_not_publicly_accessible(Check): report.status = "FAIL" report.status_extended = f"EFS {fs.id} doesn't have any policy which means it grants full access to any client." else: - for statement in fs.policy["Statement"]: - if statement["Effect"] == "Allow": - if ( - ("Principal" in statement and statement["Principal"] == "*") - or ( - "Principal" in statement - and "AWS" in statement["Principal"] - and statement["Principal"]["AWS"] == "*" - ) - or ( - "CanonicalUser" in statement["Principal"] - and statement["Principal"]["CanonicalUser"] == "*" - ) - ): - report.status = "FAIL" - report.status_extended = f"EFS {fs.id} has a policy which allows access to everyone." - break + for statement in fs.policy.get("Statement", []): + if statement.get("Effect") == "Allow" and is_public_access_allowed( + statement + ): + report.status = "FAIL" + report.status_extended = ( + f"EFS {fs.id} has a policy which allows access to everyone." + ) + break findings.append(report) - return findings diff --git a/prowler/providers/aws/services/efs/lib/__init__.py b/prowler/providers/aws/services/efs/lib/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/prowler/providers/aws/services/efs/lib/lib.py b/prowler/providers/aws/services/efs/lib/lib.py new file mode 100644 index 0000000000..2d310fc369 --- /dev/null +++ b/prowler/providers/aws/services/efs/lib/lib.py @@ -0,0 +1,45 @@ +def is_public_access_allowed(statement): + """ + Check if the statement allows public access + Args: + statement: dict: Statement from the policy + Returns: + bool: True if the statement allows public access, False otherwise + """ + principal = statement.get("Principal") + if principal == "*" or (isinstance(principal, dict) and "*" in principal.values()): + return not has_secure_conditions(statement) + return False + + +def has_secure_conditions(statement): + """ + Check if the statement has secure conditions + Args: + statement: dict: Statement from the policy + Returns: + bool: True if the statement has secure conditions, False otherwise + """ + conditions = statement.get("Condition", {}) + allowed_conditions = { + "aws:SourceArn", + "aws:SourceVpc", + "aws:SourceVpce", + "aws:SourceOwner", + "aws:SourceAccount", + } + if ( + "Bool" in conditions + and conditions["Bool"].get("elasticfilesystem:AccessedViaMountTarget") == "true" + ): + return True + + # Check for conditions with nested keys + for _, conditions_dict in conditions.items(): + for key, value in conditions_dict.items(): + if isinstance(value, dict): + if set(value.keys()).intersection(allowed_conditions): + return True + elif key in allowed_conditions: + return True + return False diff --git a/tests/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible_test.py b/tests/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible_test.py index 6722777949..25891fe123 100644 --- a/tests/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible_test.py +++ b/tests/providers/aws/services/efs/efs_not_publicly_accessible/efs_not_publicly_accessible_test.py @@ -1,4 +1,3 @@ -from re import search from unittest import mock from prowler.providers.aws.services.efs.efs_service import FileSystem @@ -32,6 +31,48 @@ filesystem_invalid_policy = { ], } +# https://docs.aws.amazon.com/efs/latest/ug/access-control-block-public-access.html#what-is-a-public-policy +filesystem_policy_with_source_arn_condition = { + "Version": "2012-10-17", + "Id": "efs-policy-wizard-15ad9567-2546-4bbb-8168-5541b6fc0e55", + "Statement": [ + { + "Sid": "efs-statement-14a7191c-9401-40e7-a388-6af6cfb7dd9c", + "Effect": "Allow", + "Principal": {"AWS": "*"}, + "Action": [ + "elasticfilesystem:ClientMount", + "elasticfilesystem:ClientWrite", + "elasticfilesystem:ClientRootAccess", + ], + "Condition": { + "ArnEquals": { + "aws:SourceArn": f"arn:aws:iam::{AWS_ACCOUNT_NUMBER}:root" + } + }, + } + ], +} + +# https://docs.aws.amazon.com/efs/latest/ug/access-control-block-public-access.html#what-is-a-public-policy +filesystem_policy_with_mount_target_condition = { + "Version": "2012-10-17", + "Id": "efs-policy-wizard-15ad9567-2546-4bbb-8168-5541b6fc0e55", + "Statement": [ + { + "Sid": "efs-statement-14a7191c-9401-40e7-a388-6af6cfb7dd9c", + "Effect": "Allow", + "Principal": {"AWS": "*"}, + "Action": [ + "elasticfilesystem:ClientMount", + "elasticfilesystem:ClientWrite", + "elasticfilesystem:ClientRootAccess", + ], + "Condition": {"Bool": {"elasticfilesystem:AccessedViaMountTarget": "true"}}, + } + ], +} + class Test_efs_not_publicly_accessible: def test_efs_valid_policy(self): @@ -59,12 +100,82 @@ class Test_efs_not_publicly_accessible: result = check.execute() assert len(result) == 1 assert result[0].status == "PASS" - assert search( - "has a policy which does not allow access to everyone", - result[0].status_extended, + assert ( + result[0].status_extended + == f"EFS {file_system_id} has a policy which does not allow access to everyone." ) assert result[0].resource_id == file_system_id assert result[0].resource_arn == efs_arn + assert result[0].region == AWS_REGION + assert result[0].resource_tags == [] + + def test_efs_valid_policy_with_mount_target_condition(self): + efs_client = mock.MagicMock + efs_arn = f"arn:aws:elasticfilesystem:{AWS_REGION}:{AWS_ACCOUNT_NUMBER}:file-system/{file_system_id}" + efs_client.filesystems = [ + FileSystem( + id=file_system_id, + arn=efs_arn, + region=AWS_REGION, + policy=filesystem_policy_with_mount_target_condition, + backup_policy=None, + encrypted=True, + ) + ] + with mock.patch( + "prowler.providers.aws.services.efs.efs_service.EFS", + efs_client, + ): + from prowler.providers.aws.services.efs.efs_not_publicly_accessible.efs_not_publicly_accessible import ( + efs_not_publicly_accessible, + ) + + check = efs_not_publicly_accessible() + result = check.execute() + assert len(result) == 1 + assert result[0].status == "PASS" + assert ( + result[0].status_extended + == f"EFS {file_system_id} has a policy which does not allow access to everyone." + ) + assert result[0].resource_id == file_system_id + assert result[0].resource_arn == efs_arn + assert result[0].region == AWS_REGION + assert result[0].resource_tags == [] + + def test_efs_valid_policy_with_source_arn_condition(self): + efs_client = mock.MagicMock + efs_arn = f"arn:aws:elasticfilesystem:{AWS_REGION}:{AWS_ACCOUNT_NUMBER}:file-system/{file_system_id}" + efs_client.filesystems = [ + FileSystem( + id=file_system_id, + arn=efs_arn, + region=AWS_REGION, + policy=filesystem_policy_with_source_arn_condition, + backup_policy=None, + encrypted=True, + ) + ] + with mock.patch( + "prowler.providers.aws.services.efs.efs_service.EFS", + efs_client, + ): + from prowler.providers.aws.services.efs.efs_not_publicly_accessible.efs_not_publicly_accessible import ( + efs_not_publicly_accessible, + ) + + check = efs_not_publicly_accessible() + result = check.execute() + assert len(result) == 1 + assert result[0].status == "PASS" + assert ( + result[0].status_extended + == f"EFS {file_system_id} has a policy which does not allow access to everyone." + ) + assert result[0].resource_id == file_system_id + assert result[0].resource_arn == efs_arn + assert result[0].region == AWS_REGION + assert result[0].resource_tags == [] def test_efs_invalid_policy(self): efs_client = mock.MagicMock @@ -92,12 +203,14 @@ class Test_efs_not_publicly_accessible: result = check.execute() assert len(result) == 1 assert result[0].status == "FAIL" - assert search( - "has a policy which allows access to everyone", - result[0].status_extended, + assert ( + result[0].status_extended + == f"EFS {file_system_id} has a policy which allows access to everyone." ) assert result[0].resource_id == file_system_id assert result[0].resource_arn == efs_arn + assert result[0].region == AWS_REGION + assert result[0].resource_tags == [] def test_efs_no_policy(self): efs_client = mock.MagicMock @@ -124,9 +237,11 @@ class Test_efs_not_publicly_accessible: result = check.execute() assert len(result) == 1 assert result[0].status == "FAIL" - assert search( - "doesn't have any policy which means it grants full access to any client", - result[0].status_extended, + assert ( + result[0].status_extended + == f"EFS {file_system_id} doesn't have any policy which means it grants full access to any client." ) assert result[0].resource_id == file_system_id assert result[0].resource_arn == efs_arn + assert result[0].region == AWS_REGION + assert result[0].resource_tags == [] diff --git a/tests/providers/aws/services/efs/lib/lib_test.py b/tests/providers/aws/services/efs/lib/lib_test.py new file mode 100644 index 0000000000..dc486ba914 --- /dev/null +++ b/tests/providers/aws/services/efs/lib/lib_test.py @@ -0,0 +1,102 @@ +from prowler.providers.aws.services.efs.lib.lib import is_public_access_allowed + + +class Test_EFS_lib: + def test__is_public_access_allowed__(self): + statement = { + "Effect": "Allow", + "Principal": "*", + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + } + assert is_public_access_allowed(statement) + + def test__is_public_access_allowed__with_principal_dict(self): + statement = { + "Effect": "Allow", + "Principal": {"AWS": "*"}, + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + } + assert is_public_access_allowed(statement) + + def test__is_public_access_allowed__with_secure_conditions(self): + statement = { + "Effect": "Allow", + "Principal": "*", + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + "Condition": {"Bool": {"elasticfilesystem:AccessedViaMountTarget": "true"}}, + } + assert not is_public_access_allowed(statement) + + def test__is_public_access_allowed__with_secure_conditions_and_allowed_conditions( + self, + ): + statement = { + "Effect": "Allow", + "Principal": "*", + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + "Condition": { + "Bool": {"elasticfilesystem:AccessedViaMountTarget": "true"}, + "StringEquals": {"aws:SourceOwner": "123456789012"}, + }, + } + assert not is_public_access_allowed(statement) + + def test__is_public_access_allowed__with_secure_conditions_and_allowed_conditions_nested( + self, + ): + statement = { + "Effect": "Allow", + "Principal": "*", + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + "Condition": { + "Bool": {"elasticfilesystem:AccessedViaMountTarget": "true"}, + "StringEquals": {"aws:SourceOwner": "123456789012"}, + "StringEqualsIfExists": {"aws:SourceVpce": "vpce-1234567890abcdef0"}, + }, + } + assert not is_public_access_allowed(statement) + + def test__is_public_access_allowed__with_secure_conditions_and_allowed_conditions_nested_dict( + self, + ): + statement = { + "Effect": "Allow", + "Principal": "*", + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + "Condition": { + "Bool": {"elasticfilesystem:AccessedViaMountTarget": "true"}, + "StringEquals": {"aws:SourceOwner": "123456789012"}, + "StringEqualsIfExists": { + "aws:SourceVpce": { + "vpce-1234567890abcdef0": "vpce-1234567890abcdef0" + } + }, + }, + } + assert not is_public_access_allowed(statement) + + def test__is_public_access_allowed__with_secure_conditions_and_allowed_conditions_nested_dict_key( + self, + ): + statement = { + "Effect": "Allow", + "Principal": "*", + "Action": "elasticfilesystem:ClientMount", + "Resource": "*", + "Condition": { + "Bool": {"elasticfilesystem:AccessedViaMountTarget": "true"}, + "StringEquals": {"aws:SourceOwner": "123456789012"}, + "StringEqualsIfExists": { + "aws:SourceVpce": { + "vpce-1234567890abcdef0": "vpce-1234567890abcdef0" + } + }, + }, + } + assert not is_public_access_allowed(statement)