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

Implement dropping group privileges #239

Merged
merged 2 commits into from
Apr 24, 2017

Conversation

pieterlexis
Copy link
Contributor

This PR adds support for running sniproxy as a different group than the primary group set with the user configuration option. A group option can now be used in the config-file.

If no group is provided, the user's default group is used.

Copy link
Owner

@dlundquist dlundquist left a comment

Choose a reason for hiding this comment

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

Code looks good, but I'm not sure I see a strong use case. The only case I can think of is when proxying the unix sockets which are restricted to specific groups. Did you have another use case in mind, or was this just for completeness?

src/sniproxy.c Outdated
@@ -222,11 +222,25 @@ drop_perms(const char *username) {
else if (user == NULL)
fatal("getpwnam(): user %s does not exist", username);

gid_t gid = user->pw_gid;

if (groupname) {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we explicitly check groupname != NULL?

@pieterlexis
Copy link
Contributor Author

This is both for completeness and the fact that it complements #224. We have some files that are mode 0540 so sniproxy needs to run as the group writing those files. Having a group command is easier than letting the package create the user+group and then moving the user to a different primary group 😄

@dlundquist
Copy link
Owner

I just noticed the is a memory leak if the configuration contains duplicate username and groupname directives. Running valgrind ./config_test with username and groupname lines duplicated.

==16401== 
==16401== HEAP SUMMARY:
==16401==     in use at exit: 983 bytes in 7 blocks
==16401==   total heap usage: 78 allocs, 71 frees, 14,571 bytes allocated
==16401== 
==16401== 7 bytes in 1 blocks are definitely lost in loss record 1 of 7
==16401==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==16401==    by 0x553F989: strdup (strdup.c:42)
==16401==    by 0x402DEB: accept_username (config.c:327)
==16401==    by 0x40356A: parse_config (cfg_parser.c:67)
==16401==    by 0x4030C6: init_config (config.c:231)
==16401==    by 0x402300: main (config_test.c:11)
==16401== 
==16401== 8 bytes in 1 blocks are definitely lost in loss record 3 of 7
==16401==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==16401==    by 0x553F989: strdup (strdup.c:42)
==16401==    by 0x402DAB: accept_groupname (config.c:338)
==16401==    by 0x40356A: parse_config (cfg_parser.c:67)
==16401==    by 0x4030C6: init_config (config.c:231)
==16401==    by 0x402300: main (config_test.c:11)
==16401== 
==16401== LEAK SUMMARY:
==16401==    definitely lost: 15 bytes in 2 blocks
==16401==    indirectly lost: 0 bytes in 0 blocks
==16401==      possibly lost: 0 bytes in 0 blocks
==16401==    still reachable: 968 bytes in 5 blocks
==16401==         suppressed: 0 bytes in 0 blocks
==16401== Reachable blocks (those to which a pointer was found) are not shown.
==16401== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16401== 
==16401== For counts of detected and suppressed errors, rerun with: -v
==16401== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

I'll add necessary checks to this accept_username and accept_groupname so a duplicate directive is a configuration error rather than leaking memory.

@dlundquist dlundquist merged commit a598db4 into dlundquist:master Apr 24, 2017
@pieterlexis pieterlexis deleted the group-support branch April 25, 2017 07:27
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.

2 participants