pam_setcred in slock not working #34
Comments
|
That should work, it's the exact same sequence of calls that xsecurelock is using, apart from not calling pam_chauthtok if pam_acct_mgmt fails, which is optional anyway. Since slock does unlock (I assume), it should definitely have called pam_setcred. Can you comment out the call to pam_acct_mgmt, just to exclude that as the cause? That's the way i3lock does it. Also, try adding the I don't have my notebook available currently on which I use pam-gnupg myself, so I can't test it directly, unfortunately. |
|
Commenting out the lines Didn't change anything. Enabling the debug option I see It seems like the credentials aren't being stored somewhere by slock? |
|
Ah, slock installs itself as setuid root to be able to read /etc/shadow (precisely because it doesn't use PAM), and then drops privileges to user ‘nobody’. pam-gnupg, on the other hand, tries to become the authenticated user, in order to read the config file ~/.pam-gnupg, and aborts if that fails, which it will since ‘nobody’ is not allowed to become you. The easiest workaround is probably to hardcode your real user as the drop user in Otherwise, you can rip out all of the setuid functionality, which is not really needed with PAM (with one exception, see below):
Note that I didn't test this, you'll have to see what happens. |
|
I do have my username as the drop user, although the thing I'm not comfortable is what should be the drop group? I currently have it set to "users" |
|
It should be your primary group, the one displayed by |
|
Welp, that was it! I had the wrong group set. It now works! Thank you so much for all of your help!! |
Pardon my ignorance, but why is this necessary? If I leave the SUID be, the EDIT: Forgot to mention that it seems to work just fine otherwise with your proposed changes and cleaning the unused variables. |
|
It's been a while, so I don't remember exactly. I think it's simply that running as root is not exactly best practice. |
|
Then how about introducing dropping the privileges back, this time automatically to the real user, making it pretty much as secure as it would be with just using your own user and group for the drop targets? Here's my current diff of the changes in case someone comes looking for it here, or if there are any suggestions to improve it. I might also submit it as a patch to suckless.org a bit later. diff --git a/config.def.h b/config.def.h
index 9855e21..19e7f62 100644
--- a/config.def.h
+++ b/config.def.h
@@ -6,7 +6,11 @@ static const char *colorname[NUMCOLS] = {
[INIT] = "black", /* after initialization */
[INPUT] = "#005577", /* during input */
[FAILED] = "#CC3333", /* wrong password */
+ [PAM] = "#9400D3", /* waiting for PAM */
};
/* treat a cleared input like a wrong password (color) */
static const int failonclear = 1;
+
+/* PAM service that's used for authentication */
+static const char* pam_service = "login";
diff --git a/config.mk b/config.mk
index 514c236..f57bb4e 100644
--- a/config.mk
+++ b/config.mk
@@ -12,10 +12,10 @@ X11LIB = /usr/X11R6/lib
# includes and libs
INCS = -I. -I/usr/include -I${X11INC}
-LIBS = -L/usr/lib -lc -lcrypt -L${X11LIB} -lX11 -lXext -lXrandr
+LIBS = -L/usr/lib -lc -lcrypt -L${X11LIB} -lX11 -lXext -lXrandr -lpam
# flags
-CPPFLAGS = -DVERSION=\"${VERSION}\" -D_DEFAULT_SOURCE -DHAVE_SHADOW_H
+CPPFLAGS = -DVERSION=\"${VERSION}\" -D_DEFAULT_SOURCE
CFLAGS = -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
LDFLAGS = -s ${LIBS}
COMPATSRC = explicit_bzero.c
diff --git a/slock.c b/slock.c
index b2f14e3..f97a1fa 100644
--- a/slock.c
+++ b/slock.c
@@ -1,8 +1,5 @@
/* See LICENSE file for license details. */
#define _XOPEN_SOURCE 500
-#if HAVE_SHADOW_H
-#include <shadow.h>
-#endif
#include <ctype.h>
#include <errno.h>
@@ -18,16 +15,22 @@
#include <X11/keysym.h>
#include <X11/Xlib.h>
#include <X11/Xutil.h>
+#include <security/pam_appl.h>
+#include <security/pam_misc.h>
#include "arg.h"
#include "util.h"
char *argv0;
+static int pam_conv(int num_msg, const struct pam_message **msg, struct pam_response **resp, void *appdata_ptr);
+struct pam_conv pamc = {pam_conv, NULL};
+char passwd[256];
enum {
INIT,
INPUT,
FAILED,
+ PAM,
NUMCOLS
};
@@ -57,6 +60,31 @@ die(const char *errstr, ...)
exit(1);
}
+static int
+pam_conv(int num_msg, const struct pam_message **msg,
+ struct pam_response **resp, void *appdata_ptr)
+{
+ int retval = PAM_CONV_ERR;
+ for(int i=0; i<num_msg; i++) {
+ if (msg[i]->msg_style == PAM_PROMPT_ECHO_OFF &&
+ strncmp(msg[i]->msg, "Password: ", 10) == 0) {
+ struct pam_response *resp_msg = malloc(sizeof(struct pam_response));
+ if (!resp_msg)
+ die("malloc failed\n");
+ char *password = malloc(strlen(passwd) + 1);
+ if (!password)
+ die("malloc failed\n");
+ memset(password, 0, strlen(passwd) + 1);
+ strcpy(password, passwd);
+ resp_msg->resp_retcode = 0;
+ resp_msg->resp = password;
+ resp[i] = resp_msg;
+ retval = PAM_SUCCESS;
+ }
+ }
+ return retval;
+}
+
#ifdef __linux__
#include <fcntl.h>
#include <linux/oom.h>
@@ -83,10 +111,9 @@ dontkillme(void)
}
#endif
-static const char *
-gethash(void)
+static struct passwd *
+getpw(void)
{
- const char *hash;
struct passwd *pw;
/* Check if the current user has a password entry */
@@ -97,43 +124,20 @@ gethash(void)
else
die("slock: cannot retrieve password entry\n");
}
- hash = pw->pw_passwd;
-
-#if HAVE_SHADOW_H
- if (!strcmp(hash, "x")) {
- struct spwd *sp;
- if (!(sp = getspnam(pw->pw_name)))
- die("slock: getspnam: cannot retrieve shadow entry. "
- "Make sure to suid or sgid slock.\n");
- hash = sp->sp_pwdp;
- }
-#else
- if (!strcmp(hash, "*")) {
-#ifdef __OpenBSD__
- if (!(pw = getpwuid_shadow(getuid())))
- die("slock: getpwnam_shadow: cannot retrieve shadow entry. "
- "Make sure to suid or sgid slock.\n");
- hash = pw->pw_passwd;
-#else
- die("slock: getpwuid: cannot retrieve shadow entry. "
- "Make sure to suid or sgid slock.\n");
-#endif /* __OpenBSD__ */
- }
-#endif /* HAVE_SHADOW_H */
-
- return hash;
+ return pw;
}
static void
readpw(Display *dpy, struct xrandr *rr, struct lock **locks, int nscreens,
- const char *hash)
+ const char *pw)
{
XRRScreenChangeNotifyEvent *rre;
- char buf[32], passwd[256], *inputhash;
- int num, screen, running, failure, oldc;
+ char buf[32];
+ int num, screen, running, failure, oldc, retval;
unsigned int len, color;
KeySym ksym;
XEvent ev;
+ pam_handle_t *pamh;
len = 0;
running = 1;
@@ -160,10 +164,27 @@ readpw(Display *dpy, struct xrandr *rr, struct lock **locks, int nscreens,
case XK_Return:
passwd[len] = '\0';
errno = 0;
- if (!(inputhash = crypt(passwd, hash)))
- fprintf(stderr, "slock: crypt: %s\n", strerror(errno));
- else
- running = !!strcmp(inputhash, hash);
+ retval = pam_start(pam_service, pw, &pamc, &pamh);
+ color = PAM;
+ for (screen = 0; screen < nscreens; screen++) {
+ XSetWindowBackground(dpy, locks[screen]->win, locks[screen]->colors[color]);
+ XClearWindow(dpy, locks[screen]->win);
+ XRaiseWindow(dpy, locks[screen]->win);
+ }
+ XSync(dpy, False);
+
+ if (retval == PAM_SUCCESS)
+ retval = pam_authenticate(pamh, 0);
+ if (retval == PAM_SUCCESS)
+ retval = pam_acct_mgmt(pamh, 0);
+
+ running = 1;
+ if (retval == PAM_SUCCESS) {
+ pam_setcred(pamh, PAM_REFRESH_CRED);
+ running = 0;
+ } else
+ fprintf(stderr, "slock: %s\n", pam_strerror(pamh, retval));
+ pam_end(pamh, retval);
if (running) {
XBell(dpy, 100);
failure = 1;
@@ -307,11 +328,7 @@ int
main(int argc, char **argv) {
struct xrandr rr;
struct lock **locks;
- struct passwd *pwd;
- struct group *grp;
- uid_t duid;
- gid_t dgid;
- const char *hash;
+ struct passwd *pw;
Display *dpy;
int s, nlocks, nscreens;
@@ -323,26 +340,12 @@ main(int argc, char **argv) {
usage();
} ARGEND
- /* validate drop-user and -group */
- errno = 0;
- if (!(pwd = getpwnam(user)))
- die("slock: getpwnam %s: %s\n", user,
- errno ? strerror(errno) : "user entry not found");
- duid = pwd->pw_uid;
- errno = 0;
- if (!(grp = getgrnam(group)))
- die("slock: getgrnam %s: %s\n", group,
- errno ? strerror(errno) : "group entry not found");
- dgid = grp->gr_gid;
-
#ifdef __linux__
dontkillme();
#endif
- hash = gethash();
+ pw = getpw();
errno = 0;
- if (!crypt("", hash))
- die("slock: crypt: %s\n", strerror(errno));
if (!(dpy = XOpenDisplay(NULL)))
die("slock: cannot open display\n");
@@ -350,9 +353,9 @@ main(int argc, char **argv) {
/* drop privileges */
if (setgroups(0, NULL) < 0)
die("slock: setgroups: %s\n", strerror(errno));
- if (setgid(dgid) < 0)
+ if (setgid(pw->pw_gid) < 0)
die("slock: setgid: %s\n", strerror(errno));
- if (setuid(duid) < 0)
+ if (setuid(pw->pw_uid) < 0)
die("slock: setuid: %s\n", strerror(errno));
/* check for Xrandr support */
@@ -389,7 +392,7 @@ main(int argc, char **argv) {
}
/* everything is now blank. Wait for the correct password */
- readpw(dpy, &rr, locks, nscreens, hash);
+ readpw(dpy, &rr, locks, nscreens, pw->pw_name);
return 0;
} |
Looks like I spoke a little too early...
I'm trying to use slock with the pam-auth patch . The patch itself doesnt call pam_setcred, so naturally as with any suckless tool, I'm trying to implement it myself. Judging from the physlock and xsecurelock PRs, it looks like it should be a simple addition. So, I thought this would work:
But it doesn't seem to be the case. Is there something else that needs to be called before
pam_setcred?Originally posted by @Barbarossa93 in #7 (comment)
The text was updated successfully, but these errors were encountered: