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.

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

fix the setusercontext(3) workaround #23

Merged
merged 1 commit into from Sep 3, 2019
Merged

Conversation

@Duncaen
Copy link
Contributor

Duncaen commented Sep 2, 2019

Seeing this being used on even more system like Illumos with this ugly
and security critical bug open makes me cringe every time I check if it
was finally fixed.

I reported it directly to the maintainer in 2017. I reported it to
pkgsrc-security@netbsd.org without a response.

Seeing this being used on even more system like Illumos with this ugly
and security critical bug open makes me cringe every time I check if it
was finally fixed.

I reported it directly to the maintainer in 2017. I reported it to
pkgsrc-security@netbsd.org without a response.
@Duncaen Duncaen changed the title fix the stupid setusercontext(3) workaround fix the setusercontext(3) workaround Sep 3, 2019
@slicer69

This comment has been minimized.

Copy link
Owner

slicer69 commented Sep 3, 2019

Thanks for submitting the patch. Would you care to explain what it is you are trying to fix and why you feel it is a security issue? How is this an improvement over existing behaviour?

@Duncaen

This comment has been minimized.

Copy link
Contributor Author

Duncaen commented Sep 3, 2019

doas uses setusercontext(3) with the LOGIN_SETUSER flag which does setresuid(3) and the LOGIN_SETGROUP flag which does initgroups(3) and setresgid(2).
The codepath that is used when HAVE_LOGIN_CAP_H is not set, only does setuid(2) which only sets the uid, the LOGIN_SETGROUP path is completely left out.

This means your doas port does not drop the groups of the executing user.

  1. This is a big problem if you use doas to run a command with less privileges, which is very security critical, this is not the intended behavior as it is on openbsd, this drastically changes what doas actually does in those cases. (Last time I used a crontab entry to switch from root to a different user as example for such a use case.)
  2. If you run a script or program which creates files or directory user and group editable, it will use the executing users group id, which could lead to unintended user writable files.
  3. When you have a umask set that would make it group writable, its not as expected root:root, its root:user.

The whole point I make is that this is wrong. Both openbsd doas and sudo change the group ids and making this port differ from this behavior without documenting and warning users is very very bad.

@slicer69 slicer69 merged commit 79c6c61 into slicer69:master Sep 3, 2019
@slicer69

This comment has been minimized.

Copy link
Owner

slicer69 commented Sep 3, 2019

I have a point of contention with the overall view. One being that I'm not sure this qualifies as a security concern. Having the group of the calling user is arguably expected, or at least certainly useful. However, I can see why, in some cases it would also be dangerous. I'm going to commit this patch and test it. Thank you for putting it together.

On a side note, I'd like to point out that my testing shows this issue, keeping the group of the original user, appears to only affect Linux. On FreeBSD group permissions were already dropped, as they were on OpenBSD, and (I think) NetBSD. Linux was the odd one out for keeping the calling user's groups. And I think this patch is worthwhile to bring Linux into the fold so it is no longer an exception.

Repository owner deleted a comment from Duncaen Sep 3, 2019
Repository owner deleted a comment from Duncaen Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.