illiliti / ssu

Extremely simple su utility
GNU General Public License v3.0
71 stars 3 forks source link

sls: add support for using system groups #3

Closed cemkeylan closed 3 years ago

cemkeylan commented 3 years ago

Hey illiliti,

I don't know what your opinion would be on adding "system group" support to sls, but I find this functionality quite useful myself. This adds a new function checkgroups() which basically verifies whether the calling user is part of the owning group.

This way

illiliti commented 3 years ago

I like this but sadly this doesn't work reliable because POSIX permits(doesn't apply to Linux and BSD's, not sure about others) getgroups to include EGID into array. This means that anyone can run commands as root :(

https://pubs.opengroup.org/onlinepubs/9699919799/functions/getgroups.html

It is implementation-defined whether getgroups() also returns the effective group ID in the grouplist array. ... As implied by the definition of supplementary groups, the effective group ID may appear in the array returned by getgroups() or it may be returned only by getegid()

The only proper way to implement this is using getgrouplist but i rather stay away from non-portable extensions as much as possible(initgroups is exception due to lacking a portable way to set supplementary groups)

cemkeylan commented 3 years ago

Oh, I see. The simplest solution would be setting the authorized group in a config.h header and just install it without setgid, that way it would be configured at compile-time. What do you think of that?

EDIT: Also EGID would be the user's own GID, so we also wouldn't have to worry about that either.

cemkeylan commented 3 years ago

I have implemented it, but reflecting on it, this completely changes the behaviour of the software into something different.

It no longer handles authentication based on file metadata (which is the biggest point of sls), and it requires manually editing the header for group configuration. I have opened a new pull request for comparison.

illiliti commented 3 years ago

Well, I came up to the another idea. Why we need this redundant 'rgid == egid' checking if we can just drop read+execute permission from others. Kernel just simply prevents anyone to execute binary who is not in owner/group. Maybe I'm missing something, what do you think ?

From my perspective, this is genius idea XD

cemkeylan commented 3 years ago

That really does make sense. It is so simple of a solution that I feel that there should be a catch. :D

illiliti commented 3 years ago

Pushed commit. Thank you for the initial idea!