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

[Joerg Sonnenberger <joerg%britannica.bec.de@localhost>]

On Mon, Mar 08, 2010 at 02:32:02PM -0500, Richard Hansen wrote:
> 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?

Exactly.

> >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?

Not good enough for the issue you have outlined next.

> 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.

Fully agreed. Given that pam_start can only fail due to programming
errors or memory allocation, the error message is not very useful
anyway. I think the attached patch is the best approach.

Joerg

Index: pam_passwd.c
===================================================================
RCS file: /home/joerg/repo/netbsd/src/usr.bin/passwd/pam_passwd.c,v
retrieving revision 1.5
diff -u -p -r1.5 pam_passwd.c
--- pam_passwd.c        2 Mar 2010 16:19:13 -0000       1.5
+++ pam_passwd.c        8 Mar 2010 19:38:44 -0000
@@ -108,7 +108,8 @@ pwpam_process(const char *username, int 
 
        /* initialize PAM -- always use the program name "passwd" */
        pam_err = pam_start("passwd", username, &pamc, &pamh);
-       pam_check("unable to start PAM session");
+       if (pam_err != PAM_SUCCESS)
+               errx(1, "unable to start PAM session");
 
        pam_err = pam_set_item(pamh, PAM_TTY, ttyname(STDERR_FILENO));
        pam_check("unable to set TTY");