diff --git a/scripts/utils/pixlet_config_editor.sh b/scripts/utils/pixlet_config_editor.sh index b0588e16..124a9c6f 100755 --- a/scripts/utils/pixlet_config_editor.sh +++ b/scripts/utils/pixlet_config_editor.sh @@ -104,6 +104,29 @@ PIXLET_BIN=$(find_pixlet) || { exit 1 } +# find_pixlet supports Darwin, so this script has to as well. macOS ships no +# timeout(1); GNU coreutils installs it as gtimeout. Resolve whichever exists +# and fail here with instructions rather than at the invocation far below, +# where the failure would land after the display has already been stopped. +find_timeout() { + local candidate + for candidate in timeout gtimeout; do + if command -v "$candidate" >/dev/null 2>&1; then + command -v "$candidate" + return 0 + fi + done + return 1 +} + +TIMEOUT_BIN=$(find_timeout) || { + echo "Neither 'timeout' nor 'gtimeout' was found on PATH." + echo "This script needs one to bound the editing session." + echo "On macOS, install GNU coreutils:" + echo " brew install coreutils" + exit 1 +} + CONFIG_FILE="$APP_DIR/config.json" if [ -f "$CONFIG_FILE" ]; then cp "$CONFIG_FILE" "$CONFIG_FILE.backup" @@ -148,11 +171,14 @@ if [ "$DISPLAY_WAS_RUNNING" = true ]; then sudo systemctl stop ledmatrix fi -if [ "$BIND_HOST" = "0.0.0.0" ]; then - REACH_HOST="$(hostname).local" -else - REACH_HOST="localhost" -fi +# Wildcard, loopback and an explicit interface address are three different +# cases. Collapsing the last two into "localhost" printed a URL pointing at the +# user's own machine whenever PIXLET_EDITOR_HOST named a LAN address. +case "$BIND_HOST" in + 0.0.0.0|::|"") REACH_HOST="$(hostname).local" ;; + 127.0.0.1|::1|localhost) REACH_HOST="localhost" ;; + *) REACH_HOST="$BIND_HOST" ;; +esac echo "" echo "Editing: $APP_ID" @@ -181,7 +207,7 @@ cd "$APP_DIR" # --foreground: stay in this script's process group so one signal reaches the # whole session. Backgrounded + `wait` so the EXIT trap can run while the child # is still alive; a foreground child would leave bash waiting on it instead. -timeout --foreground "$EDITOR_TIMEOUT" "$PIXLET_BIN" serve "$(basename "$STAR_FILE")" \ +"$TIMEOUT_BIN" --foreground "$EDITOR_TIMEOUT" "$PIXLET_BIN" serve "$(basename "$STAR_FILE")" \ --host "$BIND_HOST" \ --port "$PORT" \ --no-browser \ diff --git a/test/js/dom/test_tools_sections.js b/test/js/dom/test_tools_sections.js index 9325550c..4625f25d 100644 --- a/test/js/dom/test_tools_sections.js +++ b/test/js/dom/test_tools_sections.js @@ -86,6 +86,17 @@ const get = p => new Promise((res, rej) => ok('editor lists the apps on disk', appIds.every(id => $('pixlet-editor-body').textContent.includes(id)), appIds); ok('no session banner while idle', !$('pixlet-countdown')); + // escHtml does not encode single quotes, so an app id interpolated into an + // inline onclick="startPixletEditor('...')" could break out of the JS string + // and run script. The id must reach the handler through dataset instead. + const editBtns = [...window.document.querySelectorAll('[id^="btn-pixlet-edit-"]')]; + ok('edit buttons exist', editBtns.length > 0, editBtns.length); + ok('edit buttons carry no inline onclick', + editBtns.every(b => !b.getAttribute('onclick')), + editBtns.map(b => b.getAttribute('onclick'))); + ok('edit buttons pass the app id via dataset', + editBtns.every(b => appIds.includes(b.dataset.appId)), + editBtns.map(b => b.dataset.appId)); ok('warns that the display stops', window.document.body.textContent.includes('display stops while a session is open')); diff --git a/test/test_mqtt_bridge_config_endpoint.py b/test/test_mqtt_bridge_config_endpoint.py index 1d2ae97e..595e1c74 100644 --- a/test/test_mqtt_bridge_config_endpoint.py +++ b/test/test_mqtt_bridge_config_endpoint.py @@ -70,7 +70,10 @@ class TestABodyItCannotApplyIsRejected: class TestClearPasswordNeedsARealBoolean: def _seed(self, cfg, password="hunter2"): - cfg.write_text(json.dumps({"mqtt_password": password}), encoding="utf-8") + # TLS on so these cases exercise clear_password alone: a stored password + # with TLS off is refused by the CWE-319 guard, which is a separate test. + cfg.write_text(json.dumps({"mqtt_password": password, "mqtt_tls": True}), + encoding="utf-8") def _stored(self, cfg): return json.loads(cfg.read_text(encoding="utf-8")).get("mqtt_password") @@ -95,3 +98,41 @@ class TestClearPasswordNeedsARealBoolean: self._seed(cfg) assert c.put(URL, json={"clear_password": truthy}).status_code == 200 assert self._stored(cfg) is None + + +class TestCleartextCredentialsNeedAnExplicitOptIn: + """CWE-319. A password with TLS off crosses the network in the clear. + + Refused rather than forbidden: unencrypted MQTT on a trusted LAN is a normal + deliberate setup, so allow_insecure_mqtt is the explicit acknowledgement. + """ + + def test_a_password_without_tls_is_refused(self, client): + c, _ = client + r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": False}) + assert r.status_code == 400 + assert "allow_insecure_mqtt" in r.get_json()["message"] + + def test_the_opt_in_allows_it(self, client): + c, cfg = client + r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": False, + "allow_insecure_mqtt": True}) + assert r.status_code == 200, r.get_json() + assert json.loads(cfg.read_text(encoding="utf-8"))["mqtt_password"] == "hunter2" + + def test_tls_on_needs_no_opt_in(self, client): + c, _ = client + r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": True}) + assert r.status_code == 200, r.get_json() + + def test_no_password_is_unaffected(self, client): + c, _ = client + assert c.put(URL, json={"mqtt_tls": False}).status_code == 200 + + def test_the_opt_in_is_a_real_boolean(self, client): + """"false" must not switch the guard off, same as clear_password.""" + c, _ = client + r = c.put(URL, json={"mqtt_password": "hunter2", "mqtt_tls": False, + "allow_insecure_mqtt": "false"}) + assert r.status_code == 400 + diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 8d80e0af..bff6cf16 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -1945,6 +1945,9 @@ _MQTT_BRIDGE_DEFAULTS = { 'mqtt_topic': 'ledmatrix/command', 'mqtt_tls': False, 'mqtt_tls_insecure': False, + # Opt-in acknowledgement that credentials may cross an untrusted + # network in cleartext. Off by default: the save is refused instead. + 'allow_insecure_mqtt': False, 'ledmatrix_api_base': 'http://localhost:5000', 'request_timeout': 15, 'on_demand_duration': None, @@ -2015,7 +2018,7 @@ def _coerce_mqtt_bridge_value(key: str, raw: Any) -> Tuple[Any, Optional[str]]: if not 1 <= duration <= 86400: return None, 'On-demand duration must be between 1 and 86400 seconds' return duration, None - if key in ('mqtt_tls', 'mqtt_tls_insecure'): + if key in ('mqtt_tls', 'mqtt_tls_insecure', 'allow_insecure_mqtt'): return bool(raw) if isinstance(raw, bool) else str(raw).lower() in ('1', 'true', 'yes', 'on'), None if key == 'log_level': level = str(raw or '').upper() diff --git a/web_interface/blueprints/api_v3/misc.py b/web_interface/blueprints/api_v3/misc.py index ad88326c..5998d06d 100644 --- a/web_interface/blueprints/api_v3/misc.py +++ b/web_interface/blueprints/api_v3/misc.py @@ -472,9 +472,21 @@ def update_mqtt_bridge_config(): else: config['mqtt_password'] = existing_password - if config.get('mqtt_password') and not config.get('mqtt_tls'): - logger.warning('MQTT bridge: a password is set without TLS; ' - 'credentials will cross the network in cleartext') + # CWE-319: a password with TLS off is sent in the clear. On a trusted + # LAN that is a normal, deliberate setup, so this is refused rather + # than forbidden -- allow_insecure_mqtt is the explicit acknowledgement. + insecure = bool(config.get('mqtt_password')) and not config.get('mqtt_tls') + if insecure and not config.get('allow_insecure_mqtt'): + return jsonify({ + 'status': 'error', + 'message': 'MQTT credentials would cross the network in cleartext ' + 'with TLS disabled. Enable mqtt_tls, or set ' + 'allow_insecure_mqtt to accept that on a trusted network.' + }), 400 + if insecure: + logger.warning('MQTT bridge: a password is set without TLS and ' + 'allow_insecure_mqtt is on; credentials will cross the ' + 'network in cleartext') _MQTT_BRIDGE_DIR.mkdir(parents=True, exist_ok=True) # Write via a temp file in the same directory so a crash mid-write diff --git a/web_interface/blueprints/api_v3/starlark.py b/web_interface/blueprints/api_v3/starlark.py index 98c0f9c5..9e6f3fde 100644 --- a/web_interface/blueprints/api_v3/starlark.py +++ b/web_interface/blueprints/api_v3/starlark.py @@ -4,6 +4,7 @@ Routes decorate the shared `api_v3` Blueprint from ._common, so their endpoint names are unchanged by living here. """ import signal +import threading from web_interface.blueprints.api_v3 import ( PROJECT_ROOT, Path, _PIXLET_EDITOR_DEFAULT_PORT, @@ -771,6 +772,28 @@ def get_pixlet_editor_status(): return jsonify({'status': 'error', 'message': 'Could not read editor status', 'details': describe_exception(e)}), 500 +#: Flask runs threaded in the supported service, so two start requests can each +#: observe running=False, each launch an editor, and the second state write +#: replace the first PID -- orphaning a process that holds the display down with +#: nothing left recording it. The check-launch-write sequence takes this lock. +_EDITOR_START_LOCK = threading.Lock() + + +def _terminate_editor_process(pid, wait_s=5): + """Signal an editor's process group and wait briefly for it to exit.""" + try: + os.killpg(os.getpgid(pid), signal.SIGTERM) + except (ProcessLookupError, PermissionError, OSError): + with contextlib.suppress(ProcessLookupError, PermissionError): + os.kill(pid, signal.SIGTERM) + deadline = _pkg.time.time() + wait_s + while _pkg.time.time() < deadline and _pixlet_editor_alive(pid): + _pkg.time.sleep(0.25) + if _pixlet_editor_alive(pid): + with contextlib.suppress(ProcessLookupError, PermissionError, OSError): + os.killpg(os.getpgid(pid), signal.SIGKILL) + + @api_v3.route('/starlark/editor/start', methods=['POST']) def start_pixlet_editor(): """Start an editing session for one app.""" @@ -792,60 +815,69 @@ def start_pixlet_editor(): return jsonify({'status': 'error', 'message': 'Pixlet is not installed - install it first'}), 503 - current = _pixlet_editor_status() - if current.get('running'): - return jsonify({'status': 'error', - 'message': f"An editor session for '{current.get('app_id')}' is " - f"already running; stop it first"}), 409 + with _EDITOR_START_LOCK: + current = _pixlet_editor_status() + if current.get('running'): + return jsonify({'status': 'error', + 'message': f"An editor session for '{current.get('app_id')}' is " + f"already running; stop it first"}), 409 - try: - timeout_s = int(data.get('timeout') or _PIXLET_EDITOR_DEFAULT_TIMEOUT) - except (TypeError, ValueError): - return jsonify({'status': 'error', 'message': 'timeout must be a whole number'}), 400 - if not 60 <= timeout_s <= _PIXLET_EDITOR_MAX_TIMEOUT: - return jsonify({'status': 'error', - 'message': f'timeout must be between 60 and ' - f'{_PIXLET_EDITOR_MAX_TIMEOUT} seconds'}), 400 - try: - port = int(data.get('port') or _PIXLET_EDITOR_DEFAULT_PORT) - except (TypeError, ValueError): - return jsonify({'status': 'error', 'message': 'port must be a whole number'}), 400 - if not 1024 <= port <= 65535: - return jsonify({'status': 'error', 'message': 'port must be between 1024 and 65535'}), 400 + try: + timeout_s = int(data.get('timeout') or _PIXLET_EDITOR_DEFAULT_TIMEOUT) + except (TypeError, ValueError): + return jsonify({'status': 'error', 'message': 'timeout must be a whole number'}), 400 + if not 60 <= timeout_s <= _PIXLET_EDITOR_MAX_TIMEOUT: + return jsonify({'status': 'error', + 'message': f'timeout must be between 60 and ' + f'{_PIXLET_EDITOR_MAX_TIMEOUT} seconds'}), 400 + try: + port = int(data.get('port') or _PIXLET_EDITOR_DEFAULT_PORT) + except (TypeError, ValueError): + return jsonify({'status': 'error', 'message': 'port must be a whole number'}), 400 + if not 1024 <= port <= 65535: + return jsonify({'status': 'error', 'message': 'port must be between 1024 and 65535'}), 400 - env = dict(os.environ) - env['PIXLET_EDITOR_PORT'] = str(port) - env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s) - # A browser reaching this endpoint is remote by definition, so the - # session needs to listen on more than loopback to be usable at all -- - # but only as a *default*. An operator who has already set - # PIXLET_EDITOR_HOST (e.g. to keep it loopback-only even from the web - # UI) must not have that overridden here. - env.setdefault('PIXLET_EDITOR_HOST', '0.0.0.0') + env = dict(os.environ) + env['PIXLET_EDITOR_PORT'] = str(port) + env['PIXLET_EDITOR_TIMEOUT'] = str(timeout_s) + # A browser reaching this endpoint is remote by definition, so the + # session needs to listen on more than loopback to be usable at all -- + # but only as a *default*. An operator who has already set + # PIXLET_EDITOR_HOST (e.g. to keep it loopback-only even from the web + # UI) must not have that overridden here. + env.setdefault('PIXLET_EDITOR_HOST', '0.0.0.0') - log_path = Path(tempfile.gettempdir()) / 'ledmatrix_pixlet_editor.log' - log_handle = open(log_path, 'w', encoding='utf-8') # noqa: SIM115 - owned by the child - try: - # start_new_session so the script leads its own process group: the - # stop route signals the group, which is what lets the EXIT trap run - # and hand the display back. - process = subprocess.Popen( # nosec B603 - fixed script path, validated app_id - ['/bin/bash', str(_PIXLET_EDITOR_SCRIPT), app_dir.name], - cwd=str(PROJECT_ROOT), env=env, - stdout=log_handle, stderr=subprocess.STDOUT, - start_new_session=True) - finally: - log_handle.close() + log_path = Path(tempfile.gettempdir()) / 'ledmatrix_pixlet_editor.log' + log_handle = open(log_path, 'w', encoding='utf-8') # noqa: SIM115 - owned by the child + try: + # start_new_session so the script leads its own process group: the + # stop route signals the group, which is what lets the EXIT trap run + # and hand the display back. + process = subprocess.Popen( # nosec B603 - fixed script path, validated app_id + ['/bin/bash', str(_PIXLET_EDITOR_SCRIPT), app_dir.name], + cwd=str(PROJECT_ROOT), env=env, + stdout=log_handle, stderr=subprocess.STDOUT, + start_new_session=True) + finally: + log_handle.close() - now = _pkg.time.time() - state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port, - 'timeout': timeout_s, 'started_at': now, 'deadline': now + timeout_s, - 'host': env['PIXLET_EDITOR_HOST'], 'log': str(log_path)} - try: - with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle: - json.dump(state, handle) - except OSError as err: - logger.warning('Started an editor session but could not record it: %s', err) + now = _pkg.time.time() + state = {'pid': process.pid, 'app_id': app_dir.name, 'port': port, + 'timeout': timeout_s, 'started_at': now, 'deadline': now + timeout_s, + 'host': env['PIXLET_EDITOR_HOST'], 'log': str(log_path)} + try: + with open(_PIXLET_EDITOR_STATE, 'w', encoding='utf-8') as handle: + json.dump(state, handle) + except OSError as err: + # The process is up but nothing records its PID: status and stop + # would both report no session while the display stays down until + # the timeout expires. Take the editor with us instead. + logger.error('Started an editor session but could not record it: %s', err) + _terminate_editor_process(process.pid) + return jsonify({'status': 'error', + 'message': 'Could not record the editor session; ' + 'the editor was stopped.', + 'details': describe_exception(err)}), 500 logger.info('Pixlet editor started for %s on port %s (pid %s, %ss limit)', app_dir.name, port, process.pid, timeout_s) @@ -883,13 +915,36 @@ def stop_pixlet_editor(): while _pkg.time.time() < deadline and _pixlet_editor_alive(pid): _pkg.time.sleep(0.25) + escalated = False if _pixlet_editor_alive(pid): - logger.warning('Editor session %s ignored SIGTERM; sending SIGKILL. The display ' - 'may need a manual restart.', pid) + logger.warning('Editor session %s ignored SIGTERM; sending SIGKILL.', pid) with contextlib.suppress(ProcessLookupError, PermissionError, OSError): os.killpg(os.getpgid(pid), signal.SIGKILL) + escalated = True _clear_pixlet_editor_state() + + if escalated: + # SIGKILL gives the script's EXIT trap no chance to run, so nothing + # has handed the display back. Reporting "the display is restarting" + # here was simply untrue: restart it, and if that fails say so + # rather than leave the panel dark behind a success response. + result = _run_systemctl_command( + ['sudo', 'systemctl', 'start', 'ledmatrix.service']) + if result.get('returncode') != 0: + logger.error('Display restart after SIGKILL failed: %s', + (result.get('stderr') or '').strip()) + return jsonify({ + 'status': 'error', + 'message': 'Editor force-stopped, but the display could not be ' + 'restarted automatically - start it manually.', + 'details': (result.get('stderr') or '').strip(), + 'data': {'running': False}}), 500 + return jsonify({'status': 'success', + 'message': 'Editor force-stopped; the display has been ' + 'restarted.', + 'data': {'running': False}}) + return jsonify({'status': 'success', 'message': 'Editor stopped; the display is restarting.', 'data': {'running': False}}) diff --git a/web_interface/templates/v3/partials/tools.html b/web_interface/templates/v3/partials/tools.html index 2dd273e9..6de6dcb0 100644 --- a/web_interface/templates/v3/partials/tools.html +++ b/web_interface/templates/v3/partials/tools.html @@ -1251,7 +1251,7 @@ @@ -1263,6 +1263,15 @@ body.innerHTML = active + `
${rows}
`; + + // Bound here rather than via an inline onclick: escHtml does not encode + // single quotes, and the id used to be interpolated into a single-quoted + // JS string, so an app directory containing an apostrophe could break + // out of it and run script. Through dataset the value is only ever + // parsed as an HTML attribute, never as JavaScript. + body.querySelectorAll('[data-pixlet-edit]').forEach(btn => { + btn.addEventListener('click', () => startPixletEditor(btn.dataset.appId)); + }); } window.loadPixletEditor = function() {