tech-toolchain archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: libpam segfault when passwd passes NULL pamh (was Re: gcc -O2 produces invalid object code (x86_64, netbsd-5 branch))

Joerg Sonnenberger wrote:
My interpretation of pam_end(3) is that a NULL pamh is not prohibited, which would mean that passwd is not doing anything wrong.

I disagree. The handle is an abstract opaque type. An implemenation that only ever allows a single PAM context being open could just return NULL all the time for pam_start and it would be within the spec.

Good point. Let me restate to make sure I understand: For that hypothetical implementation, passing NULL to pam_end() would be appropriate because that's what pam_start() returns. Thus, apps should never test to see if pamh is NULL. Am I correct?

The openpam(3) implementation never returns NULL for pam_start and therefore, NULL is not a valid handle to pass down to pam_end.

So callers should only decide whether to call pam_end() or not based on pam_start()'s return code, correct? What do you think about the attached patch?

There's also the issue of whether it's OK to pass random garbage to pam_strerror(). If it's not OK, then how does one print error messages for pam_start() failures? pam_start() doesn't set pamh on error, so its value could be anything.

Index: usr.bin/passwd/pam_passwd.c
RCS file: /NETBSD-CVS/src/usr.bin/passwd/pam_passwd.c,v
retrieving revision 1.4
diff -u -r1.4 pam_passwd.c
--- usr.bin/passwd/pam_passwd.c 6 May 2007 09:19:44 -0000       1.4
+++ usr.bin/passwd/pam_passwd.c 8 Mar 2010 19:29:47 -0000
@@ -73,6 +73,7 @@
 pwpam_process(const char *username, int argc, char **argv)
        int ch, pam_err;
+       int pam_start_successful = 0;
        char hostname[MAXHOSTNAMELEN + 1];
        while ((ch = getopt(argc, argv, "")) != -1) {
@@ -103,6 +104,7 @@
        /* initialize PAM -- always use the program name "passwd" */
        pam_err = pam_start("passwd", username, &pamc, &pamh);
        pam_check("unable to start PAM session");
+       pam_start_successful = 1;
        pam_err = pam_set_item(pamh, PAM_TTY, ttyname(STDERR_FILENO));
        pam_check("unable to set TTY");
@@ -121,7 +123,8 @@
                    pam_strerror(pamh, pam_err));
-       pam_end(pamh, pam_err);
+       if (pam_start_successful)
+           pam_end(pamh, pam_err);
        if (pam_err == PAM_SUCCESS)
Index: dist/openpam/doc/man/pam_end.3
RCS file: /NETBSD-CVS/src/dist/openpam/doc/man/pam_end.3,v
retrieving revision 1.5
diff -u -r1.5 pam_end.3
--- dist/openpam/doc/man/pam_end.3      27 Jan 2008 01:22:56 -0000      1.5
+++ dist/openpam/doc/man/pam_end.3      8 Mar 2010 19:29:47 -0000
@@ -55,6 +55,10 @@
 function terminates a PAM transaction and destroys the
 corresponding PAM context, releasing all resources allocated to it.
+This function must not be called if the corresponding call to
+.Xr pam_start 3
 .Fa status
 argument should be set to the error code returned by the
@@ -71,6 +75,7 @@
 .Xr pam 3 ,
+.Xr pam_start 3 ,
 .Xr pam_strerror 3

Home | Main Index | Thread Index | Old Index