From 8105ab3a6366776a847b4d7833fd0d020da81a90 Mon Sep 17 00:00:00 2001 From: Christophe Papazian <114495376+christophe-papazian@users.noreply.github.com> Date: Fri, 13 Sep 2024 15:03:05 +0200 Subject: [PATCH] fix(asm): fixing exploit prevention on custom redirect action (#10644) Check for redirecting actions as well for blocking requests with exploit prevention. Also add regression tests in threat tests. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [ ] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) (cherry picked from commit 69c090b6f1e5c2d1ce585f2ec52ea1dd864633d0) --- ddtrace/appsec/_common_module_patches.py | 15 +- ...ntion_on_redirection-fc2c0c73abb50b7b.yaml | 4 + .../appsec/appsec/rules-rasp-redirecting.json | 216 ++++++++++++++++++ tests/appsec/contrib_appsec/utils.py | 12 +- tests/appsec/rules.py | 1 + 5 files changed, 238 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/fix_exploit_prevention_on_redirection-fc2c0c73abb50b7b.yaml create mode 100644 tests/appsec/appsec/rules-rasp-redirecting.json diff --git a/ddtrace/appsec/_common_module_patches.py b/ddtrace/appsec/_common_module_patches.py index b58309bf33a..e0e24b38fb3 100644 --- a/ddtrace/appsec/_common_module_patches.py +++ b/ddtrace/appsec/_common_module_patches.py @@ -7,6 +7,7 @@ from typing import Any from typing import Callable from typing import Dict +from typing import Iterable import ddtrace from ddtrace.appsec._constants import WAF_ACTIONS @@ -40,6 +41,10 @@ def unpatch_common_modules(): try_unwrap("urllib.request", "OpenerDirector.open") +def _must_block(actions: Iterable[str]) -> bool: + return any(action in (WAF_ACTIONS.BLOCK_ACTION, WAF_ACTIONS.REDIRECT_ACTION) for action in actions) + + def wrapped_open_CFDDB7ABBA9081B6(original_open_callable, instance, args, kwargs): """ wrapper for open file function @@ -75,7 +80,7 @@ def wrapped_open_CFDDB7ABBA9081B6(original_open_callable, instance, args, kwargs crop_trace="wrapped_open_CFDDB7ABBA9081B6", rule_type=EXPLOIT_PREVENTION.TYPE.LFI, ) - if res and WAF_ACTIONS.BLOCK_ACTION in res.actions: + if res and _must_block(res.actions): raise BlockingException(core.get_item(WAF_CONTEXT_NAMES.BLOCKED), "exploit_prevention", "lfi", filename) try: return original_open_callable(*args, **kwargs) @@ -119,7 +124,7 @@ def wrapped_open_ED4CF71136E15EBF(original_open_callable, instance, args, kwargs crop_trace="wrapped_open_ED4CF71136E15EBF", rule_type=EXPLOIT_PREVENTION.TYPE.SSRF, ) - if res and WAF_ACTIONS.BLOCK_ACTION in res.actions: + if res and _must_block(res.actions): raise BlockingException(core.get_item(WAF_CONTEXT_NAMES.BLOCKED), "exploit_prevention", "ssrf", url) return original_open_callable(*args, **kwargs) @@ -157,7 +162,7 @@ def wrapped_request_D8CB81E472AF98A2(original_request_callable, instance, args, crop_trace="wrapped_request_D8CB81E472AF98A2", rule_type=EXPLOIT_PREVENTION.TYPE.SSRF, ) - if res and WAF_ACTIONS.BLOCK_ACTION in res.actions: + if res and _must_block(res.actions): raise BlockingException(core.get_item(WAF_CONTEXT_NAMES.BLOCKED), "exploit_prevention", "ssrf", url) return original_request_callable(*args, **kwargs) @@ -193,7 +198,7 @@ def wrapped_system_5542593D237084A7(original_command_callable, instance, args, k crop_trace="wrapped_system_5542593D237084A7", rule_type=EXPLOIT_PREVENTION.TYPE.CMDI, ) - if res and WAF_ACTIONS.BLOCK_ACTION in res.actions: + if res and _must_block(res.actions): raise BlockingException( core.get_item(WAF_CONTEXT_NAMES.BLOCKED), "exploit_prevention", "cmdi", command ) @@ -249,7 +254,7 @@ def execute_4C9BAC8E228EB347(instrument_self, query, args, kwargs) -> None: crop_trace="execute_4C9BAC8E228EB347", rule_type=EXPLOIT_PREVENTION.TYPE.SQLI, ) - if res and WAF_ACTIONS.BLOCK_ACTION in res.actions: + if res and _must_block(res.actions): raise BlockingException( core.get_item(WAF_CONTEXT_NAMES.BLOCKED), "exploit_prevention", "sqli", query ) diff --git a/releasenotes/notes/fix_exploit_prevention_on_redirection-fc2c0c73abb50b7b.yaml b/releasenotes/notes/fix_exploit_prevention_on_redirection-fc2c0c73abb50b7b.yaml new file mode 100644 index 00000000000..ed4aa632108 --- /dev/null +++ b/releasenotes/notes/fix_exploit_prevention_on_redirection-fc2c0c73abb50b7b.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + ASM: This fix resolves an issue where exploit prevention was not properly blocking requests with custom redirection actions. diff --git a/tests/appsec/appsec/rules-rasp-redirecting.json b/tests/appsec/appsec/rules-rasp-redirecting.json new file mode 100644 index 00000000000..a7a53db6e3b --- /dev/null +++ b/tests/appsec/appsec/rules-rasp-redirecting.json @@ -0,0 +1,216 @@ +{ + "version": "2.1", + "metadata": { + "rules_version": "rules_rasp" + }, + "actions": [ + { + "id": "block", + "type": "redirect_request", + "parameters": { + "status_code": 301, + "location": "https://www.datadoghq.com" + } + } + ], + "rules": [ + { + "id": "rasp-930-100", + "name": "Local file inclusion exploit", + "tags": { + "type": "lfi", + "category": "vulnerability_trigger", + "cwe": "22", + "capec": "1000/255/153/126", + "confidence": "0", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.io.fs.file" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "grpc.server.request.message" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ] + }, + "operator": "lfi_detector" + } + ], + "transformers": [], + "on_match": [ + "stack_trace", + "block" + ] + }, + { + "id": "rasp-934-100", + "name": "Server-side request forgery exploit", + "tags": { + "type": "ssrf", + "category": "vulnerability_trigger", + "cwe": "918", + "capec": "1000/225/115/664", + "confidence": "0", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.io.net.url" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "grpc.server.request.message" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ] + }, + "operator": "ssrf_detector" + } + ], + "transformers": [], + "on_match": [ + "stack_trace", + "block" + ] + }, + { + "id": "rasp-942-100", + "name": "SQL injection exploit", + "tags": { + "type": "sql_injection", + "category": "vulnerability_trigger", + "cwe": "89", + "capec": "1000/152/248/66", + "confidence": "0", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.db.statement" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ], + "db_type": [ + { + "address": "server.db.system" + } + ] + }, + "operator": "sqli_detector" + } + ], + "transformers": [], + "on_match": [ + "stack_trace", + "block" + ] + }, + { + "id": "rasp-932-100", + "name": "Shell injection exploit", + "tags": { + "type": "command_injection", + "category": "vulnerability_trigger", + "cwe": "77", + "capec": "1000/152/248/88", + "confidence": "0", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.sys.shell.cmd" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "grpc.server.request.message" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ] + }, + "operator": "shi_detector" + } + ], + "transformers": [], + "on_match": [ + "stack_trace", + "block" + ] + } + ] +} \ No newline at end of file diff --git a/tests/appsec/contrib_appsec/utils.py b/tests/appsec/contrib_appsec/utils.py index 293beeb16b9..eac38c05bab 100644 --- a/tests/appsec/contrib_appsec/utils.py +++ b/tests/appsec/contrib_appsec/utils.py @@ -1299,12 +1299,13 @@ def test_stream_response( ], ) @pytest.mark.parametrize( - ("rule_file", "action_level"), + ("rule_file", "action_level", "status_expected"), # action_level 0: no action, 1: report, 2: block [ - (rules.RULES_EXPLOIT_PREVENTION, 1), - (rules.RULES_EXPLOIT_PREVENTION_BLOCKING, 2), - (rules.RULES_EXPLOIT_PREVENTION_DISABLED, 0), + (rules.RULES_EXPLOIT_PREVENTION, 1, 200), + (rules.RULES_EXPLOIT_PREVENTION_BLOCKING, 2, 403), + (rules.RULES_EXPLOIT_PREVENTION_REDIRECTING, 2, 301), + (rules.RULES_EXPLOIT_PREVENTION_DISABLED, 0, 200), ], ) def test_exploit_prevention( @@ -1321,6 +1322,7 @@ def test_exploit_prevention( top_functions, rule_file, action_level, + status_expected, ): from unittest.mock import patch as mock_patch @@ -1334,7 +1336,7 @@ def test_exploit_prevention( self.update_tracer(interface) assert asm_config._asm_enabled == asm_enabled response = interface.client.get(f"/rasp/{endpoint}/?{parameters}") - code = 403 if action_level == 2 and asm_enabled and ep_enabled else 200 + code = status_expected if asm_enabled and ep_enabled else 200 assert self.status(response) == code, (self.status(response), code) assert get_tag(http.STATUS_CODE) == str(code), (get_tag(http.STATUS_CODE), code) if code == 200: diff --git a/tests/appsec/rules.py b/tests/appsec/rules.py index a79c72ca1a2..a9b23bef349 100644 --- a/tests/appsec/rules.py +++ b/tests/appsec/rules.py @@ -13,6 +13,7 @@ RULES_BAD_VERSION = os.path.join(ROOT_DIR, "rules-bad_version.json") RULES_EXPLOIT_PREVENTION = os.path.join(ROOT_DIR, "rules-rasp.json") RULES_EXPLOIT_PREVENTION_BLOCKING = os.path.join(ROOT_DIR, "rules-rasp-blocking.json") +RULES_EXPLOIT_PREVENTION_REDIRECTING = os.path.join(ROOT_DIR, "rules-rasp-redirecting.json") RULES_EXPLOIT_PREVENTION_DISABLED = os.path.join(ROOT_DIR, "rules-rasp-disabled.json") RESPONSE_CUSTOM_JSON = os.path.join(ROOT_DIR, "response-custom.json")