From 71b1b8c2d9b99f1ca42b7ae08a1ef577341ce485 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Tue, 7 May 2019 00:49:47 +0200 Subject: [PATCH] Fix check permissions logic (#7034) Fixes #7031 I use the same approach than in `CreateVenv()` and `CompareVersions()`: a new bash function `CheckPathPermissions()` is declared an execute a python script passed to the interpreter through stdin. This allows: * to not require the temp_dir that holds a temporary script to be executed * to reduce at the bare minimum the change to make on the order of bash command to execute (including when the temp_dir is created) * Fix check permissions logic in certbot-auto by making a temp dir useless * Update CHANGELOG.md --- CHANGELOG.md | 2 + letsencrypt-auto-source/letsencrypt-auto | 177 +++++++++--------- .../letsencrypt-auto.template | 15 +- 3 files changed, 101 insertions(+), 93 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84f28cea7..408118ad0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * certbot-auto no longer prints a blank line when there are no permissions problems. +* certbot-auto no longer writes a check_permissions.py script at the root + of the filesystem. Despite us having broken lockstep, we are continuing to release new versions of all Certbot components during releases for the time being, however, the only diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index f9f02da62..da3fbe30f 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -953,6 +953,95 @@ if __name__ == '__main__': UNLIKELY_EOF } +# Check that the given PATH_TO_CHECK has secured permissions. +# Parameters: LE_PYTHON, PATH_TO_CHECK +CheckPathPermissions() { + "$1" - "$2" << "UNLIKELY_EOF" +"""Verifies certbot-auto cannot be modified by unprivileged users. + +This script takes the path to certbot-auto as its only command line +argument. It then checks that the file can only be modified by uid/gid +< 1000 and if other users can modify the file, it prints a warning with +a suggestion on how to solve the problem. + +Permissions on symlinks in the absolute path of certbot-auto are ignored +and only the canonical path to certbot-auto is checked. There could be +permissions problems due to the symlinks that are unreported by this +script, however, issues like this were not caused by our documentation +and are ignored for the sake of simplicity. + +All warnings are printed to stdout rather than stderr so all stderr +output from this script can be suppressed to avoid printing messages if +this script fails for some reason. + +""" +from __future__ import print_function + +import os +import stat +import sys + + +FORUM_POST_URL = 'https://community.letsencrypt.org/t/certbot-auto-deployment-best-practices/91979/' + + +def has_safe_permissions(path): + """Returns True if the given path has secure permissions. + + The permissions are considered safe if the file is only writable by + uid/gid < 1000. + + The reason we allow more IDs than 0 is because on some systems such + as Debian, system users/groups other than uid/gid 0 are used for the + path we recommend in our instructions which is /usr/local/bin. 1000 + was chosen because on Debian 0-999 is reserved for system IDs[1] and + on RHEL either 0-499 or 0-999 is reserved depending on the + version[2][3]. Due to these differences across different OSes, this + detection isn't perfect so we only determine permissions are + insecure when we can be reasonably confident there is a problem + regardless of the underlying OS. + + [1] https://www.debian.org/doc/debian-policy/ch-opersys.html#uid-and-gid-classes + [2] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/ch-managing_users_and_groups + [3] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-managing_users_and_groups + + :param str path: filesystem path to check + :returns: True if the path has secure permissions, otherwise, False + :rtype: bool + + """ + # os.stat follows symlinks before obtaining information about a file. + stat_result = os.stat(path) + if stat_result.st_mode & stat.S_IWOTH: + return False + if stat_result.st_mode & stat.S_IWGRP and stat_result.st_gid >= 1000: + return False + if stat_result.st_mode & stat.S_IWUSR and stat_result.st_uid >= 1000: + return False + return True + + +def main(certbot_auto_path): + current_path = os.path.realpath(certbot_auto_path) + last_path = None + permissions_ok = True + # This loop makes use of the fact that os.path.dirname('/') == '/'. + while current_path != last_path and permissions_ok: + permissions_ok = has_safe_permissions(current_path) + last_path = current_path + current_path = os.path.dirname(current_path) + + if not permissions_ok: + print('{0} has insecure permissions!'.format(certbot_auto_path)) + print('To learn how to fix them, visit {0}'.format(FORUM_POST_URL)) + + +if __name__ == '__main__': + main(sys.argv[1]) + +UNLIKELY_EOF +} + if [ "$1" = "--le-auto-phase2" ]; then # Phase 2: Create venv, install LE, and run. @@ -1505,96 +1594,10 @@ else # Don't warn about file permissions if the user disabled the check or we # can't find an up-to-date Python. if [ "$PYVER" -ge "$MIN_PYVER" -a "$NO_PERMISSIONS_CHECK" != 1 ]; then - # --------------------------------------------------------------------------- - cat << "UNLIKELY_EOF" > "$TEMP_DIR/check_permissions.py" -"""Verifies certbot-auto cannot be modified by unprivileged users. - -This script takes the path to certbot-auto as its only command line -argument. It then checks that the file can only be modified by uid/gid -< 1000 and if other users can modify the file, it prints a warning with -a suggestion on how to solve the problem. - -Permissions on symlinks in the absolute path of certbot-auto are ignored -and only the canonical path to certbot-auto is checked. There could be -permissions problems due to the symlinks that are unreported by this -script, however, issues like this were not caused by our documentation -and are ignored for the sake of simplicity. - -All warnings are printed to stdout rather than stderr so all stderr -output from this script can be suppressed to avoid printing messages if -this script fails for some reason. - -""" -from __future__ import print_function - -import os -import stat -import sys - - -FORUM_POST_URL = 'https://community.letsencrypt.org/t/certbot-auto-deployment-best-practices/91979/' - - -def has_safe_permissions(path): - """Returns True if the given path has secure permissions. - - The permissions are considered safe if the file is only writable by - uid/gid < 1000. - - The reason we allow more IDs than 0 is because on some systems such - as Debian, system users/groups other than uid/gid 0 are used for the - path we recommend in our instructions which is /usr/local/bin. 1000 - was chosen because on Debian 0-999 is reserved for system IDs[1] and - on RHEL either 0-499 or 0-999 is reserved depending on the - version[2][3]. Due to these differences across different OSes, this - detection isn't perfect so we only determine permissions are - insecure when we can be reasonably confident there is a problem - regardless of the underlying OS. - - [1] https://www.debian.org/doc/debian-policy/ch-opersys.html#uid-and-gid-classes - [2] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/ch-managing_users_and_groups - [3] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-managing_users_and_groups - - :param str path: filesystem path to check - :returns: True if the path has secure permissions, otherwise, False - :rtype: bool - - """ - # os.stat follows symlinks before obtaining information about a file. - stat_result = os.stat(path) - if stat_result.st_mode & stat.S_IWOTH: - return False - if stat_result.st_mode & stat.S_IWGRP and stat_result.st_gid >= 1000: - return False - if stat_result.st_mode & stat.S_IWUSR and stat_result.st_uid >= 1000: - return False - return True - - -def main(certbot_auto_path): - current_path = os.path.realpath(certbot_auto_path) - last_path = None - permissions_ok = True - # This loop makes use of the fact that os.path.dirname('/') == '/'. - while current_path != last_path and permissions_ok: - permissions_ok = has_safe_permissions(current_path) - last_path = current_path - current_path = os.path.dirname(current_path) - - if not permissions_ok: - print('{0} has insecure permissions!'.format(certbot_auto_path)) - print('To learn how to fix them, visit {0}'.format(FORUM_POST_URL)) - - -if __name__ == '__main__': - main(sys.argv[1]) - -UNLIKELY_EOF - # --------------------------------------------------------------------------- # If the script fails for some reason, don't break certbot-auto. set +e # Suppress unexpected error output. - CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null) + CHECK_PERM_OUT=$(CheckPathPermissions "$LE_PYTHON" "$0" 2>/dev/null) CHECK_PERM_STATUS="$?" set -e # Only print output if the script ran successfully and it actually produced diff --git a/letsencrypt-auto-source/letsencrypt-auto.template b/letsencrypt-auto-source/letsencrypt-auto.template index bff4173d4..c064580bd 100755 --- a/letsencrypt-auto-source/letsencrypt-auto.template +++ b/letsencrypt-auto-source/letsencrypt-auto.template @@ -501,6 +501,14 @@ CreateVenv() { UNLIKELY_EOF } +# Check that the given PATH_TO_CHECK has secured permissions. +# Parameters: LE_PYTHON, PATH_TO_CHECK +CheckPathPermissions() { + "$1" - "$2" << "UNLIKELY_EOF" +{{ check_permissions.py }} +UNLIKELY_EOF +} + if [ "$1" = "--le-auto-phase2" ]; then # Phase 2: Create venv, install LE, and run. @@ -663,15 +671,10 @@ else # Don't warn about file permissions if the user disabled the check or we # can't find an up-to-date Python. if [ "$PYVER" -ge "$MIN_PYVER" -a "$NO_PERMISSIONS_CHECK" != 1 ]; then - # --------------------------------------------------------------------------- - cat << "UNLIKELY_EOF" > "$TEMP_DIR/check_permissions.py" -{{ check_permissions.py }} -UNLIKELY_EOF - # --------------------------------------------------------------------------- # If the script fails for some reason, don't break certbot-auto. set +e # Suppress unexpected error output. - CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null) + CHECK_PERM_OUT=$(CheckPathPermissions "$LE_PYTHON" "$0" 2>/dev/null) CHECK_PERM_STATUS="$?" set -e # Only print output if the script ran successfully and it actually produced