diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index f787801fa..8a69985e7 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -1505,9 +1505,9 @@ else """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 0 -and if other users can modify the file, it prints a warning with a -suggestion on how to solve the problem. +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. If the absolute path of certbot-auto contains a symlink, it is not handled specially and the symlink is followed. Due to this, there could @@ -1534,7 +1534,21 @@ 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 0. + 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 forum post which is /usr/local/bin. 1000 + was chosen because on Debian 0-999 is reserved for system IDs[1] and + on RHEL 0-500 is reserved[2]. Debian recommends normal uids start at + uid 1000 and RHEL recommends uid 5000 to allow them to increase the + range used for system IDs in the future. It's possible that the + threshold of 1000 is too high, however, this seems unlikely and + avoids printing warnings when we're not confident there is a + problem. + + [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/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 @@ -1544,9 +1558,9 @@ def has_safe_permissions(path): 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 != 0: + 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 != 0: + if stat_result.st_mode & stat.S_IWUSR and stat_result.st_uid >= 1000: return False return True diff --git a/letsencrypt-auto-source/pieces/check_permissions.py b/letsencrypt-auto-source/pieces/check_permissions.py index 0ca8b2072..22d0d7c1b 100644 --- a/letsencrypt-auto-source/pieces/check_permissions.py +++ b/letsencrypt-auto-source/pieces/check_permissions.py @@ -1,9 +1,9 @@ """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 0 -and if other users can modify the file, it prints a warning with a -suggestion on how to solve the problem. +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. If the absolute path of certbot-auto contains a symlink, it is not handled specially and the symlink is followed. Due to this, there could @@ -30,7 +30,21 @@ 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 0. + 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 forum post which is /usr/local/bin. 1000 + was chosen because on Debian 0-999 is reserved for system IDs[1] and + on RHEL 0-500 is reserved[2]. Debian recommends normal uids start at + uid 1000 and RHEL recommends uid 5000 to allow them to increase the + range used for system IDs in the future. It's possible that the + threshold of 1000 is too high, however, this seems unlikely and + avoids printing warnings when we're not confident there is a + problem. + + [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/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 @@ -40,9 +54,9 @@ def has_safe_permissions(path): 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 != 0: + 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 != 0: + if stat_result.st_mode & stat.S_IWUSR and stat_result.st_uid >= 1000: return False return True diff --git a/letsencrypt-auto-source/tests/auto_test.py b/letsencrypt-auto-source/tests/auto_test.py index f14630f98..9bfe6e605 100644 --- a/letsencrypt-auto-source/tests/auto_test.py +++ b/letsencrypt-auto-source/tests/auto_test.py @@ -455,13 +455,13 @@ class AutoTests(TestCase): self.assertTrue('insecure permissions' in out) # Test group permissions - if stat_result.st_gid != 0: + 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 != 0: + if stat_result.st_uid >= 1000: chmod(path, original_mode | S_IWUSR) out, _ = run_le_auto_func() self.assertTrue('insecure permissions' in out)