YunoHost / issues

General issue tracker for the YunoHost project
72 stars 8 forks source link

[SECURITY] Many apps allow global read permissions on the directory where they store the db password #1764

Closed Jules-Bertholet closed 2 years ago

Jules-Bertholet commented 3 years ago

Just with a very quick check of the apps I had installed on my server, I found 5 apps that allow all users to read their DB passwords: ampache, freshrss, pleroma, roundcube, and wallabag. There are certainly many more apps in the catalog with this problem.

Maybe package_check can add a check for this? sudo -u unprivilegeduser grep -r $db_pwd $final_path would catch most (but not all) cases

alexAubin commented 3 years ago

Yeah we were discussing this during last week's meeting ...

Jules-Bertholet commented 3 years ago

While updating some apps to harden their permissions, I noticed that the acl package (which provides setfacl) isn't installed on YunoHost by default, so apps have to install it to use it. Perhaps this should be changed

alexAubin commented 3 years ago

Yes, it will be installed on 4.2 by default: https://github.com/YunoHost/yunohost/blob/dev/debian/control#L29

But ideally one should refrain from using ACL for permissions on /var/www/$app and other "usual" locations because ACL are a bit more cryptic to handle than regular permissions (though of course more powerful)

Jules-Bertholet commented 3 years ago

But ideally one should refrain from using ACL for permissions on /var/www/$app and other "usual" locations because ACL are a bit more cryptic to handle than regular permissions (though of course more powerful)

When you need both www-data and an app-specific user to be able to read a file (very common), ACLs are the simplest solution. Otherwise you'd need to create a group and add both www-data and the app-specific user to it, and that modifies additional files outside the app directory

alexAubin commented 3 years ago

What about just having chown $app:www-data ?

Jules-Bertholet commented 3 years ago

What about just having chown $app:www-data ?

That doesn't work when you want app to have read access only (as if it owns the file it can modify the permissions)

Jules-Bertholet commented 3 years ago

On a related note, the ACL for the yunohost.multimedia folders are also a bit lax, as everyone can read the contents of the entire folder. And admin can read/write everything, which is convenient but maybe not ideal for security (though I'm not sure if this matters that much). Ideally, Yunohost users should have access only to their own folder, and the shared one which should be owned by all-users (or maybe a new shared-multimedia-users group?), with the multimedia group getting read-write access to everything. Maybe we should also make a multimedia-readonly group, for apps that need read access only?

alexAubin commented 3 years ago

Hmpf yeah I don't know what to think about this. The main thread model we're talking about here "a random user should not be able to read administration secrets or confidential files, because that leads to priviledge escalation or personal information leak which is catastrophically 'not cool'.". A good protection measure is to just have chown o-rwx on /var/www/$app and other important app locations.

Then yes, we can discuss "advanced" threat models such as "what could an external attacker do if they compromise an app and obtain arbitrary command execution ?". I'm not sure exactly what we want to try to protect in that case exactly ... I guess yeah, you could have some situation where you want $app to only have readonly access to some file, but then just set chown root:root for that file/folder (assuming they are inside a parent that has o-rwx of course) ...

I mean maybe in some cases we do need ACL, but to me in sounds that in most cases the regular permission are enough and it's not worth the cost in tems of complexity to add ACL into the mix ... But maybe you have some "common situation" in mind

Jules-Bertholet commented 3 years ago

Making apps have read-only access to a lot of their own files can help limit damage in some cases even without anything getting hacked. For example, Ampache comes with a self-update feature which overwrites a lot of its own files. This self-update feature doesn't always work out nice on Yunohost (missing dependencies, overwritten custom configs). Making Ampache's own files read-only, other than the data files it needs to be able to modify, could help mitigate fallout from uninformed users who press Ampache's big "update to new version" button.

alexAubin commented 2 years ago

I'm closing this as it's been sortof addressed over the past X months now ... there are now many warnings in the CI about this and apps get downgraded to level 6 (I think ?) if permissions are wide open ... ultimately we should flag them as level 4, maybe in a few months