From 497a01dfdd57b6cdd336f41b82d8c8cc59240739 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 26 Nov 2024 21:22:30 +0000 Subject: [PATCH] fix(asm): add global states to ensure patching once [backport 2.16] (#11532) Backport 81824b8ba43c88164d4e709097c7772a9846150f from #11522 to 2.16. Ensure common patches for SCA and Exploit Prevention are loaded.. - only once - only if exploit prevention is active or sca is active Changes: - factorize load_common_modules logic in ddtrace.appsec - boolean state for patch_common_module and enable_iast_propagation to ensure they are only called once. - ensure it's loaded after one click activation - ensure it's properly loaded in unit tests if required - add some failsafe for iast in wrap_open for importerror - update an iast test to reflect that common_modules is loaded in the test by default. APPSEC-55997 ## 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 - [x] 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) Co-authored-by: Christophe Papazian <114495376+christophe-papazian@users.noreply.github.com> --- ddtrace/_monkey.py | 8 +++----- ddtrace/appsec/__init__.py | 16 +++++++++++++++ ddtrace/appsec/_common_module_patches.py | 20 ++++++++++++++++--- ddtrace/appsec/_iast/__init__.py | 12 +++++++++++ ddtrace/appsec/_remoteconfiguration.py | 5 +++++ ...prevention_patch_fix-1bdd7540e1d085d8.yaml | 4 ++++ .../integrations/test_flask_telemetry.py | 4 +++- tests/utils.py | 2 ++ 8 files changed, 62 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/exploit_prevention_patch_fix-1bdd7540e1d085d8.yaml diff --git a/ddtrace/_monkey.py b/ddtrace/_monkey.py index 73bd0cc2d5f..7869bb39470 100644 --- a/ddtrace/_monkey.py +++ b/ddtrace/_monkey.py @@ -5,12 +5,13 @@ from wrapt.importer import when_imported +from ddtrace.appsec import load_common_appsec_modules + from .appsec._iast._utils import _is_iast_enabled from .internal import telemetry from .internal.logger import get_logger from .internal.utils import formats from .settings import _config as config -from .settings.asm import config as asm_config if TYPE_CHECKING: # pragma: no cover @@ -233,10 +234,7 @@ def patch_all(**patch_modules): patch_iast() enable_iast_propagation() - if asm_config._ep_enabled or asm_config._iast_enabled: - from ddtrace.appsec._common_module_patches import patch_common_modules - - patch_common_modules() + load_common_appsec_modules() def patch(raise_errors=True, patch_modules_prefix=DEFAULT_MODULES_PREFIX, **patch_modules): diff --git a/ddtrace/appsec/__init__.py b/ddtrace/appsec/__init__.py index 27463c7c8a1..bc89c0f2127 100644 --- a/ddtrace/appsec/__init__.py +++ b/ddtrace/appsec/__init__.py @@ -1,3 +1,7 @@ +from ddtrace.internal import core +from ddtrace.settings.asm import config as asm_config + + _APPSEC_TO_BE_LOADED = True _IAST_TO_BE_LOADED = True @@ -20,3 +24,15 @@ def load_iast(): if _IAST_TO_BE_LOADED: iast_listen() _IAST_TO_BE_LOADED = False + + +def load_common_appsec_modules(): + """Lazily load the common module patches.""" + if (asm_config._ep_enabled and asm_config._asm_enabled) or asm_config._iast_enabled: + from ddtrace.appsec._common_module_patches import patch_common_modules + + patch_common_modules() + + +# for tests that needs to load the appsec module later +core.on("test.config.override", load_common_appsec_modules) diff --git a/ddtrace/appsec/_common_module_patches.py b/ddtrace/appsec/_common_module_patches.py index 494e8e63fb7..a5ab2d1533d 100644 --- a/ddtrace/appsec/_common_module_patches.py +++ b/ddtrace/appsec/_common_module_patches.py @@ -27,8 +27,13 @@ log = get_logger(__name__) _DD_ORIGINAL_ATTRIBUTES: Dict[Any, Any] = {} +_is_patched = False + def patch_common_modules(): + global _is_patched + if _is_patched: + return try_wrap_function_wrapper("builtins", "open", wrapped_open_CFDDB7ABBA9081B6) try_wrap_function_wrapper("urllib.request", "OpenerDirector.open", wrapped_open_ED4CF71136E15EBF) try_wrap_function_wrapper("_io", "BytesIO.read", wrapped_read_F3E51D71B4EC16EF) @@ -37,13 +42,18 @@ def patch_common_modules(): core.on("asm.block.dbapi.execute", execute_4C9BAC8E228EB347) if asm_config._iast_enabled: _set_metric_iast_instrumented_sink(VULN_PATH_TRAVERSAL) + _is_patched = True def unpatch_common_modules(): + global _is_patched + if not _is_patched: + return try_unwrap("builtins", "open") try_unwrap("urllib.request", "OpenerDirector.open") try_unwrap("_io", "BytesIO.read") try_unwrap("_io", "StringIO.read") + _is_patched = False def wrapped_read_F3E51D71B4EC16EF(original_read_callable, instance, args, kwargs): @@ -78,10 +88,14 @@ def wrapped_open_CFDDB7ABBA9081B6(original_open_callable, instance, args, kwargs wrapper for open file function """ if asm_config._iast_enabled: - from ddtrace.appsec._iast.taint_sinks.path_traversal import check_and_report_path_traversal - - check_and_report_path_traversal(*args, **kwargs) + try: + from ddtrace.appsec._iast.taint_sinks.path_traversal import check_and_report_path_traversal + check_and_report_path_traversal(*args, **kwargs) + except ImportError: + # open is used during module initialization + # and shouldn't be changed at that time + return original_open_callable(*args, **kwargs) if ( asm_config._asm_enabled and asm_config._ep_enabled diff --git a/ddtrace/appsec/_iast/__init__.py b/ddtrace/appsec/_iast/__init__.py index b98912b0410..5aab86cf783 100644 --- a/ddtrace/appsec/_iast/__init__.py +++ b/ddtrace/appsec/_iast/__init__.py @@ -27,6 +27,7 @@ def wrapped_function(wrapped, instance, args, kwargs): ) return wrapped(*args, **kwargs) """ # noqa: RST201, RST213, RST210 + import inspect import sys @@ -72,6 +73,9 @@ def ddtrace_iast_flask_patch(): sys.modules[module_name] = compiled_code +_iast_propagation_enabled = False + + def enable_iast_propagation(): """Add IAST AST patching in the ModuleWatchdog""" # DEV: These imports are here to avoid _ast.ast_patching import in the top level @@ -79,8 +83,12 @@ def enable_iast_propagation(): from ddtrace.appsec._iast._ast.ast_patching import _should_iast_patch from ddtrace.appsec._iast._loader import _exec_iast_patched_module + global _iast_propagation_enabled + if _iast_propagation_enabled: + return log.debug("IAST enabled") ModuleWatchdog.register_pre_exec_module_hook(_should_iast_patch, _exec_iast_patched_module) + _iast_propagation_enabled = True def disable_iast_propagation(): @@ -90,10 +98,14 @@ def disable_iast_propagation(): from ddtrace.appsec._iast._ast.ast_patching import _should_iast_patch from ddtrace.appsec._iast._loader import _exec_iast_patched_module + global _iast_propagation_enabled + if not _iast_propagation_enabled: + return try: ModuleWatchdog.remove_pre_exec_module_hook(_should_iast_patch, _exec_iast_patched_module) except KeyError: log.warning("IAST is already disabled and it's not in the ModuleWatchdog") + _iast_propagation_enabled = False __all__ = [ diff --git a/ddtrace/appsec/_remoteconfiguration.py b/ddtrace/appsec/_remoteconfiguration.py index 4aa7910773c..0d470db08c8 100644 --- a/ddtrace/appsec/_remoteconfiguration.py +++ b/ddtrace/appsec/_remoteconfiguration.py @@ -72,6 +72,11 @@ def enable_appsec_rc(test_tracer: Optional[Tracer] = None) -> None: remoteconfig_poller.register(PRODUCTS.ASM_DATA, asm_callback) # IP Blocking remoteconfig_poller.register(PRODUCTS.ASM, asm_callback) # Exclusion Filters & Custom Rules remoteconfig_poller.register(PRODUCTS.ASM_DD, asm_callback) # DD Rules + # ensure exploit prevention patches are loaded by one-click activation + if asm_config._asm_enabled: + from ddtrace.appsec import load_common_appsec_modules + + load_common_appsec_modules() forksafe.register(_forksafe_appsec_rc) telemetry_writer.product_activated(TELEMETRY_APM_PRODUCT.APPSEC, True) diff --git a/releasenotes/notes/exploit_prevention_patch_fix-1bdd7540e1d085d8.yaml b/releasenotes/notes/exploit_prevention_patch_fix-1bdd7540e1d085d8.yaml new file mode 100644 index 00000000000..fde9ea25c80 --- /dev/null +++ b/releasenotes/notes/exploit_prevention_patch_fix-1bdd7540e1d085d8.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + ASM: This fix ensures that common patches for exploit prevention and sca are only loaded if required, and only loaded once. diff --git a/tests/appsec/integrations/test_flask_telemetry.py b/tests/appsec/integrations/test_flask_telemetry.py index 6cea739b94f..99e00112e6f 100644 --- a/tests/appsec/integrations/test_flask_telemetry.py +++ b/tests/appsec/integrations/test_flask_telemetry.py @@ -1,4 +1,5 @@ from ddtrace.appsec._iast._handlers import _on_flask_patch +from ddtrace.appsec._iast.constants import VULN_PATH_TRAVERSAL from tests.appsec.appsec_utils import flask_server from tests.utils import override_global_config @@ -24,7 +25,8 @@ def test_flask_instrumented_metrics(telemetry_writer): metrics_result = telemetry_writer._namespace._metrics_data metrics_source_tags_result = [metric._tags[0][1] for metric in metrics_result["generate-metrics"]["iast"].values()] - assert len(metrics_source_tags_result) == 7 + assert len(metrics_source_tags_result) == 8 + assert VULN_PATH_TRAVERSAL in metrics_source_tags_result assert origin_to_str(OriginType.HEADER_NAME) in metrics_source_tags_result assert origin_to_str(OriginType.HEADER) in metrics_source_tags_result assert origin_to_str(OriginType.PARAMETER) in metrics_source_tags_result diff --git a/tests/utils.py b/tests/utils.py index fcf494075e4..56316b0198c 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -23,6 +23,7 @@ from ddtrace.constants import SPAN_MEASURED_KEY from ddtrace.ext import http from ddtrace.internal import agent +from ddtrace.internal import core from ddtrace.internal.ci_visibility.writer import CIVisibilityWriter from ddtrace.internal.compat import httplib from ddtrace.internal.compat import parse @@ -181,6 +182,7 @@ def override_global_config(values): # If ddtrace.settings.asm.config has changed, check _asm_can_be_enabled again ddtrace.settings.asm.config._eval_asm_can_be_enabled() try: + core.dispatch("test.config.override") yield finally: # Reset all to their original values