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

Call initgroups before setuid #148

Merged
merged 2 commits into from Jul 21, 2019
Merged

Call initgroups before setuid #148

merged 2 commits into from Jul 21, 2019

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Jul 1, 2019

This fixes a potential privilege escalation issue (or, more precisely, privileges not being dropped when this was the user's intent) where the groups of the spawning process's user would be incorrectly retained due to a missing call to initgroups.

This also fixes an incorrect case fallthrough in the error message lookup logic.

Fixes #149.

@bgamari
Copy link
Contributor Author

bgamari commented Jul 1, 2019

This was originally reported at ndmitchell/ghcid#261.

@snoyberg
Copy link
Collaborator

snoyberg commented Jul 4, 2019

I'm surprised that appveyor is failing on this. I'll try rebasing on master and see if that results in something else.

Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments. I have to admit with not being familiar with this part of POSIX, but overall it seems to make sense.

include/runProcess.h Show resolved Hide resolved
cbits/runProcess.c Outdated Show resolved Hide resolved
@juhp
Copy link

juhp commented Jul 5, 2019

I have tested Ben's patch with ghc-8.6.5, process:initgroups (8c3686a), and cabal-install-2.2 and can confirm that with the patch, the ghcid-0.7.5 executable has initgroups. Thanks @bgamari for this.

[f31container]$ readelf -a ~/.cabal/bin/ghcid|grep initgroups
0000008ba160  005700000007 R_X86_64_JUMP_SLO 0000000000000000 initgroups@GLIBC_2.2.5 + 0
    87: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND initgroups@GLIBC_2.2.5 (4)

The error message lookup logic would fallthrough from the
forkSetuidFailed case into the default case, meaning that the error
message of the former would never be returned.
@bgamari bgamari force-pushed the initgroups branch 2 times, most recently from d70c604 to 3e0812f Compare July 18, 2019 16:01
@juhp
Copy link

juhp commented Jul 19, 2019

Seems to build okay on Windows now 👍
(Not quite sure why lts-11 keeps failing but seems unrelated?)

Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing: can you add a ChangeLog entry explaining the change?

Previously we would fail to call initgroups before setuid'ing. This
meant that our groups we not be reset to reflect those our new user
belongs to. Fix this.
@bgamari
Copy link
Contributor Author

bgamari commented Jul 19, 2019

@snoyberg, done.

@snoyberg snoyberg merged commit 52bc96a into haskell:master Jul 21, 2019
liuyangxy pushed a commit to fedora-riscv/ghc that referenced this pull request Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process fails to call initgroups before setuid
3 participants