fix(asm): add global states to ensure patching once [backport 2.16] (#…

…11532)

Backport 81824b8 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 <[email protected]>

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):

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)

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

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,15 +73,22 @@ 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
     # because they are slow and affect serverless startup time
     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__ = [

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)

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.

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

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