Skip to content

Commit cf53953

Browse files
asheshvasheshv
committed
fix: prevent SQL injection in Maintenance tool option values (#9898)
The Maintenance tool concatenated four user-supplied JSON fields (buffer_usage_limit, vacuum_parallel, vacuum_index_cleanup, reindex_tablespace) directly into the rendered VACUUM/ANALYZE/REINDEX command, which was passed to psql --command. An authenticated user with the tools_maintenance permission could break out of the option syntax and execute arbitrary SQL on the connected server, escalating to RCE on the database host via COPY ... TO PROGRAM. Fix: - Allow-list values server-side before rendering: vacuum_index_cleanup in {AUTO, ON, OFF}; vacuum_parallel a non-negative integer up to 1024; buffer_usage_limit matching ^\d+\s*(kB|MB|GB|TB)$ (case-insensitive). Reject with HTTP 400 on failure. - Switch reindex_tablespace from manual double-quote wrapping to the qtIdent filter used elsewhere in the same template, which both escapes embedded quotes and only quotes when required. - Type-guard against unhashable values so non-string payloads return 400 instead of triggering a 500. Reported-by: j3seer <jasserchebbi@outlook.com>
1 parent d112dc3 commit cf53953

4 files changed

Lines changed: 299 additions & 4 deletions

File tree

‎web/pgadmin/tools/maintenance/__init__.py‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
"""A blueprint module implementing the maintenance tool for vacuum"""
1111

1212
import json
13+
import re
1314

1415
from flask import Response, render_template, request, current_app
1516
from flask_babel import gettext as _
@@ -148,6 +149,49 @@ def get_index_name(data):
148149
return index_name
149150

150151

152+
# Values for these options are concatenated into the rendered SQL command.
153+
# They MUST be allow-listed here — anything not matching is rejected before
154+
# rendering. reindex_tablespace is escaped via qtIdent in the template, so it
155+
# is not validated here beyond a type check.
156+
_INDEX_CLEANUP_ALLOWED = {'AUTO', 'ON', 'OFF'}
157+
_BUFFER_USAGE_LIMIT_RE = re.compile(r'^\d+\s*(kB|MB|GB|TB)$', re.IGNORECASE)
158+
_VACUUM_PARALLEL_MAX = 1024
159+
160+
161+
def validate_maintenance_data(data):
162+
"""
163+
Validate user-supplied option values that are concatenated into the
164+
maintenance SQL template. Returns an error message on failure, or None
165+
if all values are acceptable.
166+
"""
167+
index_cleanup = data.get('vacuum_index_cleanup')
168+
if index_cleanup:
169+
if not isinstance(index_cleanup, str) or \
170+
index_cleanup not in _INDEX_CLEANUP_ALLOWED:
171+
return _("Invalid value for INDEX_CLEANUP option.")
172+
173+
parallel = data.get('vacuum_parallel')
174+
if parallel not in (None, ''):
175+
try:
176+
n = int(parallel)
177+
except (TypeError, ValueError):
178+
return _("Invalid value for PARALLEL option.")
179+
if n < 0 or n > _VACUUM_PARALLEL_MAX:
180+
return _("Invalid value for PARALLEL option.")
181+
182+
buffer_limit = data.get('buffer_usage_limit')
183+
if buffer_limit:
184+
if not isinstance(buffer_limit, str) or \
185+
not _BUFFER_USAGE_LIMIT_RE.match(buffer_limit.strip()):
186+
return _("Invalid value for BUFFER_USAGE_LIMIT option.")
187+
188+
tablespace = data.get('reindex_tablespace')
189+
if tablespace is not None and not isinstance(tablespace, str):
190+
return _("Invalid value for TABLESPACE option.")
191+
192+
return None
193+
194+
151195
@blueprint.route(
152196
'/job/<int:sid>/<int:did>', methods=['POST'], endpoint='create_job'
153197
)
@@ -169,6 +213,10 @@ def create_maintenance_job(sid, did):
169213
else:
170214
data = json.loads(request.data)
171215

216+
validation_error = validate_maintenance_data(data)
217+
if validation_error is not None:
218+
return bad_request(errormsg=validation_error)
219+
172220
index_name = get_index_name(data)
173221

174222
# Fetch the server details like hostname, port, roles etc

‎web/pgadmin/tools/maintenance/templates/maintenance/sql/command.sql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
{% if data.vacuum_index_cleanup %}{{ maintenance_options.append('INDEX_CLEANUP ' + data.vacuum_index_cleanup) or "" }}{% endif %}
1414
{% if data.vacuum_parallel %}{{ maintenance_options.append('PARALLEL ' + data.vacuum_parallel) or "" }}{% endif %}
1515
{% if data.buffer_usage_limit %}{{ maintenance_options.append('BUFFER_USAGE_LIMIT "' + data.buffer_usage_limit + '"') or "" }}{% endif %}
16-
{% if data.reindex_tablespace %}{{ maintenance_options.append('TABLESPACE "' + data.reindex_tablespace + '"') or "" }}{% endif %}
16+
{% if data.reindex_tablespace %}{{ maintenance_options.append('TABLESPACE ' + conn|qtIdent(data.reindex_tablespace)) or "" }}{% endif %}
1717
{% if data.reindex_concurrently %}{{ maintenance_options.append('CONCURRENTLY') or "" }}{% endif %}
1818
{% if data.op == "VACUUM" %}
1919
VACUUM{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ option }}{% if not loop.last %}, {% endif %}{% if loop.last %}){% endif %}{% endfor %}{% if data.schema %} {{ conn|qtIdent(data.schema) }}.{{ conn|qtIdent(data.table) }}{% endif %};

‎web/pgadmin/tools/maintenance/tests/test_maintenance_create_job_unit_test.py‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,30 @@ class MaintenanceCreateJobTest(BaseTestGenerator):
559559
verbose=True
560560
),
561561
url=MAINTENANCE_URL,
562-
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE "pg_default") '
562+
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE pg_default) '
563+
'DATABASE postgres;\n'],
564+
server_min_version=140000,
565+
message='REINDEX TABLESPACE is not supported by EPAS/PG server '
566+
'less than 14.0'
567+
)),
568+
('When reindex_tablespace contains quote, qtIdent escapes it',
569+
dict(
570+
class_params=dict(
571+
sid=1,
572+
name='test_maintenance_server',
573+
port=5444,
574+
host='localhost',
575+
username='postgres'
576+
),
577+
params=dict(
578+
database='postgres',
579+
op='REINDEX',
580+
reindex_tablespace='foo"; DROP TABLE x; --',
581+
verbose=True
582+
),
583+
url=MAINTENANCE_URL,
584+
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE '
585+
'"foo""; DROP TABLE x; --") '
563586
'DATABASE postgres;\n'],
564587
server_min_version=140000,
565588
message='REINDEX TABLESPACE is not supported by EPAS/PG server '
@@ -644,7 +667,7 @@ class MaintenanceCreateJobTest(BaseTestGenerator):
644667
verbose=True
645668
),
646669
url=MAINTENANCE_URL,
647-
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE "pg_default") '
670+
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE pg_default) '
648671
'TABLE my_schema.my_table;\n'],
649672
server_min_version=140000,
650673
message='REINDEX TABLESPACE TABLE is not supported by '
@@ -711,7 +734,7 @@ class MaintenanceCreateJobTest(BaseTestGenerator):
711734
verbose=True
712735
),
713736
url=MAINTENANCE_URL,
714-
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE "pg_default") '
737+
expected_cmd_opts=['REINDEX (VERBOSE, TABLESPACE pg_default) '
715738
'INDEX my_schema.my_index;\n'],
716739
server_min_version=140000,
717740
message='REINDEX TABLESPACE is not supported by EPAS/PG server '

