[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: patch for pam_unix



On Tue, 28 Dec 1999, Steve Langasek wrote:

>On Tue, 28 Dec 1999, Nalin Dahyabhai wrote:
>> Hi, I found a problem with the new pam_unix's handling of the "unixlike"
>> argument that appears to break it.  I've attached a patch.

>This patch depends on an int and a pointer being the same size, which is a
>non-portable assumption.

>The correct solution is to malloc() the space for the return value, and pass
>its address to pam_set_data().  The pam_sm_setcred() function is then
>responsible for freeing the data when it's done with it.

> I'll pull together a patch later today that does this.

And here's the patch.  I can't believe the code that was in there slipped
through--it clearly doesn't even come close to the desired behavior. <smacking
forehead> :)

A few notes about this code:

* pam_sm_auth() will silently fail to pass the return code to pam_sm_setcred()
  if it can't malloc() the space for it.  This may not be the safest behavior,
  although I can't immediately think of any situations where it's *dangerous*
  for pam_sm_setcred() to return PAM_SUCCESS.  If anyone sees a problem here,
  please feel free to change the behavior.
* pam_unix should be thread-safe; however, multiple invocations of
  pam_sm_auth() in the same pam stack before calling pam_sm_setcred() will
  clobber previous return values, because the name we use
  (unix_setcred_return) is constant.  Is there a way around this?
* I have some notes here about the possibility of pam_get_data() returning
  PAM_NO_MODULE_DATA, and the appropriateness of returning an error code in
  this case. I haven't included this feature in the below patch.

Nalin, can you please confirm that this patch exhibits the correct behavior?
I don't have any machines which depend on the return code that I can use to
test it (which is probably why it escaped my notice :).  Thanks. :)

-Steve Langasek
postmodern programmer


diff -uNr Linux-PAM-0.72-orig/modules/pam_unix/pam_unix_auth.c Linux-PAM-0.72/modules/pam_unix/pam_unix_auth.c
--- Linux-PAM-0.72-orig/modules/pam_unix/pam_unix_auth.c	Sat Oct  9 00:07:32 1999
+++ Linux-PAM-0.72/modules/pam_unix/pam_unix_auth.c	Tue Dec 28 11:47:16 1999
@@ -85,11 +85,12 @@
 
 #define AUTH_RETURN						\
 {								\
-	if (on(UNIX_LIKE_AUTH, ctrl)) {				\
+	if (on(UNIX_LIKE_AUTH, ctrl) && ret_data) {		\
 		D(("recording return code for next time [%d]",	\
 					retval));		\
+		*ret_data = retval;				\
 		pam_set_data(pamh, "unix_setcred_return",	\
-				(void *) &retval, NULL);	\
+				(void *) ret_data, NULL);	\
 	}							\
 	D(("done. [%s]", pam_strerror(pamh, retval)));		\
 	return retval;						\
@@ -99,13 +100,17 @@
 				   ,int argc, const char **argv)
 {
 	unsigned int ctrl;
-	int retval;
+	int retval, *ret_data = NULL;
 	const char *name, *p;
 
 	D(("called."));
 
 	ctrl = _set_ctrl(flags, NULL, argc, argv);
 
+	/* Get a few bytes so we can pass our return value to
+	   pam_sm_setcred(). */
+	ret_data = malloc(sizeof(int));
+
 	/* get the user'name' */
 
 	retval = pam_get_user(pamh, &name, "login: ");
@@ -197,12 +202,16 @@
 	retval = PAM_SUCCESS;
 
 	if (on(UNIX_LIKE_AUTH, ctrl)) {
-		int *pretval = &retval;
+		int *pretval = NULL;
 
 		D(("recovering return code from auth call"));
 		pam_get_data(pamh, "unix_setcred_return", (const void **) &pretval);
 		pam_set_data(pamh, "unix_setcred_return", NULL, NULL);
-		D(("recovered data indicates that old retval was %d", retval));
+		if(pretval) {
+			retval = *pretval;
+			free(pretval);
+			D(("recovered data indicates that old retval was %d", retval));
+		}
 	}
 	return retval;
 }




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index] []