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.

Sign up for GitHub

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 IsClientAuthorized() for FreeBSD and stop failing open #209

Closed
wants to merge 2 commits into from
Closed

Implement IsClientAuthorized() for FreeBSD and stop failing open #209

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad94750
auth.c: fail the build if we don't have an IsClientAuthorized()
kevans91 Sep 5, 2024
fa4225c
auth.c: implement polkit support for FreeBSD
kevans91 Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions src/auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,32 @@

#include <errno.h>

#if defined(HAVE_POLKIT) && defined(SO_PEERCRED)
#if defined(HAVE_POLKIT) && (defined(SO_PEERCRED) || defined(LOCAL_PEERCRED))

#include <polkit/polkit.h>
#include <stdbool.h>

#ifdef __FreeBSD__
Copy link

Choose a reason for hiding this comment

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

I don't know much about socket authentication, but what about macOS? Is the struct xucred only used on FreeBSD? A quick Google search implies that it's also used on Mac. Would you mind explaining what ucred vs xucred is about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xucred is the ABI-stable userland export version (subset) of ucred -- if you have the former then the latter is considered kernel-only and it will populate an xucred for things in userland that want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macOS also seems to have LOCAL_PEERCRED, but I don't know what the polkit-on-macOS situation is like so I haven't bothered trying to expand it in that direction. Likely it also wants the xucred version of this.


#include <sys/ucred.h>
typedef struct xucred platform_cred;
#define CRED_PID(uc) (uc).cr_pid
#define CRED_UID(uc) (uc).cr_uid

#else

typedef struct ucred platform_cred;
#define CRED_PID(uc) (uc).pid
#define CRED_UID(uc) (uc).uid

#endif

extern bool disable_polkit;

/* Returns non zero when the client is authorized */
unsigned IsClientAuthorized(int socket, const char* action, const char* reader)
{
struct ucred cr;
platform_cred cr;
socklen_t cr_len;
int ret;
PolkitSubject *subject;
Expand All @@ -77,7 +92,11 @@ unsigned IsClientAuthorized(int socket, const char* action, const char* reader)
snprintf(action_name, sizeof(action_name), "org.debian.pcsc-lite.%s", action);

cr_len = sizeof(cr);
#ifdef LOCAL_PEERCRED
ret = getsockopt(socket, SOL_LOCAL, LOCAL_PEERCRED, &cr, &cr_len);
#else
ret = getsockopt(socket, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len);
#endif
if (ret == -1)
{
#ifndef NO_LOG
Expand All @@ -97,7 +116,7 @@ unsigned IsClientAuthorized(int socket, const char* action, const char* reader)
return 0;
}

subject = polkit_unix_process_new_for_owner(cr.pid, 0, cr.uid);
subject = polkit_unix_process_new_for_owner(CRED_PID(cr), 0, CRED_UID(cr));
if (subject == NULL)
{
Log1(PCSC_LOG_CRITICAL, "polkit_unix_process_new_for_owner failed");
Expand Down Expand Up @@ -144,7 +163,7 @@ unsigned IsClientAuthorized(int socket, const char* action, const char* reader)
{
Log4(PCSC_LOG_CRITICAL,
"Process %u (user: %u) is NOT authorized for action: %s",
(unsigned)cr.pid, (unsigned)cr.uid, action);
(unsigned)CRED_PID(cr), (unsigned)CRED_UID(cr), action);
}

if (result)
Expand All @@ -159,6 +178,10 @@ unsigned IsClientAuthorized(int socket, const char* action, const char* reader)
return ret;
}

#elif defined(HAVE_POLKIT)

#error polkit is enabled, but no socket cred implementation for this platform

#else

unsigned IsClientAuthorized(int socket, const char* action, const char* reader)
Copy link

Choose a reason for hiding this comment

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

Given the above, #error polkit is enabled, but no socket cred implementation for this platform, I'm not sure if this will matter or not. But I wonder if this function still has an effect, if we can log some kind of message to stderr stating that the allow-all IsClientAuthorized() function is being used.

May be outside the scope of this PR, but wanted to suggest it as a way to make it more obvious if others run into this.

Copy link
Contributor Author

@kevans91 kevans91 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The allow-all one is what you should get when you build with -Dpolkit=false to avoid sprinkling more HAVE_POLKIT around and allow other authorization mechanisms to be plugged in instead. Its use is actually expected rather than an error in the default configuration, and that seems more or less fine.

Copy link

Choose a reason for hiding this comment

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

Ah, gotcha. Thank you for explaining!

Expand Down