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

client-auth.c: Fix authentication denial for valid logins #342

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

zdohnal
Copy link
Contributor

@zdohnal zdohnal commented Feb 15, 2024

According man pages, getgrouplist() always return non-zero number, so we have to handle only the case when user is in more groups than we have static array for.

This happens when you run PAPPL based printer application with -o auth-service=password-auth -o admin-group=wheel.

@michaelrsweet
Copy link
Owner

From the getgrouplist man page on my Mac:

RETURN VALUES
     The getgrouplist() function returns 0 on success.  If the size of the
     group list is too small to hold all the user's groups, getgrouplist()
     returns -1 to indicate failure.  In this case, the group array will be
     filled with as many groups as will fit.

OpenBSD's implementation seems to match macOS:

RETURN VALUES

The getgrouplist() function returns 0 if successful and -1 if the size of the group list is too small to hold all the user's groups. Here, the group array will be filled with as many groups as will fit.

But glibc's implementation is different:

RETURN VALUE

       If the number of groups of which user is a member is less than or
       equal to *ngroups, then the value *ngroups is returned.

       If the user is a member of more than *ngroups groups, then
       getgrouplist() returns -1.  In this case, the value returned in
       *ngroups can be used to resize the buffer passed to a further
       call to getgrouplist().

The fact that you are seeing issues with 32 groups means we need to increase the size of that array, too.

Copy link
Owner

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

Go ahead and increase the size of the "groups" array to 256 elements.

pappl/client-auth.c Outdated Show resolved Hide resolved
pappl/client-auth.c Outdated Show resolved Hide resolved
pappl/client-auth.c Outdated Show resolved Hide resolved
@zdohnal
Copy link
Contributor Author

zdohnal commented Feb 19, 2024

But glibc's implementation is different:

RETURN VALUE

       If the number of groups of which user is a member is less than or
       equal to *ngroups, then the value *ngroups is returned.

       If the user is a member of more than *ngroups groups, then
       getgrouplist() returns -1.  In this case, the value returned in
       *ngroups can be used to resize the buffer passed to a further
       call to getgrouplist().

The fact that you are seeing issues with 32 groups means we need to increase the size of that array, too.

We can do that too for sure, but the issue was with the condition - the function probably returns 0 only when user would not be in any group (IMHO it is not possible, if the user exists, so it is not mentioned in man page), so it returned non-zero for my user (positive number of groups, since I'm not in more than 32 groups), so the execution got into the if scope and returned failure.

Copy link
Owner

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

See comments. We obviously need to do something here but need to be careful because getgrouplist is not POSIX.

pappl/client-auth.c Outdated Show resolved Hide resolved
pappl/client-auth.c Outdated Show resolved Hide resolved
pappl/client-auth.c Outdated Show resolved Hide resolved
pappl/client-auth.c Outdated Show resolved Hide resolved
pappl/client-auth.c Fixed Show resolved Hide resolved
According man pages, `getgrouplist()` always return non-zero number, so
we have to handle only the case when user is in more groups than we have
static array for.
@michaelrsweet michaelrsweet merged commit 988b90a into michaelrsweet:master Feb 21, 2024
3 of 5 checks passed
@michaelrsweet michaelrsweet self-assigned this Feb 21, 2024
@michaelrsweet michaelrsweet added bug Something isn't working priority-medium platform issue Issue is specific to an OS or desktop labels Feb 21, 2024
@michaelrsweet michaelrsweet added this to the v2.0 milestone Feb 21, 2024
@zdohnal
Copy link
Contributor Author

zdohnal commented Feb 22, 2024

@michaelrsweet the message in Changelog is incorrect - it was not issue about user being in more than 32 groups. Auth failed if user was in any group (the function returned number of groups he's in) with glibc.

$ groups zdohnal
zdohnal : zdohnal wheel libvirt wireshark mock

@michaelrsweet
Copy link
Owner

[v1.4.x cf6c821] Update the changelog for the getgrouplist fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working platform issue Issue is specific to an OS or desktop priority-medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants