Merge pull request #1883 from YunoHost/fix_default_permissions

[helpers v2.1] Rework _ynh_apply_default_permissions

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
 }

helpers/helpers.v2.1.d/utils

@@ -226,41 +226,61 @@ 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')
+# Apply sane permissions for files installed by ynh_setup_source and ynh_config_add.
-# on file/folders handled with ynh_setup_source and ynh_config_add
 #
 # [internal]
 #
-# Having a file others-readable or a folder others-executable(=enterable)
+# * Anything below $install_dir is chown $app:$app and chmod o-rwx,g-w
-# is a security risk comparable to "chmod 777"
+# * The rest is considered as system configuration and chown root, chmod 400
-#
-# 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.
 #
 _ynh_apply_default_permissions() {
     local target=$1
 
-    chmod o-rwx $target
+    is_in_dir() {
-    chmod g-w $target
+        # Returns false if parent is empty
-    chown -R root:root $target
+        [ -n "$2" ] || return 1
-    if ynh_system_user_exists --username=$app; then
+        local child=$(realpath "$1" 2>/dev/null)
-        chown $app:$app $target
+        local parent=$(realpath "$2" 2>/dev/null)
+        [[ "${child}" =~ ^$parent ]]
+    }
+
+    # 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:-}" || is_in_dir "$target" "/etc/$app")
+        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
+            # 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)
+            chmod -R u=rwX,g=r-X,o=--- "$target"
+            chown -R "$app:$group" "$target"
+            return
+        fi
     fi
 
-    # Crons should be owned by root
+    # Other files are considered system
-    # Also we don't want systemd conf, nginx conf or others stuff to be owned by the app,
+    chmod 400 "$target"
-    # otherwise they could self-edit their own systemd conf and escalate privilege
+    chown root:root "$target"
-    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
 }
 
 int_to_bool() {