In article <200612200913.KAA0000122500@zel459.zel.kfa-juelich.de>,
Matthias Drochner  <M.Drochner@fz-juelich.de> wrote:
>-=-=-=-=-=-
>
>
>[elad asked me to post this to tech-security. The other
>mail referred to below (about a suid helper at nss level)
>can be found in tech-userlevel.]
>
>
>[this is more or less a followup to my
>"suid helper to read own passwd entry" mail earlier this month.]
>
>Here is an implementation which plugs in as PAM module.
>The advantage is that, opposed to the nss variant, one can't
>get the encrypted password (not even as its owner) to mount
>an offline dictionary attack.
>The backside is that if one succeeds to eavesdrop the
>communication between the (unprivileged) client program
>and the SUID helper, he gets the plaintext password and
>no strong passwd encryption will help.
>One can argue (as does Joerg) that such an attacker could
>listen to X11 events carrying the passwd as well, so there
>is no additional danger.
>
>So please have a look at it and tell whether there are
>security risks besides the obvious and known ones, and
>what else can be improved.
>I'd consider that stuff for addition to pkgsrc next
>quarter, and to make the screensavers use it.
>
>best regards
>Matthias
>
>
>-=-=-=-=-=-
>
>#include <sys/types.h>
>#include <security/pam_appl.h>
>#include <security/pam_modules.h>
>
>#include <unistd.h>
>#include <string.h>
>#include <sys/wait.h>
>#include <errno.h>
>
>#define PATH_HELPER "/home/drochner/pam_passwd_suid/helper/helper"
>
>static int
>askhelper(const char *user, const char *pass)
>{
>	int fd[2];
>	pid_t pid, rpid;
>	int res, pwlen, s;

pwlen should be size_t, res should be ssize_t.
>
>	res = pipe(fd);
>	if (res < 0)
>		return (errno);
Don't use res;
	if (pipe(fd) == -1)
		return errno;

KNF here; errno, not (errno) and everywhere else.
>
>	pid = fork();

This is a good candidate for vfork.

>	if (pid == -1)
>		return (errno);

I use a switch for *fork() to avoid the 2 ifs.

>	if (pid == 0) {
>		dup2(fd[0], 0);
>		close(fd[0]);
>		close(fd[1]);

(void) before close and dup2
>		execl(PATH_HELPER, PATH_HELPER, user, NULL);
>		_exit(errno);
>	}
>
>	close(fd[0]);
>
>	pwlen = strlen(pass);
>	res = write(fd[1], pass, pwlen);
>	if (res != pwlen)
>		return (EIO);

This hides the true errno.
	if (res != pwlen)
		return res == -1 ? errno : EIO;
	
>
>	close(fd[1]);
>
>	rpid = waitpid(pid, &s, 0);
>	if (rpid != pid)
>		return (errno);
>	if (WEXITSTATUS(s))
>		return (WEXITSTATUS(s));

Should return another errno here, not the exit status.
>
>	return (0);
>}
>
>PAM_EXTERN int
>pam_sm_authenticate(pam_handle_t *pamh, int flags,
>		    int argc, const char ** argv)
KNF
>{
>	const char *user, *pass;
>	int res;
>
>	res = pam_get_user(pamh, &user, NULL);
>	if (res != PAM_SUCCESS)
>		return (res);
>	res = pam_get_authtok(pamh, PAM_AUTHTOK, &pass, NULL);
>	if (res != PAM_SUCCESS)
>		return (res);
>
>	if (askhelper(user, pass) != 0)
>		return (PAM_AUTH_ERR);
>
>	return (PAM_SUCCESS);
>}
>
>PAM_MODULE_ENTRY("pam_passwdhelper");
>
>-=-=-=-=-=-
>
>#include <pwd.h>
>#include <string.h>
>#include <errno.h>
>#include <unistd.h>
>
>static char pwbuf[1024];
>
>int
>main(int argc, char **argv)
>{
>	struct passwd *pwent;
>	int res;
res should be ssize_t
>	char *bufptr;
>	size_t buflen;
>
>	if (argc != 2)
>		return (EINVAL);
>
>	bufptr = pwbuf;
>	buflen = sizeof(pwbuf);
>	do {
>		res = read(0, bufptr, buflen);
>		if (res < 0)
>			return (errno);
>		bufptr += res;
>		buflen -= res;
>	} while (res > 0 && buflen > 0);
>	if (buflen == 0)
>		return (ENOMEM);
>
>	pwent = getpwnam(argv[1]);
>	if (!pwent || (pwent->pw_uid != getuid()))
>		return (EPERM);
>
>	if (strcmp(crypt(pwbuf, pwent->pw_passwd), pwent->pw_passwd) == 0)
>		return (0);
>
>	return (EAUTH);
>}
>
>-=-=-=-=-=-

Otherwise it looks ok.

christos