Skip to content

Commit 599ae62

Browse files
roggenkemperroggenkemper
authored
fix(detectors): Add more targeted searching for SQL Injection Detector (#93720)
this pr addresses 2 problems: 1) it replaces the regex match instead of the string match, this could've replaced parts of the string that weren't actually matching 2) limits the search to only the portion after "where" in the sql statement. That's where any filters would be applied and will prevent any false positives that might come from matches in the first part of the statement
1 parent afaee4a commit 599ae62

File tree

3 files changed

+173
-3
lines changed

3 files changed

+173
-3
lines changed

‎fixtures/events/performance_problems/sql-injection/sql-injection-not-in-where-event.json

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
{
2+
"event_id": "5d6401994d7949d2ac3474f472564370",
3+
"platform": "node",
4+
"message": "",
5+
"datetime": "2025-05-12T22:42:38.642986+00:00",
6+
"breakdowns": {
7+
"span_ops": {
8+
"ops.db": {
9+
"value": 65.715075,
10+
"unit": "millisecond"
11+
},
12+
"total.time": {
13+
"value": 67.105293,
14+
"unit": "millisecond"
15+
}
16+
}
17+
},
18+
"request": {
19+
"url": "http://localhost:3001/vulnerable-login",
20+
"method": "GET",
21+
"query_string": [["type", "account"]]
22+
},
23+
"spans": [
24+
{
25+
"timestamp": 1747089758.567536,
26+
"start_timestamp": 1747089758.567,
27+
"exclusive_time": 0.536203,
28+
"op": "middleware.express",
29+
"span_id": "4a06692f4abc8dbe",
30+
"parent_span_id": "91fa92ff0205967d",
31+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
32+
"status": "ok",
33+
"description": "corsMiddleware",
34+
"origin": "auto.http.otel.express",
35+
"data": {
36+
"express.name": "corsMiddleware",
37+
"express.type": "middleware",
38+
"sentry.op": "middleware.express",
39+
"sentry.origin": "auto.http.otel.express"
40+
},
41+
"sentry_tags": {
42+
"user": "ip:::1",
43+
"user.ip": "::1",
44+
"environment": "production",
45+
"transaction": "GET /vulnerable-login",
46+
"transaction.method": "GET",
47+
"transaction.op": "http.server",
48+
"browser.name": "Chrome",
49+
"sdk.name": "sentry.javascript.node",
50+
"sdk.version": "9.17.0",
51+
"platform": "node",
52+
"os.name": "macOS",
53+
"category": "middleware",
54+
"op": "middleware.express",
55+
"status": "ok",
56+
"trace.status": "ok"
57+
},
58+
"hash": "e6088cf8b370ed60"
59+
},
60+
{
61+
"timestamp": 1747089758.568761,
62+
"start_timestamp": 1747089758.568,
63+
"exclusive_time": 0.761032,
64+
"op": "middleware.express",
65+
"span_id": "92553d2584d250b8",
66+
"parent_span_id": "91fa92ff0205967d",
67+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
68+
"status": "ok",
69+
"description": "jsonParser",
70+
"origin": "auto.http.otel.express",
71+
"data": {
72+
"express.name": "jsonParser",
73+
"express.type": "middleware",
74+
"sentry.op": "middleware.express",
75+
"sentry.origin": "auto.http.otel.express"
76+
},
77+
"sentry_tags": {
78+
"user": "ip:::1",
79+
"user.ip": "::1",
80+
"environment": "production",
81+
"transaction": "GET /vulnerable-login",
82+
"transaction.method": "GET",
83+
"transaction.op": "http.server",
84+
"browser.name": "Chrome",
85+
"sdk.name": "sentry.javascript.node",
86+
"sdk.version": "9.17.0",
87+
"platform": "node",
88+
"os.name": "macOS",
89+
"category": "middleware",
90+
"op": "middleware.express",
91+
"status": "ok",
92+
"trace.status": "ok"
93+
},
94+
"hash": "c81e963dad9ebc6c"
95+
},
96+
{
97+
"timestamp": 1747089758.569093,
98+
"start_timestamp": 1747089758.569,
99+
"exclusive_time": 0.092983,
100+
"op": "request_handler.express",
101+
"span_id": "435146ab0909419d",
102+
"parent_span_id": "91fa92ff0205967d",
103+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
104+
"status": "ok",
105+
"description": "/vulnerable-login",
106+
"origin": "auto.http.otel.express",
107+
"data": {
108+
"express.name": "/vulnerable-login",
109+
"express.type": "request_handler",
110+
"http.route": "/vulnerable-login",
111+
"sentry.op": "request_handler.express",
112+
"sentry.origin": "auto.http.otel.express"
113+
},
114+
"sentry_tags": {
115+
"user": "ip:::1",
116+
"user.ip": "::1",
117+
"environment": "production",
118+
"transaction": "GET /vulnerable-login",
119+
"transaction.method": "GET",
120+
"transaction.op": "http.server",
121+
"browser.name": "Chrome",
122+
"sdk.name": "sentry.javascript.node",
123+
"sdk.version": "9.17.0",
124+
"platform": "node",
125+
"os.name": "macOS",
126+
"op": "request_handler.express",
127+
"status": "ok",
128+
"trace.status": "ok"
129+
},
130+
"hash": "872b0c84a6f1c590"
131+
},
132+
{
133+
"timestamp": 1747089758.637715,
134+
"start_timestamp": 1747089758.572,
135+
"exclusive_time": 65.715075,
136+
"op": "db",
137+
"span_id": "4703181ac343f71a",
138+
"parent_span_id": "91fa92ff0205967d",
139+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
140+
"status": "ok",
141+
"description": "SELECT \"company_account\".\"account\", \"company_account\".\"id\", \"company_account\".\"type\" FROM \"company_account\" WHERE \"company_account\".\"owner\" = ? ORDER BY \"company_account\".\"account\"",
142+
"origin": "auto.db.otel.mysql2",
143+
"data": {
144+
"db.system": "mysql",
145+
"db.connection_string": "jdbc:mysql://localhost:3306/injection_test",
146+
"db.name": "injection_test",
147+
"db.statement": "SELECT \"company_account\".\"account\", \"company_account\".\"id\", \"company_account\".\"type\" FROM \"company_account\" WHERE \"company_account\".\"owner\" = ? ORDER BY \"company_account\".\"account\"",
148+
"db.user": "root",
149+
"net.peer.name": "localhost",
150+
"net.peer.port": 3306,
151+
"otel.kind": "CLIENT",
152+
"sentry.op": "db",
153+
"sentry.origin": "auto.db.otel.mysql2"
154+
},
155+
"hash": "45330ba0cafa5997"
156+
}
157+
]
158+
}

