crypt/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-01-26 07:41:33 +03:00
Code Activity

Print warning when certbot-auto has insecure permissions. (#6995)

Browse Source 
This PR attempts to better inform people about the problem identified at https://community.letsencrypt.org/t/certbot-auto-deployment-best-practices/91979/.

I was hesitant to add the flag --no-permissions-check, however, if there's some obscure distro out there (or custom user setup) that has a strange users and groups, I didn't want us to either:

Have to put out a bug fix release
Refuse to fix the problem and let them deal with warnings on every run

* add check_permissions.py

* Update letsencrypt-auto.template.

* build letsencrypt-auto

* Add test_permissions_warnings to auto_test

* Allow uid/gid < 1000.

* Add --no-permissions-check to Certbot.

* Add --no-permissions-check to certbot-auto.

* Add test farm test that letsencrypt-auto is quiet.

As a bonus, this new test will catch problems like the one that the caused
0.33.1 point release.

* Update CHANGELOG about permissions check.

* Update permissions comment.

* Fix symlink handling.

* Use a better default in auto_test.py.
This commit is contained in:
Brad Warren
2019-04-30 10:45:03 -07:00
committed by GitHub
parent b41a992545
commit d1330efe41
9 changed files with 365 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
CHANGELOG.md
View File

@@ -25,6 +25,12 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
* Adding a warning noting that future versions of Certbot will automatically configure the
webserver so that all requests redirect to secure HTTPS access. You can control this
behavior and disable this warning with the --redirect and --no-redirect flags.
* certbot-auto now prints warnings when run as root with insecure file system
permissions. If you see these messages, you should fix the problem by
following the instructions at
https://community.letsencrypt.org/t/certbot-auto-deployment-best-practices/91979/,
however, these warnings can be disabled as necessary with the flag
--no-permissions-check.
### Fixed

5
certbot/cli.py
View File

@@ -1089,6 +1089,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis
help="(certbot-auto only) prevent the certbot-auto script from"
" installing OS-level dependencies (default: Prompt to install "
" OS-wide dependencies, but exit if the user says 'No')")
helpful.add(
"automation", "--no-permissions-check", action="store_true",
default=flag_default("no_permissions_check"),
help="(certbot-auto only) skip the check on the file system"
" permissions of the certbot-auto script")
helpful.add(
["automation", "renew", "certonly", "run"],
"-q", "--quiet", dest="quiet", action="store_true",

1
certbot/constants.py
View File

@@ -46,6 +46,7 @@ CLI_DEFAULTS = dict(
duplicate=False,
os_packages_only=False,
no_self_upgrade=False,
no_permissions_check=False,
no_bootstrap=False,
quiet=False,
staging=False,

4
certbot/tests/cli_test.py
View File

@@ -453,6 +453,10 @@ class ParseTest(unittest.TestCase): # pylint: disable=too-many-public-methods
for topic in ['all', 'plugins', 'dns-route53']:
self.assertFalse('certbot-route53:auth' in self._help_output([help_flag, topic]))
def test_no_permissions_check_accepted(self):
namespace = self.parse(["--no-permissions-check"])
self.assertTrue(namespace.no_permissions_check)
class DefaultTest(unittest.TestCase):
"""Tests for certbot.cli._Default."""

112
letsencrypt-auto-source/letsencrypt-auto
View File

@@ -45,6 +45,7 @@ Help for certbot itself cannot be provided until it is installed.
-h, --help print this help
-n, --non-interactive, --noninteractive run without asking for user input
--no-bootstrap do not install OS dependencies
--no-permissions-check do not warn about file system permissions
--no-self-upgrade do not download updates
--os-packages-only install OS dependencies and exit
--install-only install certbot, upgrade if needed, and exit
@@ -67,6 +68,8 @@ for arg in "$@" ; do
# Do not upgrade this script (also prevents client upgrades, because each
# copy of the script pins a hash of the python client)
NO_SELF_UPGRADE=1;;
--no-permissions-check)
NO_PERMISSIONS_CHECK=1;;
--no-bootstrap)
NO_BOOTSTRAP=1;;
--help)
@@ -172,7 +175,11 @@ SetRootAuthMechanism() {
sudo)
SUDO="sudo -E"
;;
'') ;; # Nothing to do for plain root method.
'')
# If we're not running with root, don't check that this script can only
# be modified by system users and groups.
NO_PERMISSIONS_CHECK=1
;;
*)
error "Error: unknown root authorization mechanism '$LE_AUTO_SUDO'."
exit 1
@@ -1494,6 +1501,108 @@ else
exit 0
fi
DeterminePythonVersion "NOCRASH"
# 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 and only print the script's output if it
# ran successfully.
CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null)
CHECK_PERM_STATUS="$?"
set -e
if [ "$CHECK_PERM_STATUS" = 0 ]; then
error "$CHECK_PERM_OUT"
fi
fi
if [ "$NO_SELF_UPGRADE" != 1 ]; then
TEMP_DIR=$(TempDir)
trap 'rm -rf "$TEMP_DIR"' EXIT
@@ -1650,7 +1759,6 @@ if __name__ == '__main__':
UNLIKELY_EOF
# ---------------------------------------------------------------------------
DeterminePythonVersion "NOCRASH"
if [ "$PYVER" -lt "$MIN_PYVER" ]; then
error "WARNING: couldn't find Python $MIN_PYTHON_VERSION+ to check for updates."
elif ! REMOTE_VERSION=`"$LE_PYTHON" "$TEMP_DIR/fetch.py" --latest-version` ; then

