cifs-utils: pam module to set cifs credentials in key store

Commit Message

Jeff Layton Dec. 10, 2013, 12:21 p.m. UTC

On Sat, 7 Dec 2013 09:23:41 -0500
Jeff Layton <[e-mail]> wrote:

> On Mon, 02 Dec 2013 12:36:38 -0700
> Orion Poplawski <[e-mail]> wrote:
> 
> > On 11/30/2013 03:32 AM, Jeff Layton wrote:
> > > On Wed, 13 Nov 2013 13:59:33 -0700
> > > Orion Poplawski <[e-mail]> wrote:
> > >
> > >>  From e2e6e73f05d912377656675292994858b99cabbb Mon Sep 17 00:00:00 2001
> > >> From: Orion Poplawski <[e-mail]>
> > >> Date: Wed, 13 Nov 2013 13:53:30 -0700
> > >> Subject: [PATCH] Create cifscreds PAM module to insert credentials at login
> > >>
> > >
> > > Sorry for taking so long to review this. This looks like a good start.
> > > FWIW, I'd like to see this merged for the next release, which should
> > > happen in the next month or two.
> > >
> > 
> > No problem, thanks for the review.
> > 
> > > When you think this is ready for merge, please resend with a real
> > > '[PATCH]' tag in the subject, so I don't miss it...
> > 
> > Hopefully this is okay.
> > 
> > >> @@ -231,6 +236,7 @@ AM_CONDITIONAL(CONFIG_CIFSUPCALL, [test "$enable_cifsupcall" != "no"])
> > >>   AM_CONDITIONAL(CONFIG_CIFSCREDS, [test "$enable_cifscreds" != "no"])
> > >>   AM_CONDITIONAL(CONFIG_CIFSIDMAP, [test "$enable_cifsidmap" != "no"])
> > >>   AM_CONDITIONAL(CONFIG_CIFSACL, [test "$enable_cifsacl" != "no"])
> > >> +AM_CONDITIONAL(CONFIG_PAM, [test "$enable_pam" != "no" -o "$enable_pam" != "no"])
> > >
> > > Erm...you're testing the same thing twice here?
> > 
> > Oops, copy/paste error.
> > 
> > >
> > > I think the autoconf stuff needs some work.
> > >
> > > If the box doesn't have what's needed to build it, the build is going
> > > to fail unless someone explicitly disables CONFIG_PAM.
> > >
> > > Ideally, we'd build this by default and have autoconf test for what's
> > > required to build the PAM module. If the build requires aren't present,
> > > then the building of the PAM module should be disabled with a warning
> > > message that states that.
> > >
> > > There are some other examples of that sort of behavior in configure.ac.
> > > It's a little more complicated, but it makes life easier for people
> > > building the package.
> > 
> > Okay, I've added a section for checking for security/pam_appl.h.  Should we 
> > also check for the other headers/libraries as well?
> > 
> > I've also added to the keyutils.h section so as to disable the pam module 
> > there too.
> > 
> > 
> > >> +static void
> > >> +free_password (char *password)
> > >> +{
> > >> +	volatile char *vp;
> > >> +	size_t len;
> > >> +
> > >> +	if (!password) {
> > >> +		return;
> > >> +	}
> > >> +
> > >> +	/* Defeats some optimizations */
> > >> +	len = strlen (password);
> > >> +	memset (password, 0xAA, len);
> > >> +	memset (password, 0xBB, len);
> > >> +
> > >> +	/* Defeats others */
> > >> +	vp = (volatile char*)password;
> > >> +	while (*vp) {
> > >> +		*(vp++) = 0xAA;
> > >> +	}
> > >> +
> > >
> > > I'm all for scrubbing the pw memory but that seems a bit like overkill.
> > > Got any references to cite about why you need to write over it 3 times?
> > >
> > > If that's really necessary, then we should move the password handling
> > > in cifscreds and mount.cifs binary to use something like this too.
> > > Maybe consider putting this in an a.out lib and linking it into all 3?
> > 
> > This is copied directly from the gnome-keyring pam module.  I don't have any 
> > references for why it is done.  My guess is because it is part of a PAM chain, 
> > so perhaps to prevent later modules from accessing it?  Or perhaps just to 
> > scrub memory?  I suspect that the multiple times is to defeat compiler 
> > optimization making it a no-op?
> > 
> > >> +
> > >> +/**
> > >> + * This is called when the PAM session is closed.
> > >> + *
> > >> + * Currently it does nothing.  The session closing should remove the passwords
> > >> + *
> > >> + * @param ph PAM handle
> > >> + * @param flags currently unused, TODO: check for silent flag
> > >> + * @param argc number of arguments for this PAM module
> > >> + * @param argv array of arguments for this PAM module
> > >> + * @return PAM_SUCCESS
> > >> + */
> > >> +PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
> > >> +{
> > >> +	return PAM_SUCCESS;
> > >> +}
> > >> +
> > >
> > > Cleaning up after you're done seems like a good thing to do. The thing
> > > we need to be careful about here is that we might still need these
> > > creds to write out dirty data. This may require some consideration...
> > 
> > Yeah, I'm not sure what there is to do here really.  Anything would be more 
> > than what is done currently running cifscreds directly.
> > 
> > >> +/**
> > >> + * This is called when PAM is invoked to change the user's authentication token.
> > >> + * TODO - update the credetials
> > >> + *
> > >> + * @param ph PAM handle
> > >> + * @param flags currently unused, TODO: check for silent flag
> > >> + * @param argc number of arguments for this PAM module
> > >> + * @param argv array of arguments for this PAM module
> > >> + * @return PAM_SUCCESS
> > >> + */
> > >> + PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
> > >> +{
> > >> +	return PAM_SUCCESS;
> > >> +}
> > >
> > > The rest looks reasonably straightforward, but I don't PAM that well,
> > > so it's hard for me to comment.
> > >
> > 
> > Actually, I'm not sure what should be done here.  gnome-keyring doesn't do 
> > anything.  http://pubs.opengroup.org/onlinepubs/008329799/pam_sm_setcred.htm 
> > describes the call.
> > 
> > Looks like the pam_sm_chauthtok function is used to update the passwords. 
> > I've made attempts to implement that.
> > 
> 
> Oh, and one other thing...
> 
> Can you resend the above patch with a Signed-off-by line?
> 

Ok, I tested the pam_module this morning and it seems to work as
expected. At this point, I think we can go ahead and merge it once we
have a manpage.

I also intend to add this patch on top, which just does a little
cleanup and fixes some build warnings.

Patch

From d12443fdd268e547412683d43dc03f266260f7c8 Mon Sep 17 00:00:00 2001
From: Jeff Layton <[e-mail]>
Date: Sat, 7 Dec 2013 06:52:26 -0500
Subject: [cifs-utils PATCH] cifscreds: fix up some whitespace, typos and build
 warnings in pam_cifscreds.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

gcc -g -O2 -Wall -Wextra -D_FORTIFY_SOURCE=2 -fpie -pie -Wl,-z,relro,-z,now  -shared -fpic -o pam_cifscreds.so pam_cifscreds.c cifskey.c resolve_host.c util.c -lpam -lkeyutils
pam_cifscreds.c: In function ‘cleanup_free_password’:
pam_cifscreds.c:143:38: warning: unused parameter ‘ph’ [-Wunused-parameter]
 cleanup_free_password (pam_handle_t *ph, void *data, int pam_end_status)
                                      ^
pam_cifscreds.c:143:58: warning: unused parameter ‘pam_end_status’ [-Wunused-parameter]
 cleanup_free_password (pam_handle_t *ph, void *data, int pam_end_status)
                                                          ^
pam_cifscreds.c: In function ‘cifscreds_pam_update’:
pam_cifscreds.c:271:8: warning: variable ‘addrs’ set but not used [-Wunused-but-set-variable]
  char *addrs[16];
        ^
pam_cifscreds.c: In function ‘pam_sm_authenticate’:
pam_cifscreds.c:359:58: warning: unused parameter ‘unused’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_authenticate(pam_handle_t *ph, int unused, int argc, const char **argv)
                                                          ^
pam_cifscreds.c: In function ‘pam_sm_open_session’:
pam_cifscreds.c:414:58: warning: unused parameter ‘flags’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                          ^
pam_cifscreds.c: In function ‘pam_sm_close_session’:
pam_cifscreds.c:487:51: warning: unused parameter ‘ph’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                   ^
pam_cifscreds.c:487:59: warning: unused parameter ‘flags’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                           ^
pam_cifscreds.c:487:70: warning: unused parameter ‘argc’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                                      ^
pam_cifscreds.c:487:89: warning: unused parameter ‘argv’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                                                         ^
pam_cifscreds.c: In function ‘pam_sm_setcred’:
pam_cifscreds.c:501:45: warning: unused parameter ‘ph’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
                                             ^
pam_cifscreds.c:501:53: warning: unused parameter ‘flags’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                     ^
pam_cifscreds.c:501:64: warning: unused parameter ‘argc’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                                ^
pam_cifscreds.c:501:83: warning: unused parameter ‘argv’ [-Wunused-parameter]
 PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
                                                                                   ^

Signed-off-by: Jeff Layton <[e-mail]>
---
 pam_cifscreds.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/pam_cifscreds.c b/pam_cifscreds.c
index 1385146..e0d8a55 100644
--- a/pam_cifscreds.c
+++ b/pam_cifscreds.c
@@ -140,7 +140,8 @@  free_password (char *password)
 }
 
 static void
