Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically apply permissions for stuff provisioned with ynh_setup_source. #1955

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Automatically apply permissions for stuff provisioned with ynh_setup_source. #1955

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1949a8c
Update utils
orhtej2 Sep 27, 2024
e486d12
Update sources
orhtej2 Sep 27, 2024
5f9dc93
facepalm
orhtej2 Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions helpers/helpers.v2.1.d/sources
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,5 @@ ynh_setup_source() {
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
_ynh_apply_default_permissions $dest_dir
}
11 changes: 8 additions & 3 deletions helpers/helpers.v2.1.d/utils
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,14 @@ _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:-}" || is_in_dir "$target" "/etc/$app"); then
chmod 600 "$target"
chown "$app:$app" "$target"
if (is_in_dir "$target" "${install_dir:-}" || is_in_dir "$target" "${data_dir:-}" || is_in_dir "$target" "/etc/$app"); then
if [ -d "$target" ]; then
chmod -R o+rwX "$target"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a security hole. If we do ynh_setup_source or ynh_setup_source "$install_dir" we apply here a o+rwX permission by default, but we should not allowed other users to access this dir (by default) i guess.

In more adding a -R options onto data_dir could be looooong (if there is a lot of file), so this should be done only one time by yunohost install or upgrade...

I suggest to study all chown, chmod apps to understand what should be done by default and how give advice in the right way to packagers if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was supposed to be u not o x_x

chown -R "$app:$app" "$target"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be $app:$group.

I suggest to adapt if [ "$target" == "${install_dir:-}" ]; then below by replacing it with: if is_in_dir "$target" "${install_dir:-}" ; then

Same for data_dir (and maybe /etc/$app)

else
chmod 600 "$target"
chown "$app:$app" "$target"
fi
return
fi
# If this is the install dir (so far this is the only way this helper is called with a directory - along with $data_dir via ynh_restore?)
Expand Down
Loading