From ddf2e948d1f17c8fd36c2168ea02086edb3a2e81 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Sun, 26 Jul 2026 15:40:14 -0400 Subject: [PATCH] fix(security): enforce files.read on reads; de-shell syslog unit filter Follow-ups from the GHSA-4wqh-7f4f-5qmx / GHSA-mc93-rc3x-fpgq review: - File Manager read endpoints (browse/info/read/search/disk-*/analyze/ type-breakdown/download + s3/browse, s3/read, s3/download-url) only required a valid JWT, so a user whose files.read permission is revoked via custom permissions could still read and download files. They now require permission_required('files', 'read'), matching the write side. - LogService._read_syslog() interpolated the unit name into a bash -c string using subprocess.list2cmdline(), which is cmd.exe-style quoting and does not stop $(...)/backtick expansion under bash. grep now runs as a plain argv list (with --) and the tail happens in Python. Reachable only on non-systemd hosts by admins, but the same bug class as the Postfix advisory. Regression tests: revoked files.read 403s every read endpoint; the syslog filter never invokes a shell and passes payloads verbatim. --- backend/app/api/files.py | 25 ++++----- backend/app/services/log_service.py | 9 ++- backend/tests/test_files_rbac.py | 85 +++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 14 deletions(-) diff --git a/backend/app/api/files.py b/backend/app/api/files.py index b8da7aef..7e151aa3 100644 --- a/backend/app/api/files.py +++ b/backend/app/api/files.py @@ -1,7 +1,6 @@ """File Manager API endpoints for browsing, editing, and managing files.""" from flask import Blueprint, request, jsonify, send_file -from flask_jwt_extended import jwt_required from ..middleware.rbac import permission_required from ..services.file_service import FileService from ..services.storage_provider_service import StorageProviderService @@ -12,7 +11,7 @@ files_bp = Blueprint('files', __name__) @files_bp.route('/browse', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def browse_directory(): """List directory contents.""" path = request.args.get('path', '/home') @@ -26,7 +25,7 @@ def browse_directory(): @files_bp.route('/info', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def get_file_info(): """Get information about a file or directory.""" path = request.args.get('path') @@ -47,7 +46,7 @@ def get_file_info(): @files_bp.route('/read', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def read_file(): """Read file contents.""" path = request.args.get('path') @@ -254,7 +253,7 @@ def change_permissions(): @files_bp.route('/search', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def search_files(): """Search for files matching a pattern.""" directory = request.args.get('directory', '/home') @@ -274,7 +273,7 @@ def search_files(): @files_bp.route('/disk-usage', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def get_disk_usage(): """Get disk usage for a path.""" path = request.args.get('path', '/') @@ -287,7 +286,7 @@ def get_disk_usage(): @files_bp.route('/disk-mounts', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def get_disk_mounts(): """Get disk usage for all mount points.""" result = FileService.get_all_disk_mounts() @@ -298,7 +297,7 @@ def get_disk_mounts(): @files_bp.route('/analyze', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def analyze_directory(): """Analyze directory sizes.""" path = request.args.get('path', '/home') @@ -315,7 +314,7 @@ def analyze_directory(): @files_bp.route('/type-breakdown', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def get_type_breakdown(): """Get file type breakdown for a directory.""" path = request.args.get('path', '/home') @@ -331,7 +330,7 @@ def get_type_breakdown(): @files_bp.route('/download', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def download_file(): """Download a file.""" path = request.args.get('path') @@ -420,7 +419,7 @@ def upload_file(): # ── S3 / object-storage browser (reuses the configured backup storage creds) ── @files_bp.route('/s3/browse', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def s3_browse(): """List a bucket prefix in the same entry shape as the local browser.""" path = request.args.get('path', '/') @@ -429,7 +428,7 @@ def s3_browse(): @files_bp.route('/s3/read', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def s3_read(): """Read a text object for in-app editing.""" path = request.args.get('path') @@ -466,7 +465,7 @@ def s3_delete(): @files_bp.route('/s3/download-url', methods=['GET']) -@jwt_required() +@permission_required('files', 'read') def s3_download_url(): """Return a short-lived presigned URL the browser can download directly.""" path = request.args.get('path') diff --git a/backend/app/services/log_service.py b/backend/app/services/log_service.py index 96498112..5a265e6f 100644 --- a/backend/app/services/log_service.py +++ b/backend/app/services/log_service.py @@ -315,8 +315,11 @@ class LogService: """Read system logs from a syslog file, optionally filtering by service.""" try: if service: + # No shell: list2cmdline() is cmd.exe-style quoting and does + # not protect against $(...)/backtick expansion under bash. + # grep runs as argv and the tail happens in Python instead. result = run_privileged( - ['bash', '-c', f'grep -i {subprocess.list2cmdline([service])} {subprocess.list2cmdline([filepath])} | tail -n {int(lines)}'], + ['grep', '-i', '--', service, filepath], timeout=60, ) else: @@ -327,6 +330,10 @@ class LogService: if result.returncode == 0 or (service and result.returncode == 1): log_lines = result.stdout.split('\n') if result.stdout else [] + if log_lines and log_lines[-1] == '': + log_lines.pop() # trailing newline artifact + if service: + log_lines = log_lines[-int(lines):] return sourced_result(log_lines, 'syslog', filepath) else: return {'success': False, 'error': result.stderr} diff --git a/backend/tests/test_files_rbac.py b/backend/tests/test_files_rbac.py index ca17d32b..0cf3b4ce 100644 --- a/backend/tests/test_files_rbac.py +++ b/backend/tests/test_files_rbac.py @@ -8,6 +8,8 @@ GHSA-mc93-rc3x-fpgq — PostfixService.install() interpolated the hostname into a bash -c string. Hostnames are now validated and debconf values are piped via stdin, never through a shell. """ +import subprocess + import pytest from app import db as _db @@ -90,6 +92,89 @@ def test_viewer_can_still_browse(client, role_headers): assert resp.status_code != 403 +# --------------------------------------------------------------------------- # +# files.read revocation: a user whose files.read permission is disabled must +# be blocked from read endpoints too, not just writes +# --------------------------------------------------------------------------- # + +READ_CASES = [ + '/api/v1/files/browse?path=/tmp', + '/api/v1/files/info?path=/tmp/x', + '/api/v1/files/read?path=/tmp/x', + '/api/v1/files/search?path=/tmp&query=x', + '/api/v1/files/download?path=/tmp/x', + '/api/v1/files/s3/browse?path=/', + '/api/v1/files/s3/read?path=/x', + '/api/v1/files/s3/download-url?path=/x', +] + + +@pytest.fixture +def no_read_headers(app): + """JWT headers for a viewer whose files.read permission is revoked.""" + from flask_jwt_extended import create_access_token + from werkzeug.security import generate_password_hash + + with app.app_context(): + u = User(email='files_noread@t.local', username='files_noread', + password_hash=generate_password_hash('x'), + role=User.ROLE_VIEWER, is_active=True) + perms = {f: {'read': True, 'write': False} for f in User.PERMISSION_FEATURES} + perms['files'] = {'read': False, 'write': False} + u.set_permissions(perms) + _db.session.add(u) + _db.session.commit() + return {'Authorization': f'Bearer {create_access_token(identity=u.id)}'} + + +@pytest.mark.parametrize('url', READ_CASES) +def test_revoked_files_read_blocks_read_endpoints(client, no_read_headers, url): + resp = client.get(url, headers=no_read_headers) + assert resp.status_code == 403 + assert 'files' in resp.get_json()['error'] + + +# --------------------------------------------------------------------------- # +# log_service._read_syslog: the service filter must never go through a shell +# (subprocess.list2cmdline is cmd.exe quoting and does not stop $(...)/backtick +# expansion under bash) +# --------------------------------------------------------------------------- # + +def test_read_syslog_does_not_use_a_shell(monkeypatch): + from app.services import log_service + from app.services.log_service import LogService + + captured = {} + + def fake_run_privileged(cmd, **kwargs): + captured['cmd'] = cmd + return subprocess.CompletedProcess(cmd, 0, stdout='match1\nmatch2\n', stderr='') + + monkeypatch.setattr(log_service, 'run_privileged', fake_run_privileged) + + payload = 'x$(id)`id`' + result = LogService._read_syslog('/var/log/syslog', payload, 100) + + cmd = captured['cmd'] + assert 'bash' not in cmd and '-c' not in cmd + # the payload travels as a standalone argv element, verbatim + assert payload in cmd + assert result['success'] + + +def test_read_syslog_tails_matches_in_python(monkeypatch): + from app.services import log_service + from app.services.log_service import LogService + + def fake_run_privileged(cmd, **kwargs): + return subprocess.CompletedProcess(cmd, 0, stdout='l1\nl2\nl3\n', stderr='') + + monkeypatch.setattr(log_service, 'run_privileged', fake_run_privileged) + + result = LogService._read_syslog('/var/log/syslog', 'nginx', 2) + assert result['lines'] == ['l2', 'l3'] + + # --------------------------------------------------------------------------- # # GHSA-mc93-rc3x-fpgq: postfix install rejects malicious hostnames # --------------------------------------------------------------------------- #