31
letsencrypt-auto-source/letsencrypt-auto.template
View File

@@ -45,6 +45,7 @@ Help for certbot itself cannot be provided until it is installed.
-h, --help print this help
-n, --non-interactive, --noninteractive run without asking for user input
--no-bootstrap do not install OS dependencies
--no-permissions-check do not warn about file system permissions
--no-self-upgrade do not download updates
--os-packages-only install OS dependencies and exit
--install-only install certbot, upgrade if needed, and exit
@@ -67,6 +68,8 @@ for arg in "$@" ; do
# Do not upgrade this script (also prevents client upgrades, because each
# copy of the script pins a hash of the python client)
NO_SELF_UPGRADE=1;;
--no-permissions-check)
NO_PERMISSIONS_CHECK=1;;
--no-bootstrap)
NO_BOOTSTRAP=1;;
--help)
@@ -172,7 +175,11 @@ SetRootAuthMechanism() {
sudo)
SUDO="sudo -E"
;;
'') ;; # Nothing to do for plain root method.
'')
# If we're not running with root, don't check that this script can only
# be modified by system users and groups.
NO_PERMISSIONS_CHECK=1
;;
*)
error "Error: unknown root authorization mechanism '$LE_AUTO_SUDO'."
exit 1
@@ -652,6 +659,27 @@ else
exit 0
fi
DeterminePythonVersion "NOCRASH"
# 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 and only print the script's output if it
# ran successfully.
CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null)
CHECK_PERM_STATUS="$?"
set -e
if [ "$CHECK_PERM_STATUS" = 0 ]; then
error "$CHECK_PERM_OUT"
fi
fi
if [ "$NO_SELF_UPGRADE" != 1 ]; then
TEMP_DIR=$(TempDir)
trap 'rm -rf "$TEMP_DIR"' EXIT
@@ -660,7 +688,6 @@ else
{{ fetch.py }}
UNLIKELY_EOF
# ---------------------------------------------------------------------------
DeterminePythonVersion "NOCRASH"
if [ "$PYVER" -lt "$MIN_PYVER" ]; then
error "WARNING: couldn't find Python $MIN_PYTHON_VERSION+ to check for updates."
elif ! REMOTE_VERSION=`"$LE_PYTHON" "$TEMP_DIR/fetch.py" --latest-version` ; then

81
letsencrypt-auto-source/pieces/check_permissions.py Normal file
View File

