fix(asm): enabling builtins.open sink point (#9307)

Revert #9247 and restore path traversal patching but this time, instead to patch `open` by AST, we're monkey patching like we're doing with Exploit Prevention ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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)
DataDog · May 17, 2024 · 3b08d60 · 3b08d60
1 parent 39897ac
commit 3b08d60
Show file tree

Hide file tree

Showing 9 changed files with 55 additions and 61 deletions.
diff --git a/ddtrace/appsec/_common_module_patches.py b/ddtrace/appsec/_common_module_patches.py
@@ -10,6 +10,8 @@
 
 from ddtrace.appsec._constants import WAF_ACTIONS
 from ddtrace.appsec._constants import WAF_CONTEXT_NAMES
+from ddtrace.appsec._iast._metrics import _set_metric_iast_instrumented_sink
+from ddtrace.appsec._iast.constants import VULN_PATH_TRAVERSAL
 from ddtrace.internal import core
 from ddtrace.internal._exceptions import BlockingException
 from ddtrace.internal.logger import get_logger
@@ -26,6 +28,8 @@
 def patch_common_modules():
     try_wrap_function_wrapper("builtins", "open", wrapped_open_CFDDB7ABBA9081B6)
     try_wrap_function_wrapper("urllib.request", "OpenerDirector.open", wrapped_open_ED4CF71136E15EBF)
+    if asm_config._iast_enabled:
+        _set_metric_iast_instrumented_sink(VULN_PATH_TRAVERSAL)
 
 
 def unpatch_common_modules():
@@ -38,8 +42,9 @@ def wrapped_open_CFDDB7ABBA9081B6(original_open_callable, instance, args, kwargs
     wrapper for open file function
     """
     if asm_config._iast_enabled:
-        # LFI sink to be added
-        pass
+        from ddtrace.appsec._iast.taint_sinks.path_traversal import check_and_report_path_traversal
+
+        check_and_report_path_traversal(*args, **kwargs)
 
     if asm_config._asm_enabled and asm_config._ep_enabled:
         try:

diff --git a/ddtrace/appsec/_iast/_ast/visitor.py b/ddtrace/appsec/_iast/_ast/visitor.py
@@ -147,10 +147,7 @@ def __init__(
         self._sinkpoints_spec = {
             "definitions_module": "ddtrace.appsec._iast.taint_sinks",
             "alias_module": "ddtrace_taint_sinks",
-            "functions": {
-                # FIXME: disabled to unblock release 2.9
-                # "open": "ddtrace_taint_sinks.open_path_traversal",
-            },
+            "functions": {},
         }
         self._sinkpoints_functions = self._sinkpoints_spec["functions"]
 

diff --git a/ddtrace/appsec/_iast/_patch_modules.py b/ddtrace/appsec/_iast/_patch_modules.py
@@ -4,7 +4,6 @@
 IAST_PATCH = {
     "command_injection": True,
     "header_injection": True,
-    "path_traversal": True,
     "weak_cipher": True,
     "weak_hash": True,
 }

diff --git a/ddtrace/appsec/_iast/taint_sinks/__init__.py b/ddtrace/appsec/_iast/taint_sinks/__init__.py
@@ -1,8 +1,6 @@
 from .ast_taint import ast_function
-from .path_traversal import open_path_traversal
 
 
 __all__ = [
-    "open_path_traversal",
     "ast_function",
 ]
diff --git a/ddtrace/appsec/_iast/taint_sinks/path_traversal.py b/ddtrace/appsec/_iast/taint_sinks/path_traversal.py
@@ -1,14 +1,10 @@
 from typing import Any
-from typing import Text
 
 from ddtrace.internal.logger import get_logger
 
 from ..._constants import IAST_SPAN_TAGS
 from .. import oce
-from .._metrics import _set_metric_iast_instrumented_sink
 from .._metrics import increment_iast_span_metric
-from .._patch import set_and_check_module_is_patched
-from .._patch import set_module_unpatched
 from ..constants import VULN_PATH_TRAVERSAL
 from ..processor import AppSecIastSpanProcessor
 from ._base import VulnerabilityBase
@@ -22,21 +18,6 @@ class PathTraversal(VulnerabilityBase):
     vulnerability_type = VULN_PATH_TRAVERSAL
 
 
-def get_version() -> Text:
-    return ""
-
-
-def unpatch_iast():
-    set_module_unpatched("builtins", default_attr="_datadog_path_traversal_patch")
-
-
-def patch():
-    """Wrap functions which interact with file system."""
-    if not set_and_check_module_is_patched("builtins", default_attr="_datadog_path_traversal_patch"):
-        return
-    _set_metric_iast_instrumented_sink(VULN_PATH_TRAVERSAL)
-
-
 def check_and_report_path_traversal(*args: Any, **kwargs: Any) -> None:
     if AppSecIastSpanProcessor.is_span_analyzed() and PathTraversal.has_quota():
         try:
@@ -45,14 +26,10 @@ def check_and_report_path_traversal(*args: Any, **kwargs: Any) -> None:
 
             increment_iast_span_metric(IAST_SPAN_TAGS.TELEMETRY_EXECUTED_SINK, PathTraversal.vulnerability_type)
             _set_metric_iast_executed_sink(PathTraversal.vulnerability_type)
-            if is_pyobject_tainted(args[0]):
-                PathTraversal.report(evidence_value=args[0])
+            filename_arg = args[0] if args else kwargs.get("file", None)
+            if is_pyobject_tainted(filename_arg):
+                PathTraversal.report(evidence_value=filename_arg)
         except Exception:
             log.debug("Unexpected exception while reporting vulnerability", exc_info=True)
     else:
         log.debug("IAST: no vulnerability quota to analyze more sink points")
-
-
-def open_path_traversal(*args, **kwargs):
-    check_and_report_path_traversal(*args, **kwargs)
-    return open(*args, **kwargs)
diff --git a/tests/appsec/iast/conftest.py b/tests/appsec/iast/conftest.py
@@ -3,6 +3,8 @@
 
 import pytest
 
+from ddtrace.appsec._common_module_patches import patch_common_modules
+from ddtrace.appsec._common_module_patches import unpatch_common_modules
 from ddtrace.appsec._constants import IAST
 from ddtrace.appsec._iast import oce
 from ddtrace.appsec._iast._patches.json_tainting import patch as json_patch
@@ -13,7 +15,6 @@
 from ddtrace.appsec._iast.taint_sinks.command_injection import unpatch as cmdi_unpatch
 from ddtrace.appsec._iast.taint_sinks.header_injection import patch as header_injection_patch
 from ddtrace.appsec._iast.taint_sinks.header_injection import unpatch as header_injection_unpatch
-from ddtrace.appsec._iast.taint_sinks.path_traversal import patch as path_traversal_patch
 from ddtrace.appsec._iast.taint_sinks.weak_cipher import patch as weak_cipher_patch
 from ddtrace.appsec._iast.taint_sinks.weak_cipher import unpatch_iast as weak_cipher_unpatch
 from ddtrace.appsec._iast.taint_sinks.weak_hash import patch as weak_hash_patch
@@ -70,7 +71,6 @@ def iast_span(tracer, env, request_sampling="100", deduplication=False):
             span.span_type = "web"
             weak_hash_patch()
             weak_cipher_patch()
-            path_traversal_patch()
             sqli_sqlite_patch()
             json_patch()
             psycopg_patch()
@@ -79,7 +79,9 @@ def iast_span(tracer, env, request_sampling="100", deduplication=False):
             header_injection_patch()
             langchain_patch()
             iast_span_processor.on_span_start(span)
+            patch_common_modules()
             yield span
+            unpatch_common_modules()
             iast_span_processor.on_span_finish(span)
             weak_hash_unpatch()
             weak_cipher_unpatch()

diff --git a/tests/appsec/iast/taint_sinks/test_path_traversal.py b/tests/appsec/iast/taint_sinks/test_path_traversal.py
@@ -1,5 +1,6 @@
 import os
 
+import mock
 import pytest
 
 from ddtrace.appsec._constants import IAST
@@ -23,8 +24,6 @@ def _get_path_traversal_module_functions():
                 yield module, function
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 def test_path_traversal_open(iast_span_defaults):
     mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.path_traversal")
 
@@ -51,8 +50,41 @@ def test_path_traversal_open(iast_span_defaults):
     assert vulnerability["evidence"].get("redacted") is None
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
+@mock.patch("tests.appsec.iast.fixtures.taint_sinks.path_traversal.open")
+def test_path_traversal_open_and_mock(mock_open, iast_span_defaults):
+    """Confirm we can mock the open function and IAST path traversal vulnerability is not reported"""
+    mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.path_traversal")
+
+    file_path = os.path.join(ROOT_DIR, "../fixtures", "taint_sinks", "path_traversal_test_file.txt")
+
+    tainted_string = taint_pyobject(
+        file_path, source_name="path", source_value=file_path, source_origin=OriginType.PATH
+    )
+    mod.pt_open(tainted_string)
+
+    mock_open.assert_called_once_with(file_path)
+
+    span_report = core.get_item(IAST.CONTEXT_KEY, span=iast_span_defaults)
+    assert span_report is None
+
+
+def test_path_traversal_open_and_mock_after_patch_module(iast_span_defaults):
+    """Confirm we can mock the open function and IAST path traversal vulnerability is not reported"""
+    mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.path_traversal")
+    with mock.patch("tests.appsec.iast.fixtures.taint_sinks.path_traversal.open") as mock_open:
+        file_path = os.path.join(ROOT_DIR, "../fixtures", "taint_sinks", "path_traversal_test_file.txt")
+
+        tainted_string = taint_pyobject(
+            file_path, source_name="path", source_value=file_path, source_origin=OriginType.PATH
+        )
+        mod.pt_open(tainted_string)
+
+        mock_open.assert_called_once_with(file_path)
+
+        span_report = core.get_item(IAST.CONTEXT_KEY, span=iast_span_defaults)
+        assert span_report is None
+
+
 @pytest.mark.parametrize(
     "file_path",
     (
@@ -74,8 +106,6 @@ def test_path_traversal_open_secure(file_path, iast_span_defaults):
     assert span_report is None
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize(
     "module, function",
     _get_path_traversal_module_functions(),
@@ -109,8 +139,6 @@ def test_path_traversal(module, function, iast_span_defaults):
     assert vulnerability["evidence"].get("redacted") is None
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize("num_vuln_expected", [1, 0, 0])
 def test_path_traversal_deduplication(num_vuln_expected, iast_span_deduplication_enabled):
     mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.path_traversal")

diff --git a/tests/appsec/iast/test_iast_propagation_path.py b/tests/appsec/iast/test_iast_propagation_path.py
@@ -27,8 +27,6 @@ def _assert_vulnerability(data, value_parts, file_line_label):
     assert vulnerability["hash"] == hash_value
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 def test_propagation_no_path(iast_span_defaults):
     mod = _iast_patched_module("tests.appsec.iast.fixtures.propagation_path")
     origin1 = "taintsource"
@@ -41,8 +39,6 @@ def test_propagation_no_path(iast_span_defaults):
     assert span_report is None
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize(
     "origin1",
     [
@@ -77,8 +73,6 @@ def test_propagation_path_1_origin_1_propagation(origin1, iast_span_defaults):
     _assert_vulnerability(data, value_parts, "propagation_path_1_source_1_prop")
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize(
     "origin1",
     [
@@ -115,8 +109,6 @@ def test_propagation_path_1_origins_2_propagations(origin1, iast_span_defaults):
     _assert_vulnerability(data, value_parts, "propagation_path_1_source_2_prop")
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize(
     "origin1, origin2",
     [
@@ -167,8 +159,6 @@ def test_propagation_path_2_origins_2_propagations(origin1, origin2, iast_span_d
     _assert_vulnerability(data, value_parts, "propagation_path_2_source_2_prop")
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize(
     "origin1, origin2",
     [
@@ -227,8 +217,6 @@ def test_propagation_path_2_origins_3_propagation(origin1, origin2, iast_span_de
     _assert_vulnerability(data, value_parts, "propagation_path_3_prop")
 
 
-# FIXME: enable once the mock + open issue is fixed
-@pytest.mark.skip
 @pytest.mark.parametrize(
     "origin1, origin2",
     [

diff --git a/tests/appsec/iast/test_telemetry.py b/tests/appsec/iast/test_telemetry.py
@@ -1,6 +1,8 @@
 import pytest
 
 from ddtrace.appsec import _asm_request_context
+from ddtrace.appsec._common_module_patches import patch_common_modules
+from ddtrace.appsec._common_module_patches import unpatch_common_modules
 from ddtrace.appsec._constants import IAST_SPAN_TAGS
 from ddtrace.appsec._handlers import _on_django_patch
 from ddtrace.appsec._iast._metrics import TELEMETRY_DEBUG_VERBOSITY
@@ -19,8 +21,6 @@
 from ddtrace.appsec._iast.taint_sinks.command_injection import patch as cmdi_patch
 from ddtrace.appsec._iast.taint_sinks.header_injection import patch as header_injection_patch
 from ddtrace.appsec._iast.taint_sinks.header_injection import unpatch as header_injection_unpatch
-from ddtrace.appsec._iast.taint_sinks.path_traversal import patch as path_traversal_patch
-from ddtrace.appsec._iast.taint_sinks.path_traversal import unpatch_iast as path_traversal_unpatch
 from ddtrace.contrib.sqlalchemy import patch as sqli_sqlalchemy_patch
 from ddtrace.contrib.sqlite3 import patch as sqli_sqlite3_patch
 from ddtrace.ext import SpanTypes
@@ -107,11 +107,11 @@ def test_metric_instrumented_cmdi(no_request_sampling, telemetry_writer):
 
 def test_metric_instrumented_path_traversal(no_request_sampling, telemetry_writer):
     # We need to unpatch first because ddtrace.appsec._iast._patch_modules loads at runtime this patch function
-    path_traversal_unpatch()
+    unpatch_common_modules()
     with override_env(dict(DD_IAST_TELEMETRY_VERBOSITY="INFORMATION")), override_global_config(
         dict(_iast_enabled=True)
     ):
-        path_traversal_patch()
+        patch_common_modules()
 
     _assert_instrumented_sink(telemetry_writer, VULN_PATH_TRAVERSAL)