[PATCH ubox] logd: fix priviledge dropping order

[Giovanni Giacobbi]

On 27/07/2021 14:39, Giovanni Giacobbi wrote:

> Fixes: 41664054b8b1 ("logd: fix ignored return values in set{gid,uid}")
> Signed-off-by: Giovanni Giacobbi <giovanni at giacobbi.net>
> ---
>   log/logd.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Let me add some remarks about this change. Comments on each point are 
welcome.

1) The way it is now, the daemon is completely broken as it cannot 
setgid(2) after setuid(2) to a non-zero uid. This is the minimal fix to 
this situation. The bug exists already in 21.02 and master but the 
setgid is failing silently (so the egid remains zero).

2) I'm not sure that exit() is the right response in case of failure. 
Given the role of this daemon I would consider letting it run as root to 
avoid ending up without a syslog.

3) Currently the privilege de-escalation is activated by the sole 
presence of the "logd" user. If it doesn't exist, it silently keeps 
running as root, while if the user is there but setuid/setgid fails, it 
exits. I personally don't like this asymmetric behaviour, i'd rather 
have it print a warning (and maybe self-log it to itself as log 
critical?) and continue running as root. As an alternative, I would 
propose to add a command line switch to indicate the user to switch to, 
and then fail if user not found or setgid/setuid fail, so that you know 
for sure once you specify the command line switch that either privilege 
is dropped, or it doesn't start. Not using the command line switch would 
NOT attempt to switch user even if "logd" exists. Ideas? Should I submit 
this?


P.S. for the maintainer: If you commit this patch, please fix the typo 
in the subject, thx. "logd: fix privilege dropping order"