@@ -0,0 +1,81 @@
"""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])

130
letsencrypt-auto-source/tests/auto_test.py
View File

@@ -4,13 +4,13 @@ from BaseHTTPServer import HTTPServer, BaseHTTPRequestHandler
from contextlib import contextmanager
from functools import partial
from json import dumps
from os import chmod, environ, makedirs
from os import chmod, environ, makedirs, stat
from os.path import abspath, dirname, exists, join
import re
from shutil import copy, rmtree
import socket
import ssl
from stat import S_IRUSR, S_IXUSR
from stat import S_IMODE, S_IRUSR, S_IWUSR, S_IXUSR, S_IWGRP, S_IWOTH
from subprocess import CalledProcessError, Popen, PIPE
import sys
from tempfile import mkdtemp
@@ -192,7 +192,7 @@ def install_le_auto(contents, install_path):
chmod(install_path, S_IRUSR | S_IXUSR)
def run_le_auto(le_auto_path, venv_dir, base_url, **kwargs):
def run_le_auto(le_auto_path, venv_dir, base_url=None, le_auto_args_str='--version', **kwargs):
"""Run the prebuilt version of letsencrypt-auto, returning stdout and
stderr strings.
@@ -201,13 +201,17 @@ def run_le_auto(le_auto_path, venv_dir, base_url, **kwargs):
"""
env = environ.copy()
d = dict(VENV_PATH=venv_dir,
# URL to PyPI-style JSON that tell us the latest released version
# of LE:
LE_AUTO_JSON_URL=base_url + 'certbot/json',
# URL to dir containing letsencrypt-auto and letsencrypt-auto.sig:
LE_AUTO_DIR_TEMPLATE=base_url + '%s/',
# The public key corresponding to signing.key:
LE_AUTO_PUBLIC_KEY="""-----BEGIN PUBLIC KEY-----
NO_CERT_VERIFY='1',
**kwargs)
if base_url is not None:
# URL to PyPI-style JSON that tell us the latest released version
# of LE:
d['LE_AUTO_JSON_URL'] = base_url + 'certbot/json'
# URL to dir containing letsencrypt-auto and letsencrypt-auto.sig:
d['LE_AUTO_DIR_TEMPLATE'] = base_url + '%s/'
# The public key corresponding to signing.key:
d['LE_AUTO_PUBLIC_KEY'] = """-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsMoSzLYQ7E1sdSOkwelg
tzKIh2qi3bpXuYtcfFC0XrvWig071NwIj+dZiT0OLZ2hPispEH0B7ISuuWg1ll7G
hFW0VdbxL6JdGzS2ShNWkX9hE9z+j8VqwDPOBn3ZHm03qwpYkBDwQib3KqOdYbTT
@@ -215,12 +219,12 @@ uUtJmmGcuk3a9Aq/sCT6DdfmTSdP5asdQYwIcaQreDrOosaS84DTWI3IU+UYJVgl
LsIVPBuy9IcgHidUQ96hJnoPsDCWsHwX62495QKEarauyKQrJzFes0EY95orDM47
Z5o/NDiQB11m91yNB0MmPYY9QSbnOA9j7IaaC97AwRLuwXY+/R2ablTcxurWou68
iQIDAQAB
-----END PUBLIC KEY-----""",
NO_CERT_VERIFY='1',
**kwargs)
-----END PUBLIC KEY-----"""
env.update(d)
return out_and_err(
le_auto_path + ' --version',
le_auto_path + ' ' + le_auto_args_str,
shell=True,
env=env)
@@ -240,6 +244,12 @@ def set_le_script_version(venv_dir, version):
chmod(letsencrypt_path, S_IRUSR | S_IXUSR)
def sudo_chmod(path, mode):
"""Runs `sudo chmod mode path`."""
mode = oct(mode).replace('o', '')
out_and_err(['sudo', 'chmod', mode, path])
class AutoTests(TestCase):
"""Test the major branch points of letsencrypt-auto:
@@ -395,3 +405,95 @@ class AutoTests(TestCase):
else:
self.fail("Pip didn't detect a bad hash and stop the "
"installation.")
def test_permissions_warnings(self):
"""Make sure letsencrypt-auto properly warns about permissions problems."""
# This test assumes that only the parent of the directory containing
# letsencrypt-auto (usually /tmp) may have permissions letsencrypt-auto
# considers insecure.
with temp_paths() as (le_auto_path, venv_dir):
le_auto_path = abspath(le_auto_path)
le_auto_dir = dirname(le_auto_path)
le_auto_dir_parent = dirname(le_auto_dir)
install_le_auto(self.NEW_LE_AUTO, le_auto_path)
run_letsencrypt_auto = partial(
run_le_auto, le_auto_path, venv_dir,
le_auto_args_str='--install-only --no-self-upgrade',
PIP_FIND_LINKS=join(tests_dir(), 'fake-letsencrypt', 'dist'))
# Run letsencrypt-auto once with current permissions to avoid
# potential problems when the script tries to write to temporary
# directories.
run_letsencrypt_auto()
le_auto_dir_mode = stat(le_auto_dir).st_mode
le_auto_dir_parent_mode = S_IMODE(stat(le_auto_dir_parent).st_mode)
try:
# Make letsencrypt-auto happy with the current permissions
chmod(le_auto_dir, S_IRUSR | S_IXUSR)
sudo_chmod(le_auto_dir_parent, 0o755)
self._test_permissions_warnings_about_path(le_auto_path, run_letsencrypt_auto)
self._test_permissions_warnings_about_path(le_auto_dir, run_letsencrypt_auto)
finally:
chmod(le_auto_dir, le_auto_dir_mode)
sudo_chmod(le_auto_dir_parent, le_auto_dir_parent_mode)
def _test_permissions_warnings_about_path(self, path, run_le_auto_func):
# Test that there are no problems with the current permissions
out, _ = run_le_auto_func()
self.assertFalse('insecure permissions' in out)
stat_result = stat(path)
original_mode = stat_result.st_mode
# Test world permissions
chmod(path, original_mode | S_IWOTH)
out, _ = run_le_auto_func()
self.assertTrue('insecure permissions' in out)
# Test group permissions
if stat_result.st_gid >= 1000:
chmod(path, original_mode | S_IWGRP)
out, _ = run_le_auto_func()
self.assertTrue('insecure permissions' in out)
# Test owner permissions
if stat_result.st_uid >= 1000:
chmod(path, original_mode | S_IWUSR)
out, _ = run_le_auto_func()
self.assertTrue('insecure permissions' in out)
# Test that permissions were properly restored
chmod(path, original_mode)
out, _ = run_le_auto_func()
self.assertFalse('insecure permissions' in out)
def test_disabled_permissions_warnings(self):
"""Make sure that letsencrypt-auto permissions warnings can be disabled."""
with temp_paths() as (le_auto_path, venv_dir):
le_auto_path = abspath(le_auto_path)
install_le_auto(self.NEW_LE_AUTO, le_auto_path)
le_auto_args_str='--install-only --no-self-upgrade'
pip_links=join(tests_dir(), 'fake-letsencrypt', 'dist')
out, _ = run_le_auto(le_auto_path, venv_dir,
le_auto_args_str=le_auto_args_str,
PIP_FIND_LINKS=pip_links)
self.assertTrue('insecure permissions' in out)
# Test that warnings are disabled when the script isn't run as
# root.
out, _ = run_le_auto(le_auto_path, venv_dir,
le_auto_args_str=le_auto_args_str,
LE_AUTO_SUDO='',
PIP_FIND_LINKS=pip_links)
self.assertFalse('insecure permissions' in out)
# Test that --no-permissions-check disables warnings.
le_auto_args_str += ' --no-permissions-check'
out, _ = run_le_auto(
le_auto_path, venv_dir,
le_auto_args_str=le_auto_args_str,
PIP_FIND_LINKS=pip_links)
self.assertFalse('insecure permissions' in out)

