tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
2.89k stars 244 forks source link

Check for unnecessary privilege escalation #1743

Closed jdeanwallace closed 4 months ago

jdeanwallace commented 4 months ago

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

jdeanwallace commented 4 months ago
Automated comment from CodeApprove ➜

⏳ @jotaen4tinypilot please review this Pull Request

jdeanwallace commented 4 months ago

@mtlynch - Can we mark the new check_privilege_guard check as "required" on GitHub? I don't have the right permissions to do that myself.

Screen Shot 2024-02-15 at 21 22 14
mtlynch commented 4 months ago

Can we mark the new check_privilege_guard check as "required" on GitHub? I don't have the right permissions to do that myself.

Sure thing! I've added it.