Skip to content

Commit dde2e0c

Browse files
christophe-papazianemmettbutlertabgokmajorgreysmabdinur
authored
feat(asm): redirect requests for custom blocking (#6884)
Allow tracer to handle custom blocking actions of type `redirect_request` in Django and Flask. This completes the full support of RFC "ASM Rules blocking configuration - libraries" started with #6615 ## 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] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [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)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly 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) - [x] If this PR 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`. - [x] This PR doesn't touch any of that. --------- Co-authored-by: Emmett Butler <[email protected]> Co-authored-by: Teague Bick <[email protected]> Co-authored-by: Tahir H. Butt <[email protected]> Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Alberto Vara <[email protected]> Co-authored-by: Federico Mon <[email protected]>
1 parent c795b91 commit dde2e0c

File tree

10 files changed

+209
-23
lines changed

10 files changed

+209
-23
lines changed

ddtrace/appsec/_constants.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ class WAF_ACTIONS(object):
166166
ID = "id"
167167
DEFAULT_PARAMETERS = STATUS_403_TYPE_AUTO
168168
BLOCK_ACTION = "block_request"
169+
REDIRECT_ACTION = "redirect_request"
169170
DEFAULT_ACTONS = {
170171
BLOCK: {
171172
ID: BLOCK,

ddtrace/appsec/processor.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,9 +321,21 @@ def _waf_action(self, span, ctx, custom_data=None):
321321
log.debug("[DDAS-011-00] ASM In-App WAF returned: %s. Timeout %s", waf_results.data, waf_results.timeout)
322322

323323
for action in waf_results.actions:
324-
if self._actions.get(action, {}).get(WAF_ACTIONS.TYPE) == WAF_ACTIONS.BLOCK_ACTION:
324+
action_type = self._actions.get(action, {}).get(WAF_ACTIONS.TYPE, None)
325+
if action_type == WAF_ACTIONS.BLOCK_ACTION:
325326
blocked = self._actions[action][WAF_ACTIONS.PARAMETERS]
326327
break
328+
elif action_type == WAF_ACTIONS.REDIRECT_ACTION:
329+
blocked = self._actions[action][WAF_ACTIONS.PARAMETERS]
330+
location = blocked.get("location", "")
331+
if not location:
332+
blocked = WAF_ACTIONS.DEFAULT_PARAMETERS
333+
break
334+
status_code = str(blocked.get("status_code", ""))
335+
if not (status_code[:3].isdigit() and status_code.startswith("3")):
336+
blocked["status_code"] = "303"
337+
blocked[WAF_ACTIONS.TYPE] = "none"
338+
break
327339
else:
328340
blocked = {}
329341
_asm_request_context.set_waf_results(waf_results, self._ddwaf.info, bool(blocked))

ddtrace/contrib/django/patch.py

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -471,14 +471,21 @@ def blocked_response():
471471
from django.http import HttpResponse
472472

473473
block_config = core.get_item(WAF_CONTEXT_NAMES.BLOCKED, span=span)
474-
if block_config.get("type", "auto") == "auto":
475-
ctype = "text/html" if "text/html" in request_headers.get("Accept", "").lower() else "text/json"
476-
else:
477-
ctype = "text/" + block_config["type"]
474+
desired_type = block_config.get("type", "auto")
478475
status = block_config.get("status_code", 403)
479-
content = appsec_utils._get_blocked_template(ctype)
480-
response = HttpResponse(content, content_type=ctype, status=status)
481-
response.content = content
476+
if desired_type == "none":
477+
response = HttpResponse("", status=status)
478+
location = block_config.get("location", "")
479+
if location:
480+
response["location"] = location
481+
else:
482+
if desired_type == "auto":
483+
ctype = "text/html" if "text/html" in request_headers.get("Accept", "").lower() else "text/json"
484+
else:
485+
ctype = "text/" + desired_type
486+
content = appsec_utils._get_blocked_template(ctype)
487+
response = HttpResponse(content, content_type=ctype, status=status)
488+
response.content = content
482489
utils._after_request_tags(pin, span, request, response)
483490
return response
484491

ddtrace/contrib/flask/patch.py

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,12 +111,16 @@ def _wrapped_start_response(self, start_response, ctx, status_code, headers, exc
111111
if core.get_item(HTTP_REQUEST_BLOCKED):
112112
# response code must be set here, or it will be too late
113113
block_config = core.get_item(HTTP_REQUEST_BLOCKED)
114-
if block_config.get("type", "auto") == "auto":
115-
ctype = "text/html" if "text/html" in headers_from_context else "text/json"
116-
else:
117-
ctype = "text/" + block_config["type"]
114+
desired_type = block_config.get("type", "auto")
118115
status = block_config.get("status_code", 403)
119-
response_headers = [("content-type", ctype)]
116+
if desired_type == "none":
117+
response_headers = []
118+
else:
119+
if block_config.get("type", "auto") == "auto":
120+
ctype = "text/html" if "text/html" in headers_from_context else "text/json"
121+
else:
122+
ctype = "text/" + block_config["type"]
123+
response_headers = [("content-type", ctype)]
120124
result = start_response(str(status), response_headers)
121125
core.dispatch("flask.start_response.blocked", config.flask, response_headers, status)
122126
else:

ddtrace/contrib/wsgi/wsgi.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,14 +99,14 @@ def __call__(self, environ, start_response):
9999
middleware=self,
100100
) as ctx:
101101
if core.get_item(HTTP_REQUEST_BLOCKED):
102-
status, ctype, content = core.dispatch("wsgi.block.started", ctx, construct_url)[0][0]
103-
start_response(str(status), [("content-type", ctype)])
102+
status, headers, content = core.dispatch("wsgi.block.started", ctx, construct_url)[0][0]
103+
start_response(str(status), headers)
104104
closing_iterator = [content]
105105
not_blocked = False
106106

107107
def blocked_view():
108-
status, ctype, content = core.dispatch("wsgi.block.started", ctx, construct_url)[0][0]
109-
return content, status, [("content-type", ctype)]
108+
status, headers, content = core.dispatch("wsgi.block.started", ctx, construct_url)[0][0]
109+
return content, status, headers
110110

111111
core.dispatch("wsgi.block_decided", blocked_view)
112112

ddtrace/tracing/trace_handlers.py

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,23 @@ def _make_block_content(ctx, construct_url):
9191
if req_span is None:
9292
raise ValueError("request span not found")
9393
block_config = core.get_item(HTTP_REQUEST_BLOCKED, span=req_span)
94-
if block_config.get("type", "auto") == "auto":
95-
ctype = "text/html" if "text/html" in headers.get("Accept", "").lower() else "text/json"
94+
desired_type = block_config.get("type", "auto")
95+
ctype = None
96+
if desired_type == "none":
97+
content = ""
98+
resp_headers = [("content-type", "text/plain; charset=utf-8"), ("location", block_config.get("location", ""))]
9699
else:
97-
ctype = "text/" + block_config["type"]
98-
content = http_utils._get_blocked_template(ctype).encode("UTF-8")
100+
if desired_type == "auto":
101+
ctype = "text/html" if "text/html" in headers.get("Accept", "").lower() else "text/json"
102+
else:
103+
ctype = "text/" + block_config["type"]
104+
content = http_utils._get_blocked_template(ctype).encode("UTF-8")
105+
resp_headers = [("content-type", ctype)]
99106
status = block_config.get("status_code", 403)
100107
try:
101108
req_span.set_tag_str(RESPONSE_HEADERS + ".content-length", str(len(content)))
102-
req_span.set_tag_str(RESPONSE_HEADERS + ".content-type", ctype)
109+
if ctype is not None:
110+
req_span.set_tag_str(RESPONSE_HEADERS + ".content-type", ctype)
103111
req_span.set_tag_str(http.STATUS_CODE, str(status))
104112
url = construct_url(environ)
105113
query_string = environ.get("QUERY_STRING")
@@ -115,7 +123,7 @@ def _make_block_content(ctx, construct_url):
115123
except Exception as e:
116124
log.warning("Could not set some span tags on blocked request: %s", str(e)) # noqa: G200
117125

118-
return status, ctype, content
126+
return status, resp_headers, content
119127

120128

121129
def _on_request_prepare(ctx, start_response):
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
features:
3+
- |
4+
ASM: This introduces support for custom blocking actions of type redirect_request.

tests/appsec/rules-suspicious-requests-custom-actions.json

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,21 @@
2424
"status_code": 503,
2525
"type": "html"
2626
}
27+
},
28+
{
29+
"id": "block_301",
30+
"type": "redirect_request",
31+
"parameters": {
32+
"status_code": 301,
33+
"location": "https://www.datadoghq.com"
34+
}
35+
},
36+
{
37+
"id": "block_303",
38+
"type": "redirect_request",
39+
"parameters": {
40+
"location": "https://www.datadoghq.com"
41+
}
2742
}
2843
],
2944
"rules": [
@@ -104,6 +119,58 @@
104119
"on_match": [
105120
"block_503_html"
106121
]
122+
},
123+
{
124+
"id": "tst-040-004",
125+
"name": "Test block on query parameter with custom code 301",
126+
"tags": {
127+
"type": "lfi",
128+
"crs_id": "040003",
129+
"category": "attack_attempt"
130+
},
131+
"conditions": [
132+
{
133+
"parameters": {
134+
"inputs": [
135+
{
136+
"address": "server.request.query"
137+
}
138+
],
139+
"regex": "suspicious_301"
140+
},
141+
"operator": "match_regex"
142+
}
143+
],
144+
"transformers": [],
145+
"on_match": [
146+
"block_301"
147+
]
148+
},
149+
{
150+
"id": "tst-040-005",
151+
"name": "Test block on query parameter with custom code 303",
152+
"tags": {
153+
"type": "lfi",
154+
"crs_id": "040003",
155+
"category": "attack_attempt"
156+
},
157+
"conditions": [
158+
{
159+
"parameters": {
160+
"inputs": [
161+
{
162+
"address": "server.request.query"
163+
}
164+
],
165+
"regex": "suspicious_303"
166+
},
167+
"operator": "match_regex"
168+
}
169+
],
170+
"transformers": [],
171+
"on_match": [
172+
"block_303"
173+
]
107174
}
108175
]
109176
}