14
tests/letstest/scripts/test_letsencrypt_auto_certonly_standalone.sh
View File

@@ -9,7 +9,13 @@ set -eo pipefail
#private_ip=$(curl -s http://169.254.169.254/2014-11-05/meta-data/local-ipv4)
cd letsencrypt
export PATH="$PWD/letsencrypt-auto-source:$PATH"
LE_AUTO_DIR="/usr/local/bin"
LE_AUTO_PATH="$LE_AUTO_DIR/letsencrypt-auto"
sudo cp letsencrypt-auto-source/letsencrypt-auto "$LE_AUTO_PATH"
sudo chown root "$LE_AUTO_PATH"
sudo chmod 0755 "$LE_AUTO_PATH"
export PATH="$LE_AUTO_DIR:$PATH"
letsencrypt-auto --os-packages-only --debug --version
# Create a venv-like layout at the old virtual environment path to test that a
@@ -35,3 +41,9 @@ if ! letsencrypt-auto --help --no-self-upgrade | grep -F "letsencrypt-auto [SUBC
echo "letsencrypt-auto not included in help output!"
exit 1
fi
OUTPUT=$(letsencrypt-auto --install-only --no-self-upgrade --quiet 2>&1)
if [ -n "$OUTPUT" ]; then
echo letsencrypt-auto produced unexpected output!
exit 1
fi