-cleanup_free_password (pam_handle_t *ph, void *data, int pam_end_status)
+cleanup_free_password (pam_handle_t *ph __attribute__((unused)), void *data,
+			int pam_end_status __attribute__((unused)))
 {
 	free_password (data);
 }
@@ -268,7 +269,6 @@  static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
 	int ret = PAM_SUCCESS;
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress;
-	char *addrs[16];
 	int id, count = 0;
 	char keytype = ((args & ARG_DOMAIN) == ARG_DOMAIN) ? 'd' : 'a';
 
@@ -308,10 +308,8 @@  static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress, keytype) > 0) {
-			addrs[count] = currentaddress;
+		if (key_search(currentaddress, keytype) > 0)
 			count++;
-		}
 
 		currentaddress = nextaddress;
 		if (currentaddress) {
@@ -322,7 +320,7 @@  static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
 	}
 
 	if (!count) {
-		pam_syslog(ph, LOG_ERR, "You have no same stached credentials for %s", hostdomain);
+		pam_syslog(ph, LOG_ERR, "You have no same stashed credentials for %s", hostdomain);
 		return PAM_SERVICE_ERR;
 	}
 
@@ -344,7 +342,7 @@  static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
  * scenarios are possible:
  *
  * - A session is already available which usually means that the user is already
