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

[Richard Hansen <rhansen%bbn.com@localhost>]

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.

Thanks,
Richard

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));
 
  end:
-       pam_end(pamh, pam_err);
+       if (pam_start_successful)
+           pam_end(pamh, pam_err);
        if (pam_err == PAM_SUCCESS)
                return;
        exit(1);
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.
 .Pp
+This function must not be called if the corresponding call to
+.Xr pam_start 3
+failed.
+.Pp
 The
 .Fa status
 argument should be set to the error code returned by the
@@ -71,6 +75,7 @@
 .El
 .Sh SEE ALSO
 .Xr pam 3 ,
+.Xr pam_start 3 ,
 .Xr pam_strerror 3
 .Sh STANDARDS
 .Rs