Hello,

I'm trying to maintain 2 packages I'm using with Alpine and would not 
like to see being removed from the repositories from future releases.
But I could see that there is some basic problem.
Currently you are unlocking users in pre-install of packages without any 
further checks of the existing system environment.
There is assumed the user is not existing, there is no username clash, 
the user has not set a password, the user is used only for this package 
and so on.
In short... this is a no-go to circumvent any administrative security 
related restrictions by package installations.
There is the possibility to allow an unintended (remote) login or local 
privilege expansion by unlocking users in apk-executed scripts.
And there is no sensitivity for this problem, because it is the 
recommended way of providing packages. (Quote: "see the <...apk> 
.pre-install, which is how all of them are done").

I'm negatively surprised how careless the basic system permissions are 
used.

Are you aware of this situation in Alpine and happy with it?

Markus

How would you improve this situation? Fail package installation if
user exists and is non-system (id >= 1000)?
How do other distros solve conflicts between admin's usernames and
service's usernames?

On Sat, Jun 18, 2022 at 1:00 PM Markus Kolb
<alpinelinux+develml@tower-net.de> wrote:
>
> Hello,
>
> I'm trying to maintain 2 packages I'm using with Alpine and would not
> like to see being removed from the repositories from future releases.
> But I could see that there is some basic problem.
> Currently you are unlocking users in pre-install of packages without any
> further checks of the existing system environment.
> There is assumed the user is not existing, there is no username clash,
> the user has not set a password, the user is used only for this package
> and so on.
> In short... this is a no-go to circumvent any administrative security
> related restrictions by package installations.
> There is the possibility to allow an unintended (remote) login or local
> privilege expansion by unlocking users in apk-executed scripts.
> And there is no sensitivity for this problem, because it is the
> recommended way of providing packages. (Quote: "see the <...apk>
> .pre-install, which is how all of them are done").
>
> I'm negatively surprised how careless the basic system permissions are
> used.
>
> Are you aware of this situation in Alpine and happy with it?
>
> Markus

Am 18.06.2022 13:43, schrieb Konstantin Kulikov:
> How would you improve this situation? Fail package installation if
> user exists and is non-system (id >= 1000)?
> How do other distros solve conflicts between admin's usernames and
> service's usernames?

Hi Konstantin,

this is not the direction I'd like to think about.
You can't rely on configurable settings (uid for users starting with 
1000 or the area where sys uids/gids are requested).

Some distros create system users only with predefined uid/gid. So if you 
want to create a user or group during package installation you request a 
fix uid/gid for usage and need to use special commands to create the 
user during build, not the admin commands.
Installation of the built package, in restricted build environments for 
audit, finds misbehaving installation procedures.
So scripts of packages may work on the files and directories of the 
package they belong to. Anything else should be part of distro's base 
packages or the package build system.

A first effective improvement would be, not to lower/remove restrictions 
of _any_ existing user in a kind of black box (installation script from 
admin perspective) during package installation.
Same proceeding would be good for file/directory permissions.
For this there should be also taken care in e.g. checkpath of 
openrc-scripts. If the admin locks out users of a directory and during 
the next service start by a checkpath the directory becomes world 
accessible again, might be a problem. IMO openrc should not modify or 
create anything by checkpath but simply report about the undesired 
mismatch and abort. But there is no such option in checkpath to only 
check. The modification is implied, the name of the function is 
misleading. It is a "fixpath".

I don't see a requirement for adding users like gitea/gogs to a 
website-content-group www-data. The hook scripts of the code 
repositories might be allowed to read files (e.g. htpasswd) of webserver 
environments they don't need to. If www-data group is used in multi-user 
environments for editing the website content by multiple users, hook 
scripts could modify group-writable files below /var/www. Who is aware 
of this when installing one of these packages?
I've found a commit comment from 2021 saying 20 aports would add its 
users to the www-data group... are these all webservers owning content 
under /var/www for delivering and I stumbled over the only two black 
sheep named above?
Will they stay out of the group if the admin remove them manually or are 
they put back in during the next fresh installation/update?
What is happening below /var/log, /var/run in all the packages over time 
if there is no check what packages are doing?

Btw. is it intended that you add user to the same group it has as 
primary group, so that it has ineffectively the same group 2 times?

I really don't know what should be done now with a package like gogs, 
which has up to now a walking through directory structure in Alpine 
where at least every user on the system can read the database with 
registered users, emails and hashed passwords. And any of these users 
with membership of www-data group could have modified the config file of 
the service and with changing repository path the code in repository 
could have been untrustworthy modified.
Default installations need to be fixed here, but it is not foreseeable 
what configuration changes has been done on all the installations out 
there. Overwriting this more restrictive and safe can break 
installations using a different, custom configuration and directory 
structure.
There could be provided an output to the user during the installation 
about the changes. In other distributions there would be a confirmation 
dialog.
How this should be done in Alpine?

> Some distros create system users only with predefined uid/gid.

There are ~500 *-openrc packages so I guess it can work.

> A first effective improvement would be, not to lower/remove restrictions
> of _any_ existing user in a kind of black box (installation script from
> admin perspective) during package installation.

adduser fails if user exists so I'm not sure what you mean here.

> Same proceeding would be good for file/directory permissions.
> For this there should be also taken care in e.g. checkpath of
> openrc-scripts. If the admin locks out users of a directory and during
> the next service start by a checkpath the directory becomes world
> accessible again, might be a problem. IMO openrc should not modify or
> create anything by checkpath but simply report about the undesired
> mismatch and abort. But there is no such option in checkpath to only
> check. The modification is implied, the name of the function is
> misleading. It is a "fixpath".

I don't agree that admin should be required to manually create
directories with correct permissions.
Maybe it's possible to checkpath with 700 and then give permissions to
others when required using ACLs(setfacl/getfacl)?

> I don't see a requirement for adding users like gitea/gogs to a
> website-content-group www-data.

I believe "someuser/www-data" is usually done to let apache/nginx
handle uploads and static file serving.
In gogs case this definitely shouldn't be done. Feel free to send a MR.

> What is happening below /var/log, /var/run in all the packages over time
> if there is no check what packages are doing?

Logging in alpine is in a shit state because openrc doesn't really
implement logging properly.
There was some talk about replacing it, no idea what state it is currently in.

> Btw. is it intended that you add user to the same group it has as
> primary group, so that it has ineffectively the same group 2 times?

Unlikely. Feel free to send MRs.

> Default installations need to be fixed here, but it is not foreseeable
> what configuration changes has been done on all the installations out
> there. Overwriting this more restrictive and safe can break
> installations using a different, custom configuration and directory
> structure.
> There could be provided an output to the user during the installation
> about the changes. In other distributions there would be a confirmation
> dialog.

apk doesn't do notifications. Postgres did fail installation in the
past when a major version was released,
but thankfully major postgres versions are now separate packages.

> How this should be done in Alpine?

I'd say get rid of www-data, but leave 755 for now.
Move directory creation from package() to initd script with checkpath.
This will let you edit initd on your installation and not fear for it
being overwritten on the next update.
You can get rid of setcap usage as well (see [1]).

Finally bring up your concerns and suggestions with TSC here [2] and
start a formal discussion.

[1] https://gitlab.alpinelinux.org/alpine/tsc/-/issues/45
[2] https://gitlab.alpinelinux.org/alpine/tsc

On Sun, Jun 19, 2022 at 04:42:35PM +0300, Konstantin Kulikov wrote:
> 
> > How this should be done in Alpine?
> 
> I'd say get rid of www-data, but leave 755 for now.
> Move directory creation from package() to initd script with checkpath.
> This will let you edit initd on your installation and not fear for it
> being overwritten on the next update.
> You can get rid of setcap usage as well (see [1]).

Note that this will most like break container installations, which do no
run services. So you cannot rely on checkpath in an init script to
created required directories.

Hi,

> I don't agree that admin should be required to manually create directories with correct permissions.

Me neither.

>> There could be provided an output to the user during the installation about the changes.
> apk doesn't do notifications.

That’s not entirely true, we use post-upgrade scripts to inform users (print messages to stderr) about important changes when upgrading packages. It’s not ideal though.

> Logging in alpine is in a shit state because openrc doesn't really implement logging properly.

I don’t know how do you define proper logging implementation, but maybe you don’t know about `output_log` and `error_log` parameters. You can use it to “redirect” stdout/stderr to syslog using logger(1) command. See kresd.initd [1] for example.


Jakub

[1]: https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/community/knot-resolver/kresd.initd


On 6/19/22 3:42 PM, Konstantin Kulikov wrote:
>> Some distros create system users only with predefined uid/gid.
> 
> There are ~500 *-openrc packages so I guess it can work.
> 
>> A first effective improvement would be, not to lower/remove restrictions
>> of _any_ existing user in a kind of black box (installation script from
>> admin perspective) during package installation.
> 
> adduser fails if user exists so I'm not sure what you mean here.
> 
>> Same proceeding would be good for file/directory permissions.
>> For this there should be also taken care in e.g. checkpath of
>> openrc-scripts. If the admin locks out users of a directory and during
>> the next service start by a checkpath the directory becomes world
>> accessible again, might be a problem. IMO openrc should not modify or
>> create anything by checkpath but simply report about the undesired
>> mismatch and abort. But there is no such option in checkpath to only
>> check. The modification is implied, the name of the function is
>> misleading. It is a "fixpath".
> 
> I don't agree that admin should be required to manually create
> directories with correct permissions.
> Maybe it's possible to checkpath with 700 and then give permissions to
> others when required using ACLs(setfacl/getfacl)?
> 
>> I don't see a requirement for adding users like gitea/gogs to a
>> website-content-group www-data.
> 
> I believe "someuser/www-data" is usually done to let apache/nginx
> handle uploads and static file serving.
> In gogs case this definitely shouldn't be done. Feel free to send a MR.
> 
>> What is happening below /var/log, /var/run in all the packages over time
>> if there is no check what packages are doing?
> 
> Logging in alpine is in a shit state because openrc doesn't really
> implement logging properly.
> There was some talk about replacing it, no idea what state it is currently in.
> 
>> Btw. is it intended that you add user to the same group it has as
>> primary group, so that it has ineffectively the same group 2 times?
> 
> Unlikely. Feel free to send MRs.
> 
>> Default installations need to be fixed here, but it is not foreseeable
>> what configuration changes has been done on all the installations out
>> there. Overwriting this more restrictive and safe can break
>> installations using a different, custom configuration and directory
>> structure.
>> There could be provided an output to the user during the installation
>> about the changes. In other distributions there would be a confirmation
>> dialog.
> 
> apk doesn't do notifications. Postgres did fail installation in the
> past when a major version was released,
> but thankfully major postgres versions are now separate packages.
> 
>> How this should be done in Alpine?
> 
> I'd say get rid of www-data, but leave 755 for now.
> Move directory creation from package() to initd script with checkpath.
> This will let you edit initd on your installation and not fear for it
> being overwritten on the next update.
> You can get rid of setcap usage as well (see [1]).
> 
> Finally bring up your concerns and suggestions with TSC here [2] and
> start a formal discussion.
> 
> [1] https://gitlab.alpinelinux.org/alpine/tsc/-/issues/45
> [2] https://gitlab.alpinelinux.org/alpine/tsc
> 

> There is the possibility to allow an unintended (remote) login or local privilege expansion by unlocking users in apk-executed scripts.

No, if the user already exists, then adduser(8) does nothing.

> Are you aware of this situation in Alpine and happy with it?

I’m not. I’d prefer a declarative approach – needed users/groups declared in APKBUILD, so abuild can check if they meet some requirements, and also to be easily auditable. However, it doesn’t bother me enough to actually do the job and implement it…
And even this would not prevent package maintainers from doing stupid things, such as making some directory group-writable for www-data to allow running some PHP app with Apache mod_php like in ’90s instead of using php-fpm (with an unique user for each app)…

Jakub

On 6/18/22 12:00 PM, Markus Kolb wrote:
> Hello,
> 
> I'm trying to maintain 2 packages I'm using with Alpine and would not like to see being removed from the repositories from future releases.
> But I could see that there is some basic problem.
> Currently you are unlocking users in pre-install of packages without any further checks of the existing system environment.
> There is assumed the user is not existing, there is no username clash, the user has not set a password, the user is used only for this package and so on.
> In short... this is a no-go to circumvent any administrative security related restrictions by package installations.
> There is the possibility to allow an unintended (remote) login or local privilege expansion by unlocking users in apk-executed scripts.
> And there is no sensitivity for this problem, because it is the recommended way of providing packages. (Quote: "see the <...apk> .pre-install, which is how all of them are done").
> 
> I'm negatively surprised how careless the basic system permissions are used.
> 
> Are you aware of this situation in Alpine and happy with it?
> 
> Markus

>Note that this will most like break container installations, which do no
>run services. So you cannot rely on checkpath in an init script to
>created required directories.

Do people actually use alpine in this way?
That would be really surprising to me.
And I have heard no complaints about grafana for example.

>I don’t know how do you define proper logging implementation, but maybe you don’t know about `output_log` and `error_log` parameters. You can use it to “redirect” stdout/stderr to syslog using logger(1) command. See kresd.initd [1] for example.

If only error_logger worked with supervise-daemon.
Even then it does work enough for my small installation, but it is far
from ideal - for example everybody has write access to /dev/log or if
logger is killed service will stall or silently drop logs.
Configuration UI is also bad - you should be able to just set where
logs go right in conf.d.
And no error_log is not the answer, as it requires logfiles to be
owned by service and cannot rotate logs at all.

>Logging in alpine is in a shit state because openrc doesn't really
>implement logging properly.
>There was some talk about replacing it, no idea what state it is currently in.

  It's still some time away, but the parts of the replacement that
provide better logging are already available on Alpine and could be
used today. It's not yet the case because devs prefer not mix-and-
matching tools and would rather have the full openrc replacement
before performing the switch.

  I have to say I don't really understand why this discussion has gone
to such depths and with such ramifications.

  The issue seems to be that users can squat user/group names that
conflict with later installations of Alpine packages; my probably
unpopular opinion is that it's *fine*.
  Administrators are root on their machines, they can do whatever they
want, including squatting user and group names. What packagers should do
is document the user and group names that their packages use; Alpine
could aggregate and publish the list, and then defer to administrators
the responsibility of making sure there are no conflicts before
user-defining packages are installed, under penality of risking security
weaknesses.
  Documentation is often better than coercion.

--
  Laurent

On Sun Jun 19, 2022 at 3:42 PM CEST, Konstantin Kulikov wrote:
> > Btw. is it intended that you add user to the same group it has as
> > primary group, so that it has ineffectively the same group 2 times?
>
> Unlikely. Feel free to send MRs.
for this specifically, i assume it was something like:

addgroup -S x
adduser -S -G x x

`adduser -S` does not create a group for the user, so this is required.
haven't seen any other instances where that statement would apply..

Am 19.06.2022 18:54, schrieb Jakub Jirutka:
> Hi,
> 
>> I don't agree that admin should be required to manually create 
>> directories with correct permissions.
> 
> Me neither.
> 

Never requested anything like this...

But rc-scripts shouldn't overwrite permissions in an unsafe manner, and 
this can only be achieved if they don't modify clandestinely 
permissions.
The directories have to be created with correctly set permissions during 
package installation.
You can inform the admin during startup that there is something wrong, 
and the service can not start because of this, which is also expressed 
by the function name "check...". It doesn't say fix, modify, repair or 
anything else explaining that this is a mutable process.

On Tue, 21 Jun 2022 10:43:37 +0200
Markus Kolb <alpinelinux+develml@tower-net.de> wrote:

> Am 19.06.2022 18:54, schrieb Jakub Jirutka:
> > Hi,
> >   
> >> I don't agree that admin should be required to manually create 
> >> directories with correct permissions.  
> > 
> > Me neither.
> >   
> 
> Never requested anything like this...
> 
> But rc-scripts shouldn't overwrite permissions in an unsafe manner, and 
> this can only be achieved if they don't modify clandestinely 
> permissions.

Can you please explain what you mean with "an unsafe manner"?

> The directories have to be created with correctly set permissions during 
> package installation.
>
> You can inform the admin during startup that there is something wrong, 
> and the service can not start because of this, which is also expressed 
> by the function name "check...". It doesn't say fix, modify, repair or 
> anything else explaining that this is a mutable process.

What about services that needs write permissions for sockets in
directories under /run, which is a tmpfs and gets wiped each reboot?

Should the admin need to change the permissions for those services
every reboot?

-nc

On Sat, 18 Jun 2022 12:00:38 +0200
Markus Kolb <alpinelinux+develml@tower-net.de> wrote:

> Hello,
> 
> I'm trying to maintain 2 packages I'm using with Alpine and would not 
> like to see being removed from the repositories from future releases.
> But I could see that there is some basic problem.
> Currently you are unlocking users in pre-install of packages without any 
> further checks of the existing system environment.

Where are users unlocked in pre-install?

...

> There is the possibility to allow an unintended (remote) login or local 
> privilege expansion by unlocking users in apk-executed scripts.

Can you please explain with an example how that would work?

I as an admin create a user called 'foo' and then a package with a
service tries to create the same user 'foo'? Using adduser? or using
passwd?

> And there is no sensitivity for this problem, because it is the 
> recommended way of providing packages. (Quote: "see the <...apk> 
> .pre-install, which is how all of them are done").
> 
> I'm negatively surprised how careless the basic system permissions are 
> used.
> 
> Are you aware of this situation in Alpine and happy with it?

Apparently I was not aware of the severity of the situation. no.

Can you please give me an example of how this could be exploited by an
attacker?

> Markus

Hi,

On Sat, 18 Jun 2022, Markus Kolb wrote:

> Hello,
>
> I'm trying to maintain 2 packages I'm using with Alpine and would not like to 
> see being removed from the repositories from future releases.
> But I could see that there is some basic problem.
> Currently you are unlocking users in pre-install of packages without any 
> further checks of the existing system environment.
> There is assumed the user is not existing, there is no username clash, the 
> user has not set a password, the user is used only for this package and so 
> on.

This is a problem with adduser in busybox, then, and it should be fixed 
there.  In the past adduser was performing these checks, so something has 
changed.

> In short... this is a no-go to circumvent any administrative security related 
> restrictions by package installations.
> There is the possibility to allow an unintended (remote) login or local 
> privilege expansion by unlocking users in apk-executed scripts.
> And there is no sensitivity for this problem, because it is the recommended 
> way of providing packages. (Quote: "see the <...apk> .pre-install, which is 
> how all of them are done").
>
> I'm negatively surprised how careless the basic system permissions are used.
>
> Are you aware of this situation in Alpine and happy with it?

I am aware of this situation, but am skeptical that it is an actual 
security problem (unless new versions of busybox have actually broken 
adduser).  That awareness is why we are moving user/group management 
directly to apk-tools as part of apk 3.

Ariadne

Hello Markus,

I've read thru the entire conversation - the problem you are drawing 
isn't one.

1. The passwd calls have an adduser call right above them, creating a 
system user with that name.
That fails if the user already exists and would return a non-zero return 
code. Thereby the package installation fails.

2. When you would create those users yourself it would fail too.
So the only reason your procedure would work is when an administrator 
decides to give you access to a system user and change the shell wrapper 
for that user.

3. This could also occur when you change the running user of gitea / 
gogs in their respective config.

All of this is the responsibility of the system administrator.
This is NOT an exploit the same way "I can change my root password to 
123456" is not an exploit.

Am I missing something here?
- Paul

Am 18.06.22 um 12:00 schrieb Markus Kolb:
> Hello,
>
> I'm trying to maintain 2 packages I'm using with Alpine and would not 
> like to see being removed from the repositories from future releases.
> But I could see that there is some basic problem.
> Currently you are unlocking users in pre-install of packages without 
> any further checks of the existing system environment.
> There is assumed the user is not existing, there is no username clash, 
> the user has not set a password, the user is used only for this 
> package and so on.
> In short... this is a no-go to circumvent any administrative security 
> related restrictions by package installations.
> There is the possibility to allow an unintended (remote) login or 
> local privilege expansion by unlocking users in apk-executed scripts.
> And there is no sensitivity for this problem, because it is the 
> recommended way of providing packages. (Quote: "see the <...apk> 
> .pre-install, which is how all of them are done").
>
> I'm negatively surprised how careless the basic system permissions are 
> used.
>
> Are you aware of this situation in Alpine and happy with it?
>
> Markus

Am 22.06.2022 14:14, schrieb Paul Zillmann:
> Hello Markus,
> 
> I've read thru the entire conversation - the problem you are drawing 
> isn't one.
> 
> 1. The passwd calls have an adduser call right above them, creating a
> system user with that name.
> That fails if the user already exists and would return a non-zero
> return code. Thereby the package installation fails.

This is not true. And the rest is irrelevant. It is not the admin doing 
anything wrong. The packages are installed unsafe, and if the admin 
wants to repair it, the packages mess it up again. (Relevant for the 
other problems with permissions, file and group owners.)

The adduser doesn't fail, the pre-install is not aborted, unlock is 
happening over and over again, the installation doesn't fail.
This would also not be possible to do, you couldn't uninstall and 
reinstall packages.

Maybe you should try out before talking about, like all the others 
seeing no problem and had not even a look in the repository or tried an 
installation to see where the problems are.
Because I'm tired to explain over and over again that it is like I say.

Last of my comments.

Hi,

On Wed, 22 Jun 2022, Markus Kolb wrote:

> Am 22.06.2022 14:14, schrieb Paul Zillmann:
>> Hello Markus,
>> 
>> I've read thru the entire conversation - the problem you are drawing isn't 
>> one.
>> 
>> 1. The passwd calls have an adduser call right above them, creating a
>> system user with that name.
>> That fails if the user already exists and would return a non-zero
>> return code. Thereby the package installation fails.
>
> This is not true. And the rest is irrelevant. It is not the admin doing 
> anything wrong. The packages are installed unsafe, and if the admin wants to 
> repair it, the packages mess it up again. (Relevant for the other problems 
> with permissions, file and group owners.)

This is why apk-tools 3 is directly gaining the ability to do account 
management, so that maintainer scripts are not adding/removing accounts.

In other words: we are already doing something about this.

> The adduser doesn't fail, the pre-install is not aborted, unlock is happening 
> over and over again, the installation doesn't fail.
> This would also not be possible to do, you couldn't uninstall and reinstall 
> packages.

The unlock of a specified account, e.g. gogs or gitea, which must be 
unlocked for the package to work as intended (e.g. allow remote SSH git 
pushes).

And whether that account is locked or not does not actually have any 
influence over your data exfiltration scenario outlined elsewhere.

> Maybe you should try out before talking about, like all the others seeing no 
> problem and had not even a look in the repository or tried an installation to 
> see where the problems are.

I took a look before I concluded it was not a problem.  I still say it is 
not a problem.  Your data exfiltration scenario has nothing to do with 
whether the account is locked or not.

Ariadne

On Wed, 22 Jun 2022 14:14:59 +0200
Paul Zillmann <p.zillmann@h6g.de> wrote:

> Hello Markus,
> 
> I've read thru the entire conversation - the problem you are drawing 
> isn't one.
> 
> 1. The passwd calls have an adduser call right above them, creating a 
> system user with that name.
> That fails if the user already exists and would return a non-zero return 
> code. Thereby the package installation fails.

That is actually not true.
Here is the gogs.pre-install

  1 #!/bin/sh
  2 
  3 addgroup -S -g 82 www-data 2>/dev/null
  4 adduser -S -D -h /var/lib/gogs -s /bin/ash -G www-data -g gogs gogs 2>/dev/null
  5 passwd -u gogs 2>/dev/null
  6 
  7 exit 0

If user already exist, the line 4 adduser will fail, however script
will continue and also do the passwd on line 5.

Line 7 makes sure that script always exists with success, and the
pre-install will never result in a package installation failure.

I believe this should be fixed to:

diff --git a/community/gogs/gogs.pre-install b/community/gogs/gogs.pre-install
index ea77703d1e..80d9ac0763 100644
--- a/community/gogs/gogs.pre-install
+++ b/community/gogs/gogs.pre-install
@@ -1,7 +1,7 @@
 #!/bin/sh
 
 addgroup -S -g 82 www-data 2>/dev/null
-adduser -S -D -h /var/lib/gogs -s /bin/ash -G www-data -g gogs gogs 2>/dev/null
-passwd -u gogs 2>/dev/null
+adduser -S -D -h /var/lib/gogs -s /bin/ash -G www-data -g gogs gogs 2>/dev/null \
+       && passwd -u gogs 2>/dev/null
 
 exit 0

That way the user is only unlocked at first install. If admin
afterwards locks the user (and intentionally break gogs'
functionality), it will continue that way on future upgrades.

I was about to ping the gogs package maintainer, but seems like nobody
wants maintain it. I guess I just go ahead and fix this.


-nc