‎web/pgadmin/tools/maintenance/tests/test_maintenance_input_validation.py‎

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
##########################################################################
2+
#
3+
# pgAdmin 4 - PostgreSQL Tools
4+
#
5+
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
6+
# This software is released under the PostgreSQL Licence
7+
#
8+
##########################################################################
9+
10+
import json
11+
12+
from pgadmin.utils.route import BaseTestGenerator
13+
from regression import parent_node_dict
14+
from unittest.mock import patch, MagicMock
15+
16+
from config import PG_DEFAULT_DRIVER
17+
18+
MAINTENANCE_URL = '/maintenance/job/{0}/{1}'
19+
20+
21+
class MaintenanceInputValidationTest(BaseTestGenerator):
22+
"""
23+
Reject malicious / out-of-range values for the four maintenance options
24+
that get concatenated into the rendered SQL command. These payloads
25+
correspond to the SQL-injection PoC patterns; the endpoint must return
26+
HTTP 400 and never reach BatchProcess.
27+
"""
28+
29+
scenarios = [
30+
('buffer_usage_limit with SQL-injection payload is rejected',
31+
dict(
32+
params=dict(
33+
database='postgres',
34+
op='ANALYZE',
35+
buffer_usage_limit='256kB"); COPY (SELECT 1) TO PROGRAM '
36+
"'id'; --",
37+
verbose=True,
38+
),
39+
)),
40+
('buffer_usage_limit with bad unit is rejected',
41+
dict(
42+
params=dict(
43+
database='postgres',
44+
op='VACUUM',
45+
buffer_usage_limit='1XB',
46+
verbose=True,
47+
),
48+
)),
49+
('buffer_usage_limit non-string is rejected',
50+
dict(
51+
params=dict(
52+
database='postgres',
53+
op='VACUUM',
54+
buffer_usage_limit=12345,
55+
verbose=True,
56+
),
57+
)),
58+
('vacuum_parallel with SQL-injection payload is rejected',
59+
dict(
60+
params=dict(
61+
database='postgres',
62+
op='VACUUM',
63+
vacuum_parallel='0); COPY (SELECT 1) TO PROGRAM \'id\'; --',
64+
verbose=True,
65+
),
66+
)),
67+
('vacuum_parallel negative is rejected',
68+
dict(
69+
params=dict(
70+
database='postgres',
71+
op='VACUUM',
72+
vacuum_parallel='-1',
73+
verbose=True,
74+
),
75+
)),
76+
('vacuum_parallel above max is rejected',
77+
dict(
78+
params=dict(
79+
database='postgres',
80+
op='VACUUM',
81+
vacuum_parallel='99999',
82+
verbose=True,
83+
),
84+
)),
85+
('vacuum_index_cleanup with SQL-injection payload is rejected',
86+
dict(
87+
params=dict(
88+
database='postgres',
89+
op='VACUUM',
90+
vacuum_index_cleanup='OFF); COPY (SELECT 1) TO PROGRAM '
91+
"'id'; ANALYZE (VERBOSE",
92+
verbose=True,
93+
),
94+
)),
95+
('vacuum_index_cleanup with unknown value is rejected',
96+
dict(
97+
params=dict(
98+
database='postgres',
99+
op='VACUUM',
100+
vacuum_index_cleanup='MAYBE',
101+
verbose=True,
102+
),
103+
)),
104+
('vacuum_index_cleanup lowercase is rejected',
105+
dict(
106+
params=dict(
107+
database='postgres',
108+
op='VACUUM',
109+
vacuum_index_cleanup='off',
110+
verbose=True,
111+
),
112+
)),
113+
('reindex_tablespace non-string is rejected',
114+
dict(
115+
params=dict(
116+
database='postgres',
117+
op='REINDEX',
118+
reindex_tablespace=['pg_default'],
119+
verbose=True,
120+
),
121+
)),
122+
('vacuum_index_cleanup as list is rejected (no 500)',
123+
dict(
124+
params=dict(
125+
database='postgres',
126+
op='VACUUM',
127+
vacuum_index_cleanup=['OFF'],
128+
verbose=True,
129+
),
130+
)),
131+
('vacuum_index_cleanup as dict is rejected (no 500)',
132+
dict(
133+
params=dict(
134+
database='postgres',
135+
op='VACUUM',
136+
vacuum_index_cleanup={'$ne': None},
137+
verbose=True,
138+
),
139+
)),
140+
]
141+
142+
@patch('pgadmin.tools.maintenance.BatchProcess')
143+
@patch('pgadmin.utils.driver.{0}.server_manager.ServerManager.'
144+
'export_password_env'.format(PG_DEFAULT_DRIVER))
145+
def runTest(self, export_password_env_mock, batch_process_mock):
146+
self.server_id = parent_node_dict["database"][-1]["server_id"]
147+
self.db_id = parent_node_dict["database"][-1]["db_id"]
148+
url = MAINTENANCE_URL.format(self.server_id, self.db_id)
149+
150+
batch_process_mock.return_value.id = 1
151+
batch_process_mock.return_value.set_env_variables = MagicMock(
152+
return_value=True)
153+
batch_process_mock.return_value.start = MagicMock(return_value=True)
154+
export_password_env_mock.return_value = True
155+
156+
response = self.tester.post(url,
157+
data=json.dumps(self.params),
158+
content_type='html/json')
159+
160+
self.assertEqual(response.status_code, 400)
161+
# Validation must reject the request before BatchProcess is touched.
162+
self.assertFalse(batch_process_mock.called)
163+
164+
165+
class MaintenanceInputValidationAcceptsValidTest(BaseTestGenerator):
166+
"""
167+
Confirm that legitimate option values still pass validation. Anything
168+
that the UI can produce should reach BatchProcess.
169+
"""
170+
171+
scenarios = [
172+
('vacuum_index_cleanup AUTO is accepted',
173+
dict(params=dict(database='postgres', op='VACUUM',
174+
vacuum_index_cleanup='AUTO', verbose=True))),
175+
('buffer_usage_limit with space is accepted',
176+
dict(params=dict(database='postgres', op='ANALYZE',
177+
buffer_usage_limit='128 kB', verbose=True))),
178+
('buffer_usage_limit case-insensitive is accepted',
179+
dict(params=dict(database='postgres', op='VACUUM',
180+
buffer_usage_limit='1mb', verbose=True))),
181+
('vacuum_parallel zero is accepted',
182+
dict(params=dict(database='postgres', op='VACUUM',
183+
vacuum_parallel='0', verbose=True))),
184+
('vacuum_parallel max is accepted',
185+
dict(params=dict(database='postgres', op='VACUUM',
186+
vacuum_parallel='1024', verbose=True))),
187+
]
188+
189+
@patch('pgadmin.tools.maintenance.Server')
190+
@patch('pgadmin.tools.maintenance.Message')
191+
@patch('pgadmin.tools.maintenance.BatchProcess')
192+
@patch('pgadmin.utils.driver.{0}.server_manager.ServerManager.'
193+
'export_password_env'.format(PG_DEFAULT_DRIVER))
194+
def runTest(self, export_password_env_mock, batch_process_mock,
195+
message_mock, server_mock):
196+
self.server_id = parent_node_dict["database"][-1]["server_id"]
197+
self.db_id = parent_node_dict["database"][-1]["db_id"]
198+
url = MAINTENANCE_URL.format(self.server_id, self.db_id)
199+
200+
class TestMockServer:
201+
def __init__(self, host, port, sid, username):
202+
self.host = host
203+
self.port = port
204+
self.id = sid
205+
self.username = username
206+
207+
mock_obj = TestMockServer('localhost', 5444, self.server_id,
208+
'postgres')
209+
server_mock.query.filter_by.return_value.first.return_value = mock_obj
210+
211+
batch_process_mock.return_value.id = 140391
212+
batch_process_mock.return_value.set_env_variables = MagicMock(
213+
return_value=True)
214+
batch_process_mock.return_value.start = MagicMock(return_value=True)
215+
message_mock.message = 'test'
216+
batch_process_mock.return_value.desc = message_mock
217+
export_password_env_mock.return_value = True
218+
219+
response = self.tester.post(url,
220+
data=json.dumps(self.params),
221+
content_type='html/json')
222+
223+
self.assertEqual(response.status_code, 200)
224+
self.assertTrue(batch_process_mock.called)

0 commit comments

Comments
 (0)
⚡