‎src/sentry/performance_issues/detectors/sql_injection_detector.py

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,21 @@ def visit_span(self, span: Span) -> None:
119119
spans_involved = [span["span_id"]]
120120
vulnerable_parameters = []
121121

122+
if "WHERE" not in description.upper():
123+
return
124+
122125
for parameter in self.request_parameters:
123126
value = parameter[1]
124127
key = parameter[0]
125-
if re.search(rf'(?<![\w.])"?{re.escape(key)}"?(?![\w."])', description) and re.search(
126-
rf'(?<![\w.])"?{re.escape(value)}"?(?![\w."])', description
128+
regex_key = rf'(?<![\w.$])"?{re.escape(key)}"?(?![\w.$"])'
129+
regex_value = rf'(?<![\w.$])"?{re.escape(value)}"?(?![\w.$"])'
130+
where_index = description.upper().find("WHERE")
131+
if re.search(regex_key, description[where_index:]) and re.search(
132+
regex_value, description[where_index:]
127133
):
128-
description = description.replace(value, "?")
134+
description = description[:where_index] + re.sub(
135+
regex_value, "?", description[where_index:]
136+
)
129137
vulnerable_parameters.append(key)
130138

131139
if len(vulnerable_parameters) == 0:

‎tests/sentry/performance_issues/test_sql_injection_detector.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ def test_sql_injection_regex(self):
5858
injection_event = get_event("sql-injection/sql-injection-test-regex-event")
5959
assert len(self.find_problems(injection_event)) == 0
6060

61+
def test_sql_injection_not_in_where(self):
62+
injection_event = get_event("sql-injection/sql-injection-not-in-where-event")
63+
assert len(self.find_problems(injection_event)) == 0
64+
6165
def test_sql_injection_on_non_vulnerable_query(self):
6266
injection_event = get_event("sql-injection/sql-injection-event-non-vulnerable")
6367
assert len(self.find_problems(injection_event)) == 0

0 commit comments

Comments
 (0)