From 7b36179e1d16eb0f5a3458f2af374a2c537cbc64 Mon Sep 17 00:00:00 2001 From: Jason Wallace Date: Thu, 15 Feb 2024 21:23:01 +0200 Subject: [PATCH] Check for unnecessary privilege escalation (#1743) Resolves https://github.com/tiny-pilot/tinypilot-pro/issues/1214 Blocked by https://github.com/tiny-pilot/tinypilot/pull/1744 Blocked by https://github.com/tiny-pilot/tinypilot/pull/1745 This PR adds a dev script that checks for possible cases of privilege escalation in tinypilot-writable scripts (i.e., `scripts/`). The script only does a superficial check that root privileges were at least considered by matching on: > This script doesn't require root privileges. Example output of `dev-scripts/check-privilege-guard`: ``` $ ./dev-scripts/check-privilege-guard These files are missing a guard against privilege escalation: scripts/is-ssh-enabled scripts/streaming-mode scripts/update-service scripts/upgrade Please add the following check (or similar) to the above scripts: if [[ "${EUID}" == 0 ]]; then >&2 echo "This script doesn't require root privileges." >&2 echo 'Please re-run as tinypilot:' >&2 echo " runuser tinypilot --command '$0 $*'" exit 1 fi ``` Notes 1. These tinypilot-writable scripts legitimately require root privileges: * `scripts/install-bundle` * `script/upgrade` So they do risk being used for privilege escalation, but they are/should never be executed by privileged scripts on the device. I've also added a superficial check for this too. 2. This PR also fixes the privilege escalation issues that `dev-scripts/check-privilege-guard` as picked up. As a reminder, the fix is a runtime error asking for reduced permissions which is something we'll only encounter when we physically test the device. So as a result, this PR also tries to avoid those runtime errors by running these identified scripts as `tinypilot` where needed: ``` runuser tinypilot --command '/opt/tinypilot/scripts/some-script' ``` Review on CodeApprove --- .circleci/config.yml | 9 +++++ .../scripts/collect-debug-logs | 4 +-- dev-scripts/check-privilege-guard | 34 +++++++++++++++++++ scripts/is-ssh-enabled | 7 ++++ scripts/streaming-mode | 7 ++++ scripts/update-service | 11 ++++++ 6 files changed, 70 insertions(+), 2 deletions(-) create mode 100755 dev-scripts/check-privilege-guard diff --git a/.circleci/config.yml b/.circleci/config.yml index aecb8c5d0..e251075a7 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -22,6 +22,14 @@ jobs: - run: name: Run static analysis on bash scripts command: ./dev-scripts/check-bash + check_privilege_guard: + docker: + - image: cimg/base:2020.01 + steps: + - checkout + - run: + name: Check for unnecessary privilege escalation + command: ./dev-scripts/check-privilege-guard lint_sql: docker: - image: cimg/python:3.9.17 @@ -272,6 +280,7 @@ workflows: jobs: - check_whitespace - check_bash + - check_privilege_guard - lint_sql - check_style - decode_edid diff --git a/debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs b/debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs index 77297caa2..740d8073e 100755 --- a/debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs +++ b/debian-pkg/opt/tinypilot-privileged/scripts/collect-debug-logs @@ -109,7 +109,7 @@ print_info "Checking if filesystem is read-only..." print_info "Checking if SSH is enabled..." { SSH_STATUS="disabled" - if /opt/tinypilot/scripts/is-ssh-enabled ; then + if runuser tinypilot --command '/opt/tinypilot/scripts/is-ssh-enabled' ; then SSH_STATUS="enabled" fi readonly SSH_STATUS @@ -166,7 +166,7 @@ print_info "Checking for voltage issues..." print_info "Checking TinyPilot streaming mode..." { echo "Streaming mode" - echo "Selected mode: $(/opt/tinypilot/scripts/streaming-mode)" + echo "Selected mode: $(runuser tinypilot --command '/opt/tinypilot/scripts/streaming-mode')" printf "Current mode: " # H264 mode is considered active when the last Janus video log line contains # "Memsink opened; reading frames". diff --git a/dev-scripts/check-privilege-guard b/dev-scripts/check-privilege-guard new file mode 100755 index 000000000..487eb832b --- /dev/null +++ b/dev-scripts/check-privilege-guard @@ -0,0 +1,34 @@ +#!/bin/bash +# +# Check that the TinyPilot scripts contain a guard against privilege escalation. +# +# This script enforces a pattern of ensuring that scripts which are writable by +# the `tinypilot` user don't get executed with unnecessary root privileges. + +# Exit on first failing command. +set -e + +# Exit on unset variable. +set -u + +# Find TinyPilot scripts that don't guard against privilege escalation. +MATCHES="$(grep \ + --files-without-match \ + --fixed-strings \ + --regexp "This script doesn't require root privileges." \ + scripts/*; true)" +readonly MATCHES +if [[ -n "${MATCHES}" ]]; then + >&2 echo 'These files are missing a guard against privilege escalation:' + >&2 echo "${MATCHES}" + >&2 echo 'Please add the following check (or similar) to the above scripts:' + >&2 cat <<'EOF' +if [[ "${EUID}" == 0 ]]; then + >&2 echo "This script doesn't require root privileges." + >&2 echo 'Please re-run as tinypilot:' + >&2 echo " runuser tinypilot --command '$0 $*'" + exit 1 +fi +EOF + exit 1 +fi diff --git a/scripts/is-ssh-enabled b/scripts/is-ssh-enabled index 25ac068e6..958f42062 100755 --- a/scripts/is-ssh-enabled +++ b/scripts/is-ssh-enabled @@ -8,6 +8,13 @@ set -u # Exit on first error. set -e +if [[ "${EUID}" == 0 ]]; then + >&2 echo "This script doesn't require root privileges." + >&2 echo 'Please re-run as tinypilot:' + >&2 echo " runuser tinypilot --command '$0 $*'" + exit 1 +fi + print_help() { cat << EOF Usage: ${0##*/} [-h] diff --git a/scripts/streaming-mode b/scripts/streaming-mode index 4eacdddc2..9d10f3acf 100755 --- a/scripts/streaming-mode +++ b/scripts/streaming-mode @@ -8,6 +8,13 @@ set -e # Exit on unset variable. set -u +if [[ "${EUID}" == 0 ]]; then + >&2 echo "This script doesn't require root privileges." + >&2 echo 'Please re-run as tinypilot:' + >&2 echo " runuser tinypilot --command '$0 $*'" + exit 1 +fi + SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" readonly SCRIPT_DIR cd "${SCRIPT_DIR}/.." diff --git a/scripts/update-service b/scripts/update-service index 2390bb2c4..4b6fd23a5 100755 --- a/scripts/update-service +++ b/scripts/update-service @@ -10,7 +10,9 @@ import argparse import logging +import os import subprocess +import sys # We’re importing the log package first because it needs to overwrite the # app-wide logger class before any other module loads it. @@ -61,6 +63,15 @@ def main(_): if __name__ == '__main__': + # Ensure that the script doesn't have unnecessary privileges. + if os.geteuid() == 0: + print("This script doesn't require root privileges.", file=sys.stderr) + print('Please re-run as tinypilot:', file=sys.stderr) + print(' runuser tinypilot --command', + f"'{' '.join(sys.argv)}'", + file=sys.stderr) + sys.exit(1) + parser = argparse.ArgumentParser( prog='TinyPilot Update Service', formatter_class=argparse.ArgumentDefaultsHelpFormatter)