- *	logged on and PAM has been used inside the screensaver. In that case, no need to 
+ *	logged on and PAM has been used inside the screensaver. In that case, no need to
  *	do anything(?).
  *
  * - A session is not yet available. Store the password inside PAM data so
@@ -356,7 +354,7 @@  static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const char *
  * @param argv array of arguments for this PAM module
  * @return any of the PAM return values
  */
-PAM_EXTERN int pam_sm_authenticate(pam_handle_t *ph, int unused, int argc, const char **argv)
+PAM_EXTERN int pam_sm_authenticate(pam_handle_t *ph, int unused __attribute__((unused)), int argc, const char **argv)
 {
 	const char *hostdomain;
 	const char *user;
@@ -365,7 +363,7 @@  PAM_EXTERN int pam_sm_authenticate(pam_handle_t *ph, int unused, int argc, const
 	int ret;
 
 	args = parse_args(ph, argc, argv, &hostdomain);
-	
+
 	/* Figure out and/or prompt for the user name */
 	ret = pam_get_user(ph, &user, NULL);
 	if (ret != PAM_SUCCESS || !user) {
@@ -411,7 +409,7 @@  PAM_EXTERN int pam_sm_authenticate(pam_handle_t *ph, int unused, int argc, const
  * @param argv array of arguments for this PAM module
  * @return any of the PAM return values
  */
-PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags, int argc, const char **argv)
+PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags __attribute__((unused)), int argc, const char **argv)
 {
 	const char *user = NULL;
 	const char *password = NULL;
@@ -484,7 +482,7 @@  PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags, int argc, const
  * @param argv array of arguments for this PAM module
  * @return PAM_SUCCESS
  */
-PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const char **argv)
+PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph __attribute__((unused)), int flags __attribute__((unused)), int argc __attribute__((unused)), const char **argv __attribute__((unused)))
 {
 	return PAM_SUCCESS;
 }
@@ -498,7 +496,7 @@  PAM_EXTERN int pam_sm_close_session(pam_handle_t *ph, int flags, int argc, const
  * @param argv array of arguments for this PAM module
  * @return PAM_SUCCESS
  */
-PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph, int flags, int argc, const char **argv)
+PAM_EXTERN int pam_sm_setcred(pam_handle_t *ph __attribute__((unused)), int flags __attribute__((unused)), int argc __attribute__((unused)), const char **argv __attribute__((unused)))
 {
 	return PAM_SUCCESS;
 }
@@ -520,15 +518,14 @@  pam_sm_chauthtok (pam_handle_t *ph, int flags, int argc, const char **argv)
 	const char *password = NULL;
 	uint args;
 	int ret;
-	
+
 	args = parse_args(ph, argc, argv, &hostdomain);
 
 	if (flags & PAM_UPDATE_AUTHTOK) {
 		/* Figure out the user name */
 		ret = pam_get_user(ph, &user, NULL);
 		if (ret != PAM_SUCCESS) {
-			pam_syslog(ph, LOG_ERR, "couldn't get the user name: %s", 
-				pam_strerror (ph, ret));
+			pam_syslog(ph, LOG_ERR, "couldn't get the user name: %s", pam_strerror (ph, ret));
 			return PAM_SERVICE_ERR;
 		}
 
@@ -537,14 +534,13 @@  pam_sm_chauthtok (pam_handle_t *ph, int flags, int argc, const char **argv)
 			if (ret == PAM_SUCCESS) {
 				pam_syslog(ph, LOG_WARNING, "no password is available for user");
 			} else {
-				pam_syslog(ph, LOG_WARNING, "no password is available for user: %s",
-					pam_strerror(ph, ret));
+				pam_syslog(ph, LOG_WARNING, "no password is available for user: %s", pam_strerror(ph, ret));
 			}
 			return PAM_AUTHTOK_RECOVER_ERR;
 		}
-	
+
 		return cifscreds_pam_update(ph, user, password, args, hostdomain);
 	}
-	else 
+	else
 		return PAM_IGNORE;
 }
-- 
1.8.4.2