tests/contrib/django/test_django_appsec.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -856,6 +856,31 @@ def test_request_suspicious_request_block_custom_actions(client, test_spans, tra
856856
http._JSON_BLOCKED_TEMPLATE_CACHE = None
857857

858858

859+
@pytest.mark.parametrize(
860+
["suspicious_value", "expected_code", "rule"],
861+
[
862+
("suspicious_301", 301, "tst-040-004"),
863+
("suspicious_303", 303, "tst-040-005"),
864+
],
865+
)
866+
def test_request_suspicious_request_redirect_actions(client, test_spans, tracer, suspicious_value, expected_code, rule):
867+
# value suspicious_306_auto must be blocked
868+
with override_global_config(dict(_appsec_enabled=True)), override_env(
869+
dict(
870+
DD_APPSEC_RULES=RULES_SRBCA,
871+
)
872+
):
873+
root_span, response = _aux_appsec_get_root_span(
874+
client, test_spans, tracer, url="index.html?toto=%s" % suspicious_value
875+
)
876+
assert response.status_code == expected_code
877+
# check if response content is custom as expected
878+
assert not response.content
879+
assert response["location"] == "https://www.datadoghq.com"
880+
loaded = json.loads(root_span.get_tag(APPSEC.JSON))
881+
assert [t["rule"]["id"] for t in loaded["triggers"]] == [rule]
882+
883+
859884
@pytest.mark.django_db
860885
def test_django_login_events_disabled_explicitly(client, test_spans, tracer):
861886
from django.contrib.auth import get_user

tests/contrib/flask/test_flask_appsec.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -867,3 +867,61 @@ def test_route():
867867
# remove cache to avoid using template from other tests
868868
uhttp._HTML_BLOCKED_TEMPLATE_CACHE = None
869869
uhttp._JSON_BLOCKED_TEMPLATE_CACHE = None
870+
871+
def test_request_suspicious_request_block_redirect_actions_301(self):
872+
@self.app.route("/index.html")
873+
def test_route():
874+
return "Ok: %s" % request.args.get("toto", ""), 200
875+
876+
# value suspicious_301 must be redirected
877+
with override_global_config(dict(_appsec_enabled=True)), override_env(
878+
dict(
879+
DD_APPSEC_RULES=RULES_SRBCA,
880+
DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON=RESPONSE_CUSTOM_JSON,
881+
DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML=RESPONSE_CUSTOM_HTML,
882+
)
883+
):
884+
self._aux_appsec_prepare_tracer()
885+
resp = self.client.get("/index.html?toto=suspicious_301")
886+
assert resp.status_code == 301
887+
assert resp.headers["location"] == "https://www.datadoghq.com"
888+
assert not get_response_body(resp)
889+
root_span = self.pop_spans()[0]
890+
loaded = json.loads(root_span.get_tag(APPSEC.JSON))
891+
assert [t["rule"]["id"] for t in loaded["triggers"]] == ["tst-040-004"]
892+
assert root_span.get_tag(http.STATUS_CODE) == "301"
893+
assert root_span.get_tag(http.URL) == "http://localhost/index.html?toto=suspicious_301"
894+
assert root_span.get_tag(http.METHOD) == "GET"
895+
assert root_span.get_tag(http.USER_AGENT).startswith("werkzeug/")
896+
assert root_span.get_tag(SPAN_DATA_NAMES.RESPONSE_HEADERS_NO_COOKIES + ".content-type").startswith(
897+
"text/plain"
898+
)
899+
900+
def test_request_suspicious_request_block_redirect_actions_303(self):
901+
@self.app.route("/index.html")
902+
def test_route():
903+
return "Ok: %s" % request.args.get("toto", ""), 200
904+
905+
# value suspicious_301 must be redirected
906+
with override_global_config(dict(_appsec_enabled=True)), override_env(
907+
dict(
908+
DD_APPSEC_RULES=RULES_SRBCA,
909+
DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON=RESPONSE_CUSTOM_JSON,
910+
DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML=RESPONSE_CUSTOM_HTML,
911+
)
912+
):
913+
self._aux_appsec_prepare_tracer()
914+
resp = self.client.get("/index.html?toto=suspicious_303")
915+
assert resp.status_code == 303
916+
assert resp.headers["location"] == "https://www.datadoghq.com"
917+
assert not get_response_body(resp)
918+
root_span = self.pop_spans()[0]
919+
loaded = json.loads(root_span.get_tag(APPSEC.JSON))
920+
assert [t["rule"]["id"] for t in loaded["triggers"]] == ["tst-040-005"]
921+
assert root_span.get_tag(http.STATUS_CODE) == "303"
922+
assert root_span.get_tag(http.URL) == "http://localhost/index.html?toto=suspicious_303"
923+
assert root_span.get_tag(http.METHOD) == "GET"
924+
assert root_span.get_tag(http.USER_AGENT).startswith("werkzeug/")
925+
assert root_span.get_tag(SPAN_DATA_NAMES.RESPONSE_HEADERS_NO_COOKIES + ".content-type").startswith(
926+
"text/plain"
927+
)

0 commit comments

Comments
 (0)