diff --git a/.github/workflows/api-container-build-push.yml b/.github/workflows/api-container-build-push.yml index 69edd64e64..e013329e32 100644 --- a/.github/workflows/api-container-build-push.yml +++ b/.github/workflows/api-container-build-push.yml @@ -99,6 +99,12 @@ jobs: with: persist-credentials: false + - name: Pin prowler SDK to latest master commit + if: github.event_name == 'push' + run: | + LATEST_SHA=$(git ls-remote https://github.com/prowler-cloud/prowler.git refs/heads/master | cut -f1) + sed -i "s|prowler-cloud/prowler.git@master|prowler-cloud/prowler.git@${LATEST_SHA}|" api/pyproject.toml + - name: Login to DockerHub uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3.7.0 with: diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index 7cd4fce52f..17acf2155a 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -2,11 +2,28 @@ All notable changes to the **Prowler API** are documented in this file. +## [1.23.0] (Prowler UNRELEASED) + +### 🔐 Security + +- Replace stdlib XML parser with `defusedxml` in SAML metadata parsing to prevent XML bomb (billion laughs) DoS attacks [(#10165)](https://github.com/prowler-cloud/prowler/pull/10165) + +--- + +## [1.22.1] (Prowler v5.21.1) + +### 🐞 Fixed + +- Threat score aggregation query to eliminate unnecessary JOINs and `COUNT(DISTINCT)` overhead [(#10394)](https://github.com/prowler-cloud/prowler/pull/10394) + +--- + ## [1.22.0] (Prowler v5.21.0) ### 🚀 Added - `CORS_ALLOWED_ORIGINS` configurable via environment variable [(#10355)](https://github.com/prowler-cloud/prowler/pull/10355) +- Finding groups support `check_title` substring filtering [(#10377)](https://github.com/prowler-cloud/prowler/pull/10377) - Attack Paths: Tenant and provider related labels to the nodes so they can be easily filtered on custom queries [(#10308)](https://github.com/prowler-cloud/prowler/pull/10308) ### 🔄 Changed @@ -21,6 +38,7 @@ All notable changes to the **Prowler API** are documented in this file. ### 🔐 Security - Use `psycopg2.sql` to safely compose DDL in `PostgresEnumMigration`, preventing SQL injection via f-string interpolation [(#10166)](https://github.com/prowler-cloud/prowler/pull/10166) +- Replace stdlib XML parser with `defusedxml` in SAML metadata parsing to prevent XML bomb (billion laughs) DoS attacks [(#10165)](https://github.com/prowler-cloud/prowler/pull/10165) --- diff --git a/api/poetry.lock b/api/poetry.lock index 6fbdb874bc..0b0da9c853 100644 --- a/api/poetry.lock +++ b/api/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 2.1.4 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.3.2 and should not be changed by hand. [[package]] name = "about-time" @@ -2699,6 +2699,18 @@ files = [ {file = "decorator-5.2.1.tar.gz", hash = "sha256:65f266143752f734b0a7cc83c46f4618af75b8c5911b00ccb61d0ac9b6da0360"}, ] +[[package]] +name = "defusedxml" +version = "0.7.1" +description = "XML bomb protection for Python stdlib modules" +optional = false +python-versions = ">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*, !=3.4.*" +groups = ["main"] +files = [ + {file = "defusedxml-0.7.1-py2.py3-none-any.whl", hash = "sha256:a352e7e428770286cc899e2542b6cdaedb2b4953ff269a210103ec58f6198a61"}, + {file = "defusedxml-0.7.1.tar.gz", hash = "sha256:1bb3032db185915b62d7c6209c5a8792be6a32ab2fedacc84e01b52c51aa3e69"}, +] + [[package]] name = "deprecated" version = "1.3.1" @@ -2971,7 +2983,7 @@ files = [ [package.dependencies] autopep8 = "*" Django = ">=4.2" -gprof2dot = ">=2017.09.19" +gprof2dot = ">=2017.9.19" sqlparse = "*" [[package]] @@ -4579,7 +4591,7 @@ files = [ [package.dependencies] attrs = ">=22.2.0" -jsonschema-specifications = ">=2023.03.6" +jsonschema-specifications = ">=2023.3.6" referencing = ">=0.28.4" rpds-py = ">=0.7.1" @@ -4787,7 +4799,7 @@ librabbitmq = ["librabbitmq (>=2.0.0) ; python_version < \"3.11\""] mongodb = ["pymongo (==4.15.3)"] msgpack = ["msgpack (==1.1.2)"] pyro = ["pyro4 (==4.82)"] -qpid = ["qpid-python (==1.36.0-1)", "qpid-tools (==1.36.0-1)"] +qpid = ["qpid-python (==1.36.0.post1)", "qpid-tools (==1.36.0.post1)"] redis = ["redis (>=4.5.2,!=4.5.5,!=5.0.2,<6.5)"] slmq = ["softlayer_messaging (>=1.0.3)"] sqlalchemy = ["sqlalchemy (>=1.4.48,<2.1)"] @@ -4808,7 +4820,7 @@ files = [ ] [package.dependencies] -certifi = ">=14.05.14" +certifi = ">=14.5.14" durationpy = ">=0.7" google-auth = ">=1.0.1" oauthlib = ">=3.2.2" @@ -7170,7 +7182,7 @@ files = [ ] [package.dependencies] -astroid = ">=3.2.2,<=3.3.0-dev0" +astroid = ">=3.2.2,<=3.3.0.dev0" colorama = {version = ">=0.4.5", markers = "sys_platform == \"win32\""} dill = [ {version = ">=0.3.7", markers = "python_version >= \"3.12\""}, @@ -8184,10 +8196,10 @@ files = [ ] [package.dependencies] -botocore = ">=1.37.4,<2.0a.0" +botocore = ">=1.37.4,<2.0a0" [package.extras] -crt = ["botocore[crt] (>=1.37.4,<2.0a.0)"] +crt = ["botocore[crt] (>=1.37.4,<2.0a0)"] [[package]] name = "safety" @@ -9382,4 +9394,4 @@ files = [ [metadata] lock-version = "2.1" python-versions = ">=3.11,<3.13" -content-hash = "6e38c38b1f8dc05b881f49703fa445eec299527e6697992b18e4613534fbcdb6" +content-hash = "2ed5b4e47d81da81963814f21702220ac5619f50cd605fd779be53c8c46ffca5" diff --git a/api/pyproject.toml b/api/pyproject.toml index bc9c742a6c..3a89f52e05 100644 --- a/api/pyproject.toml +++ b/api/pyproject.toml @@ -22,6 +22,7 @@ dependencies = [ "drf-nested-routers (>=0.94.1,<1.0.0)", "drf-spectacular==0.27.2", "drf-spectacular-jsonapi==0.5.1", + "defusedxml==0.7.1", "gunicorn==23.0.0", "lxml==5.3.2", "prowler @ git+https://github.com/prowler-cloud/prowler.git@master", diff --git a/api/src/backend/api/filters.py b/api/src/backend/api/filters.py index a64cc0ea13..d8b803d475 100644 --- a/api/src/backend/api/filters.py +++ b/api/src/backend/api/filters.py @@ -926,6 +926,9 @@ class FindingGroupSummaryFilter(FilterSet): check_id = CharFilter(field_name="check_id", lookup_expr="exact") check_id__in = CharInFilter(field_name="check_id", lookup_expr="in") check_id__icontains = CharFilter(field_name="check_id", lookup_expr="icontains") + check_title__icontains = CharFilter( + field_name="check_title", lookup_expr="icontains" + ) # Provider filters provider_id = UUIDFilter(field_name="provider_id", lookup_expr="exact") @@ -1025,6 +1028,9 @@ class LatestFindingGroupSummaryFilter(FilterSet): check_id = CharFilter(field_name="check_id", lookup_expr="exact") check_id__in = CharInFilter(field_name="check_id", lookup_expr="in") check_id__icontains = CharFilter(field_name="check_id", lookup_expr="icontains") + check_title__icontains = CharFilter( + field_name="check_title", lookup_expr="icontains" + ) # Provider filters provider_id = UUIDFilter(field_name="provider_id", lookup_expr="exact") diff --git a/api/src/backend/api/models.py b/api/src/backend/api/models.py index 7b7ccb9061..78bfec5262 100644 --- a/api/src/backend/api/models.py +++ b/api/src/backend/api/models.py @@ -1,7 +1,6 @@ import json import logging import re -import xml.etree.ElementTree as ET from datetime import datetime, timedelta, timezone from uuid import UUID, uuid4 @@ -9,6 +8,8 @@ from allauth.socialaccount.models import SocialApp from config.custom_logging import BackendLogger from config.settings.social_login import SOCIALACCOUNT_PROVIDERS from cryptography.fernet import Fernet, InvalidToken +import defusedxml +from defusedxml import ElementTree as ET from django.conf import settings from django.contrib.auth.models import AbstractBaseUser from django.contrib.postgres.fields import ArrayField @@ -2067,6 +2068,8 @@ class SAMLConfiguration(RowLevelSecurityProtectedModel): root = ET.fromstring(self.metadata_xml) except ET.ParseError as e: raise ValidationError({"metadata_xml": f"Invalid XML: {e}"}) + except defusedxml.DefusedXmlException as e: + raise ValidationError({"metadata_xml": f"Unsafe XML content rejected: {e}"}) # Entity ID entity_id = root.attrib.get("entityID") diff --git a/api/src/backend/api/tests/integration/test_authentication.py b/api/src/backend/api/tests/integration/test_authentication.py index 2abf725124..26d5b8e108 100644 --- a/api/src/backend/api/tests/integration/test_authentication.py +++ b/api/src/backend/api/tests/integration/test_authentication.py @@ -301,7 +301,7 @@ class TestTokenSwitchTenant: assert invalid_tenant_response.status_code == 400 assert invalid_tenant_response.json()["errors"][0]["code"] == "invalid" assert invalid_tenant_response.json()["errors"][0]["detail"] == ( - "Tenant does not exist or user is not a " "member." + "Tenant does not exist or user is not a member." ) @@ -912,10 +912,9 @@ class TestAPIKeyLifecycle: auth_response = client.get(reverse("provider-list"), headers=api_key_headers) # Must return 401 Unauthorized, not 500 Internal Server Error - assert auth_response.status_code == 401, ( - f"Expected 401 but got {auth_response.status_code}: " - f"{auth_response.json()}" - ) + assert ( + auth_response.status_code == 401 + ), f"Expected 401 but got {auth_response.status_code}: {auth_response.json()}" # Verify error message is present response_json = auth_response.json() diff --git a/api/src/backend/api/tests/test_apps.py b/api/src/backend/api/tests/test_apps.py index 712bc33882..5889b4e2cb 100644 --- a/api/src/backend/api/tests/test_apps.py +++ b/api/src/backend/api/tests/test_apps.py @@ -10,11 +10,11 @@ from django.conf import settings import api import api.apps as api_apps_module from api.apps import ( - ApiConfig, PRIVATE_KEY_FILE, PUBLIC_KEY_FILE, SIGNING_KEY_ENV, VERIFYING_KEY_ENV, + ApiConfig, ) @@ -187,9 +187,10 @@ def test_ready_initializes_driver_for_api_process(monkeypatch): _set_argv(monkeypatch, ["gunicorn"]) _set_testing(monkeypatch, False) - with patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), patch( - "api.attack_paths.database.init_driver" - ) as init_driver: + with ( + patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), + patch("api.attack_paths.database.init_driver") as init_driver, + ): config.ready() init_driver.assert_called_once() @@ -200,9 +201,10 @@ def test_ready_skips_driver_for_celery(monkeypatch): _set_argv(monkeypatch, ["celery", "-A", "api"]) _set_testing(monkeypatch, False) - with patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), patch( - "api.attack_paths.database.init_driver" - ) as init_driver: + with ( + patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), + patch("api.attack_paths.database.init_driver") as init_driver, + ): config.ready() init_driver.assert_not_called() @@ -213,9 +215,10 @@ def test_ready_skips_driver_for_manage_py_skip_command(monkeypatch): _set_argv(monkeypatch, ["manage.py", "migrate"]) _set_testing(monkeypatch, False) - with patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), patch( - "api.attack_paths.database.init_driver" - ) as init_driver: + with ( + patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), + patch("api.attack_paths.database.init_driver") as init_driver, + ): config.ready() init_driver.assert_not_called() @@ -226,9 +229,10 @@ def test_ready_skips_driver_when_testing(monkeypatch): _set_argv(monkeypatch, ["gunicorn"]) _set_testing(monkeypatch, True) - with patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), patch( - "api.attack_paths.database.init_driver" - ) as init_driver: + with ( + patch.object(ApiConfig, "_ensure_crypto_keys", return_value=None), + patch("api.attack_paths.database.init_driver") as init_driver, + ): config.ready() init_driver.assert_not_called() diff --git a/api/src/backend/api/tests/test_models.py b/api/src/backend/api/tests/test_models.py index 13878794b1..b8b7f61dd1 100644 --- a/api/src/backend/api/tests/test_models.py +++ b/api/src/backend/api/tests/test_models.py @@ -243,6 +243,39 @@ class TestSAMLConfigurationModel: assert "Invalid XML" in errors["metadata_xml"][0] assert "not well-formed" in errors["metadata_xml"][0] + def test_xml_bomb_rejected(self, tenants_fixture): + """ + Regression test: a 'billion laughs' XML bomb in the SAML metadata field + must be rejected and not allowed to exhaust server memory / CPU. + + Before the fix, xml.etree.ElementTree was used directly, which does not + protect against entity-expansion attacks. The fix switches to defusedxml + which raises an exception for any XML containing entity definitions. + """ + tenant = tenants_fixture[0] + xml_bomb = ( + "" + "" + " " + " " + " " + "]>" + "" + ) + config = SAMLConfiguration( + email_domain="xmlbomb.com", + metadata_xml=xml_bomb, + tenant=tenant, + ) + + with pytest.raises(ValidationError) as exc_info: + config._parse_metadata() + + errors = exc_info.value.message_dict + assert "metadata_xml" in errors + def test_metadata_missing_sso_fails(self, tenants_fixture): tenant = tenants_fixture[0] xml = """ diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index d857b2bc07..7dd14bf420 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -15526,6 +15526,22 @@ class TestFindingGroupViewSet: assert len(response.json()["data"]) == 1 assert "bucket" in response.json()["data"][0]["id"].lower() + def test_finding_groups_check_title_icontains( + self, authenticated_client, finding_groups_fixture + ): + """Test searching check titles with icontains.""" + response = authenticated_client.get( + reverse("finding-group-list"), + { + "filter[inserted_at]": TODAY, + "filter[check_title.icontains]": "public access", + }, + ) + assert response.status_code == status.HTTP_200_OK + data = response.json()["data"] + assert len(data) == 1 + assert data[0]["id"] == "s3_bucket_public_access" + def test_resources_not_found(self, authenticated_client): """Test 404 returned for nonexistent check_id.""" response = authenticated_client.get( diff --git a/api/src/backend/api/v1/views.py b/api/src/backend/api/v1/views.py index 7878b51351..7c2de4a41c 100644 --- a/api/src/backend/api/v1/views.py +++ b/api/src/backend/api/v1/views.py @@ -4,7 +4,6 @@ import json import logging import os import time - from collections import defaultdict from copy import deepcopy from datetime import datetime, timedelta, timezone @@ -12,7 +11,6 @@ from decimal import ROUND_HALF_UP, Decimal, InvalidOperation from urllib.parse import urljoin import sentry_sdk - from allauth.socialaccount.models import SocialAccount, SocialApp from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter from allauth.socialaccount.providers.google.views import GoogleOAuth2Adapter @@ -76,6 +74,7 @@ from rest_framework.exceptions import ( ) from rest_framework.generics import GenericAPIView, get_object_or_404 from rest_framework.permissions import SAFE_METHODS +from rest_framework_json_api import filters as jsonapi_filters from rest_framework_json_api.views import RelationshipView, Response from rest_framework_simplejwt.exceptions import InvalidToken, TokenError from tasks.beat import schedule_provider_scan @@ -100,7 +99,6 @@ from api.attack_paths import database as graph_database from api.attack_paths import get_queries_for_provider, get_query_by_id from api.attack_paths import views_helpers as attack_paths_views_helpers from api.base_views import BaseRLSViewSet, BaseTenantViewset, BaseUserViewset -from api.renderers import APIJSONRenderer, PlainTextRenderer from api.compliance import ( PROWLER_COMPLIANCE_OVERVIEW_TEMPLATE, get_compliance_frameworks, @@ -199,6 +197,7 @@ from api.models import ( ) from api.pagination import ComplianceOverviewPagination from api.rbac.permissions import Permissions, get_providers, get_role +from api.renderers import APIJSONRenderer, PlainTextRenderer from api.rls import Tenant from api.utils import ( CustomOAuth2Client, @@ -6777,13 +6776,29 @@ class FindingGroupViewSet(BaseRLSViewSet): queryset = FindingGroupDailySummary.objects.all() serializer_class = FindingGroupSerializer filterset_class = FindingGroupSummaryFilter + filter_backends = [ + jsonapi_filters.QueryParameterValidationFilter, + jsonapi_filters.OrderingFilter, + CustomDjangoFilterBackend, + ] http_method_names = ["get"] required_permissions = [] def get_filterset_class(self): - """Return appropriate filter based on action.""" + """Return the filterset class used for schema generation and the list action. + + Note: The resources and latest_resources actions do not use this method + at runtime. They manually instantiate FindingGroupFilter / + LatestFindingGroupFilter against a Finding queryset (see + _get_finding_queryset). The class returned here for those actions only + affects the OpenAPI schema generated by drf-spectacular. + """ if self.action == "latest": return LatestFindingGroupSummaryFilter + if self.action == "resources": + return FindingGroupFilter + if self.action == "latest_resources": + return LatestFindingGroupFilter return FindingGroupSummaryFilter def get_queryset(self): @@ -7237,6 +7252,7 @@ class FindingGroupViewSet(BaseRLSViewSet): and timing information including how long they have been failing. """, tags=["Finding Groups"], + filters=True, ) @action(detail=True, methods=["get"], url_path="resources") def resources(self, request, pk=None): @@ -7311,6 +7327,7 @@ class FindingGroupViewSet(BaseRLSViewSet): and timing information. No date filters required. """, tags=["Finding Groups"], + filters=True, ) @action( detail=False, diff --git a/api/src/backend/tasks/jobs/threatscore_utils.py b/api/src/backend/tasks/jobs/threatscore_utils.py index a17125af46..2ef29484ee 100644 --- a/api/src/backend/tasks/jobs/threatscore_utils.py +++ b/api/src/backend/tasks/jobs/threatscore_utils.py @@ -4,7 +4,7 @@ from django.db.models import Count, Q from api.db_router import READ_REPLICA_ALIAS from api.db_utils import rls_transaction -from api.models import Finding, StatusChoices +from api.models import Finding, Scan, StatusChoices from prowler.lib.outputs.finding import Finding as FindingOutput logger = get_task_logger(__name__) @@ -35,25 +35,26 @@ def _aggregate_requirement_statistics_from_database( } """ requirement_statistics_by_check_id = {} - # TODO: take into account that now the relation is 1 finding == 1 resource, review this when the logic changes + # TODO: review when finding-resource relation changes from 1:1 with rls_transaction(tenant_id, using=READ_REPLICA_ALIAS): + # Pre-check: skip if the scan's provider is deleted (avoids JOINs in the main query) + if Scan.all_objects.filter(id=scan_id, provider__is_deleted=True).exists(): + return requirement_statistics_by_check_id + aggregated_statistics_queryset = ( Finding.all_objects.filter( tenant_id=tenant_id, scan_id=scan_id, muted=False, - resources__provider__is_deleted=False, ) .values("check_id") .annotate( total_findings=Count( "id", - distinct=True, filter=Q(status__in=[StatusChoices.PASS, StatusChoices.FAIL]), ), passed_findings=Count( "id", - distinct=True, filter=Q(status=StatusChoices.PASS), ), ) diff --git a/api/src/backend/tasks/tests/test_reports.py b/api/src/backend/tasks/tests/test_reports.py index a6a80b0891..858f4c06ca 100644 --- a/api/src/backend/tasks/tests/test_reports.py +++ b/api/src/backend/tasks/tests/test_reports.py @@ -169,35 +169,27 @@ class TestAggregateRequirementStatistics: assert result["check_1"]["passed"] == 1 assert result["check_1"]["total"] == 1 - def test_excludes_findings_without_resources(self, tenants_fixture, scans_fixture): - """Verify findings without resources are excluded from aggregation.""" + def test_skips_aggregation_for_deleted_provider( + self, tenants_fixture, scans_fixture + ): + """Verify aggregation returns empty when the scan's provider is soft-deleted.""" tenant = tenants_fixture[0] scan = scans_fixture[0] - # Finding WITH resource → should be counted self._create_finding_with_resource( tenant, scan, "finding-1", "check_1", StatusChoices.PASS ) - # Finding WITHOUT resource → should be EXCLUDED - Finding.objects.create( - tenant_id=tenant.id, - scan=scan, - uid="finding-2", - check_id="check_1", - status=StatusChoices.FAIL, - severity=Severity.high, - impact=Severity.high, - check_metadata={}, - raw_result={}, - ) + # Soft-delete the provider + provider = scan.provider + provider.is_deleted = True + provider.save(update_fields=["is_deleted"]) result = _aggregate_requirement_statistics_from_database( str(tenant.id), str(scan.id) ) - assert result["check_1"]["passed"] == 1 - assert result["check_1"]["total"] == 1 + assert result == {} def test_multiple_resources_no_double_count(self, tenants_fixture, scans_fixture): """Verify a finding with multiple resources is only counted once."""