From 8846381d4758dd891ba7cc218778851669069109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Pi=C3=A9dallu?= Date: Fri, 28 Jun 2024 14:43:46 +0200 Subject: [PATCH 1/9] Rework _ynh_apply_default_permissions, only check if target is a child of install_dir. --- helpers/helpers.v2.1.d/utils | 47 ++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index be57a6d73..ff378a951 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -226,41 +226,36 @@ ynh_app_upgrading_from_version_before_or_equal_to() { dpkg --compare-versions $YNH_APP_CURRENT_VERSION le $version } -# Check if we should enforce sane default permissions (= disable rwx for 'others') -# on file/folders handled with ynh_setup_source and ynh_config_add +# Apply sane permissions for files installed by ynh_setup_source and ynh_config_add. # # [internal] # -# Having a file others-readable or a folder others-executable(=enterable) -# is a security risk comparable to "chmod 777" -# -# Configuration files may contain secrets. Or even just being able to enter a -# folder may allow an attacker to do nasty stuff (maybe a file or subfolder has -# some write permission enabled for 'other' and the attacker may edit the -# content or create files as leverage for priviledge escalation ...) -# -# The sane default should be to set ownership to $app:$app. -# In specific case, you may want to set the ownership to $app:www-data -# for example if nginx needs access to static files. +# * Anything below $install_dir is chown $app:$app and chmod o-rwx,g-w +# * The rest is considered as system configuration and chown root, chmod 400 # _ynh_apply_default_permissions() { local target=$1 - chmod o-rwx $target - chmod g-w $target - chown -R root:root $target - if ynh_system_user_exists --username=$app; then - chown $app:$app $target + is_subdir() { + # Returns false if child or parent is empty + child=$(realpath "$1" 2>/dev/null) + parent=$(realpath "$2" 2>/dev/null) + [[ "${child/$parent/}" != "$child" ]] + } + + # App files can have files of their own + if ynh_system_user_exists --username="$app"; then + if is_subdir "$target" "$install_dir" || is_subdir "$target" "$data_dir"; then + chmod -R u=rwX,g=rX,o=X "$target" + chown -R "$app:$app" "$target" + chown "$app:www-data" "$target" + return + fi fi - # Crons should be owned by root - # Also we don't want systemd conf, nginx conf or others stuff to be owned by the app, - # otherwise they could self-edit their own systemd conf and escalate privilege - if echo "$target" | grep -q '^/etc/cron\|/etc/php\|/etc/nginx/conf.d\|/etc/fail2ban\|/etc/systemd/system' - then - chmod 400 $target - chown root:root $target - fi + # Other files are considered system + chmod -R 400 "$target" + chown -R root:root "$target" } int_to_bool() { From d9d404a5b24f1745f414a1bce47fda3c44007d1e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 16:06:40 +0200 Subject: [PATCH 2/9] ynh_setup_source: apply default perms *after* extracting files to hopefully remove the need to manually chown/chmod --- helpers/helpers.v2.1.d/sources | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helpers/helpers.v2.1.d/sources b/helpers/helpers.v2.1.d/sources index ced0e05c7..c0c2fb863 100644 --- a/helpers/helpers.v2.1.d/sources +++ b/helpers/helpers.v2.1.d/sources @@ -182,10 +182,6 @@ ynh_setup_source() { # Extract source into the app dir mkdir --parents "$dest_dir" - if [ -n "${install_dir:-}" ] && [ "$dest_dir" == "$install_dir" ]; then - _ynh_apply_default_permissions $dest_dir - fi - if [[ "$src_extract" == "false" ]]; then if [[ -z "$src_rename" ]] then @@ -258,4 +254,8 @@ ynh_setup_source() { done fi rm -rf /var/cache/yunohost/files_to_keep_during_setup_source/ + + if [ -n "${install_dir:-}" ] && [ "$dest_dir" == "$install_dir" ]; then + _ynh_apply_default_permissions $dest_dir + fi } From 3608c5678c145ce4b02caca350c913ef67a1a41f Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 16:45:43 +0200 Subject: [PATCH 3/9] Proper 'if' cases to distinguish between $install_dir vs regular files in $install_dir and $data_dir --- helpers/helpers.v2.1.d/utils | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index ff378a951..6c63fe82d 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -236,7 +236,7 @@ ynh_app_upgrading_from_version_before_or_equal_to() { _ynh_apply_default_permissions() { local target=$1 - is_subdir() { + is_in_dir() { # Returns false if child or parent is empty child=$(realpath "$1" 2>/dev/null) parent=$(realpath "$2" 2>/dev/null) @@ -245,17 +245,27 @@ _ynh_apply_default_permissions() { # App files can have files of their own if ynh_system_user_exists --username="$app"; then - if is_subdir "$target" "$install_dir" || is_subdir "$target" "$data_dir"; then - chmod -R u=rwX,g=rX,o=X "$target" - chown -R "$app:$app" "$target" - chown "$app:www-data" "$target" + # If this is a file in $install_dir or $data_dir : it should be owned and read+writable by $app only + if [ -f "$target" ] && (([[ -z "${install_dir:-}" ]] is_in_dir "$target" "$install_dir") || ([[ -z "${install_dir:-}" ]] is_in_dir "$target" "$data_dir")) + then + chmod 600 "$target" + chown "$app:$app" "$target" + return + fi + # If this is the install dir (so far this is the only way this helper is called with a directory) + if [ "$target" == "$install_dir" ] + then + # Files inside should be owned by $app/www-data with rw-r----- (+x for folders or files that already have +x) + chmod -R u=rwX,g=r-X,o=--- "$target" + # We set the group to www-data because most apps do serve static assets that need to be readable by nginx ... + chown -R "$app:www-data" "$target" return fi fi # Other files are considered system - chmod -R 400 "$target" - chown -R root:root "$target" + chmod 400 "$target" + chown root:root "$target" } int_to_bool() { From 3b26ccc2a542c67769af5b7150137500dd1eb26f Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 16:55:39 +0200 Subject: [PATCH 4/9] Properly handle case where $parent is empty to simplify condition --- helpers/helpers.v2.1.d/utils | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index 6c63fe82d..af2859d78 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -237,23 +237,24 @@ _ynh_apply_default_permissions() { local target=$1 is_in_dir() { - # Returns false if child or parent is empty - child=$(realpath "$1" 2>/dev/null) - parent=$(realpath "$2" 2>/dev/null) + # Returns false if parent is empty + [ -n "$2" ] || return 1 + local child=$(realpath "$1" 2>/dev/null) + local parent=$(realpath "$2" 2>/dev/null) [[ "${child/$parent/}" != "$child" ]] } # App files can have files of their own if ynh_system_user_exists --username="$app"; then # If this is a file in $install_dir or $data_dir : it should be owned and read+writable by $app only - if [ -f "$target" ] && (([[ -z "${install_dir:-}" ]] is_in_dir "$target" "$install_dir") || ([[ -z "${install_dir:-}" ]] is_in_dir "$target" "$data_dir")) + if [ -f "$target" ] && (is_in_dir "$target" "${install_dir:-}" || is_in_dir "$target" "${data_dir:-}") then chmod 600 "$target" chown "$app:$app" "$target" return fi # If this is the install dir (so far this is the only way this helper is called with a directory) - if [ "$target" == "$install_dir" ] + if [ "$target" == "${install_dir:-}" ] then # Files inside should be owned by $app/www-data with rw-r----- (+x for folders or files that already have +x) chmod -R u=rwX,g=r-X,o=--- "$target" From 75d704297470d358af7d6864df76f050f5fda7b8 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin <4533074+alexAubin@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:56:28 +0200 Subject: [PATCH 5/9] Update helpers/helpers.v2.1.d/utils: use regex matching to check if path is child from a parent path --- helpers/helpers.v2.1.d/utils | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index af2859d78..2ee0b5d38 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -241,7 +241,7 @@ _ynh_apply_default_permissions() { [ -n "$2" ] || return 1 local child=$(realpath "$1" 2>/dev/null) local parent=$(realpath "$2" 2>/dev/null) - [[ "${child/$parent/}" != "$child" ]] + [[ "${child}" =~ ^$parent ]] } # App files can have files of their own From 8b8768fd776f9d15efd5aeb9d65cd943c0bfc920 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 18:09:35 +0200 Subject: [PATCH 6/9] Only set www-data as group for webapps --- helpers/helpers.v2.1.d/utils | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index 2ee0b5d38..f03e67804 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -258,8 +258,13 @@ _ynh_apply_default_permissions() { then # Files inside should be owned by $app/www-data with rw-r----- (+x for folders or files that already have +x) chmod -R u=rwX,g=r-X,o=--- "$target" + local group="$app" # We set the group to www-data because most apps do serve static assets that need to be readable by nginx ... - chown -R "$app:www-data" "$target" + # The fact that the app is a webapp is infered by the fact that $domain and $path are defined + if [[ -n "${domain:-}" ]] && [[ -n "${path:-}" ]] then + group="www-data" + fi + chown -R "$app:$group" "$target" return fi fi From ae3018cdd0d97f4f56117fdc80bacb9637e5426e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 18:39:12 +0200 Subject: [PATCH 7/9] Infer the necessity to use www-data as group from the presence of alias or root in nginx.conf --- helpers/helpers.v2.1.d/utils | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index f03e67804..05a859498 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -256,14 +256,15 @@ _ynh_apply_default_permissions() { # If this is the install dir (so far this is the only way this helper is called with a directory) if [ "$target" == "${install_dir:-}" ] then - # Files inside should be owned by $app/www-data with rw-r----- (+x for folders or files that already have +x) - chmod -R u=rwX,g=r-X,o=--- "$target" local group="$app" - # We set the group to www-data because most apps do serve static assets that need to be readable by nginx ... - # The fact that the app is a webapp is infered by the fact that $domain and $path are defined - if [[ -n "${domain:-}" ]] && [[ -n "${path:-}" ]] then + # We set the group to www-data for webapps that do serve static assets, which therefore need to be readable by nginx ... + # The fact that the app needs this is infered by the existence of an nginx.conf and the presence of "alias" or "root" directive + if grep -q '^\s*alias\s\|^\s*root\s' "$YNH_APP_BASEDIR/conf/nginx.conf" 2>/dev/null then group="www-data" fi + # Files inside should be owned by $app with rw-r----- (+x for folders or files that already have +x) + # The group needs read/dirtraversal (in particular if it's www-data) + chmod -R u=rwX,g=r-X,o=--- "$target" chown -R "$app:$group" "$target" return fi From 656ff823a914a15741be2e756d3f5dbcdb893184 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 18:56:18 +0200 Subject: [PATCH 8/9] Also handle files in /etc/$app --- helpers/helpers.v2.1.d/utils | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index 05a859498..73b32a3f3 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -247,7 +247,7 @@ _ynh_apply_default_permissions() { # App files can have files of their own if ynh_system_user_exists --username="$app"; then # If this is a file in $install_dir or $data_dir : it should be owned and read+writable by $app only - if [ -f "$target" ] && (is_in_dir "$target" "${install_dir:-}" || is_in_dir "$target" "${data_dir:-}") + if [ -f "$target" ] && (is_in_dir "$target" "${install_dir:-}" || is_in_dir "$target" "${data_dir:-}" || is_in_dir "$target" "/etc/$app") then chmod 600 "$target" chown "$app:$app" "$target" From ef68485c5fc4fea221e13e05b6ed638ab519dc2f Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Fri, 28 Jun 2024 19:24:07 +0200 Subject: [PATCH 9/9] Use the group defined in the manifest by default --- helpers/helpers.v2.1.d/utils | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/helpers/helpers.v2.1.d/utils b/helpers/helpers.v2.1.d/utils index 73b32a3f3..ad7a7620d 100644 --- a/helpers/helpers.v2.1.d/utils +++ b/helpers/helpers.v2.1.d/utils @@ -256,11 +256,19 @@ _ynh_apply_default_permissions() { # If this is the install dir (so far this is the only way this helper is called with a directory) if [ "$target" == "${install_dir:-}" ] then - local group="$app" - # We set the group to www-data for webapps that do serve static assets, which therefore need to be readable by nginx ... - # The fact that the app needs this is infered by the existence of an nginx.conf and the presence of "alias" or "root" directive - if grep -q '^\s*alias\s\|^\s*root\s' "$YNH_APP_BASEDIR/conf/nginx.conf" 2>/dev/null then - group="www-data" + # Read the group from the install_dir manifest resource + local group="$(ynh_read_manifest '.resources.install_dir.group' | sed 's/null//g' | sed "s/__APP__/$app/g" | cut -f1 -d:)" + if [[ -z "$group" ]] + then + # We set the group to www-data for webapps that do serve static assets, which therefore need to be readable by nginx ... + # The fact that the app needs this is infered by the existence of an nginx.conf and the presence of "alias" or "root" directive + if grep -q '^\s*alias\s\|^\s*root\s' "$YNH_APP_BASEDIR/conf/nginx.conf" 2>/dev/null; + then + group="www-data" + # Or default to "$app" + else + group="$app" + fi fi # Files inside should be owned by $app with rw-r----- (+x for folders or files that already have +x) # The group needs read/dirtraversal (in particular if it's www-data)