Skip to content

Commit

Permalink
chore(asm): clean libddwaf loading (#12102)
Browse files Browse the repository at this point in the history
Depending of the timing, libddwaf loading process could create triggers
that would create loops in our instrumentation.
From what I investigated:
- if loaded too early, it could have bad interactions with gevent.
- if loaded too late, it could be self instrumented by the tracer,
creating a loop, as ctypes is using Popen and subprocess.

while keeping the late loading introduced by 2 previous PRs
- #11987
- #12013
this PR introduced a mechanism to bypass tracer instrumentation during
ctypes loading, to avoid a possible loop that would prevent the WAF to
be loaded.


## 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)
  • Loading branch information
christophe-papazian authored Jan 28, 2025
1 parent 16d5280 commit 4f0bcb5
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 12 deletions.
3 changes: 3 additions & 0 deletions ddtrace/appsec/_ddwaf/ddwaf_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@

if system() == "Linux":
try:
asm_config._bypass_instrumentation_for_waf = True
ctypes.CDLL(ctypes.util.find_library("rt"), mode=ctypes.RTLD_GLOBAL)
except Exception: # nosec
pass
finally:
asm_config._bypass_instrumentation_for_waf = False

ARCHI = machine().lower()

Expand Down
26 changes: 14 additions & 12 deletions ddtrace/appsec/_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@
from json.decoder import JSONDecodeError
import os
import os.path
from typing import TYPE_CHECKING
from typing import Any
from typing import Dict
from typing import List
from typing import Optional
from typing import Set
from typing import Tuple
from typing import Union


if TYPE_CHECKING:
import ddtrace.appsec._ddwaf as ddwaf

import weakref

from ddtrace._trace.processor import SpanProcessor
Expand Down Expand Up @@ -167,14 +173,17 @@ def __post_init__(self) -> None:
def delayed_init(self) -> None:
try:
if self._rules is not None and not hasattr(self, "_ddwaf"):
self._ddwaf = ddwaf.DDWaf(
from ddtrace.appsec._ddwaf import DDWaf # noqa: E402
import ddtrace.appsec._metrics as metrics # noqa: E402

self.metrics = metrics
self._ddwaf = DDWaf(
self._rules, self.obfuscation_parameter_key_regexp, self.obfuscation_parameter_value_regexp
)
_set_waf_init_metric(self._ddwaf.info)
self.metrics._set_waf_init_metric(self._ddwaf.info)
except Exception:
# Partial of DDAS-0005-00
log.warning("[DDAS-0005-00] WAF initialization failed")
raise
self._update_required()

def _update_required(self):
Expand All @@ -193,7 +202,7 @@ def _update_rules(self, new_rules: Dict[str, Any]) -> bool:
if asm_config._asm_static_rule_file is not None:
return result
result = self._ddwaf.update_rules(new_rules)
_set_waf_updates_metric(self._ddwaf.info)
self.metrics._set_waf_updates_metric(self._ddwaf.info)
self._update_required()
return result

Expand Down Expand Up @@ -241,7 +250,7 @@ def waf_callable(custom_data=None, **kwargs):
return self._waf_action(span._local_root or span, ctx, custom_data, **kwargs)

_asm_request_context.set_waf_callback(waf_callable)
_asm_request_context.add_context_callback(_set_waf_request_metrics)
_asm_request_context.add_context_callback(self.metrics._set_waf_request_metrics)
if headers is not None:
_asm_request_context.set_waf_address(SPAN_DATA_NAMES.REQUEST_HEADERS_NO_COOKIES, headers)
_asm_request_context.set_waf_address(
Expand Down Expand Up @@ -436,10 +445,3 @@ def on_span_finish(self, span: Span) -> None:
del self._span_to_waf_ctx[s]
except Exception: # nosec B110
pass


# load waf at the end only to avoid possible circular imports with gevent
import ddtrace.appsec._ddwaf as ddwaf # noqa: E402
from ddtrace.appsec._metrics import _set_waf_init_metric # noqa: E402
from ddtrace.appsec._metrics import _set_waf_request_metrics # noqa: E402
from ddtrace.appsec._metrics import _set_waf_updates_metric # noqa: E402
6 changes: 6 additions & 0 deletions ddtrace/contrib/internal/subprocess/patch.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,8 @@ def unpatch() -> None:
@trace_utils.with_traced_module
def _traced_ossystem(module, pin, wrapped, instance, args, kwargs):
try:
if asm_config._bypass_instrumentation_for_waf:
return wrapped(*args, **kwargs)
if isinstance(args[0], str):
for callback in _STR_CALLBACKS.values():
callback(args[0])
Expand Down Expand Up @@ -393,6 +395,8 @@ def _traced_osspawn(module, pin, wrapped, instance, args, kwargs):
@trace_utils.with_traced_module
def _traced_subprocess_init(module, pin, wrapped, instance, args, kwargs):
try:
if asm_config._bypass_instrumentation_for_waf:
return wrapped(*args, **kwargs)
cmd_args = args[0] if len(args) else kwargs["args"]
if isinstance(cmd_args, (list, tuple, str)):
if kwargs.get("shell", False):
Expand Down Expand Up @@ -425,6 +429,8 @@ def _traced_subprocess_init(module, pin, wrapped, instance, args, kwargs):
@trace_utils.with_traced_module
def _traced_subprocess_wait(module, pin, wrapped, instance, args, kwargs):
try:
if asm_config._bypass_instrumentation_for_waf:
return wrapped(*args, **kwargs)
binary = core.get_item("subprocess_popen_binary")

with pin.tracer.trace(COMMANDS.SPAN_NAME, resource=binary, span_type=SpanTypes.SYSTEM) as span:
Expand Down
1 change: 1 addition & 0 deletions ddtrace/settings/asm.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ class ASMConfig(Env):
default=r"^[+-]?((0b[01]+)|(0x[0-9A-Fa-f]+)|(\d+\.?\d*(?:[Ee][+-]?\d+)?|\.\d+(?:[Ee][+-]"
+ r"?\d+)?)|(X\'[0-9A-Fa-f]+\')|(B\'[01]+\'))$",
)
_bypass_instrumentation_for_waf = False

def __init__(self):
super().__init__()
Expand Down

0 comments on commit 4f0bcb5

Please sign in to comment.