Re: PAM authentication patch - v2

[Derek Robert Price]

Comments below.

Brian Murphy wrote:

> Please find attached a patch to authenticate pserver cvs access via PAM.
> I have re-indented to follow the official cvs policy and have made PAM
> a configurable option, only enabled if --with-pam is given as an option
> to configure.
>
> It applies to the cvs1-11-x-branch which I presume is the experimental
> branch. It should also apply to the trunk. 


Actually, the trunk is experimental.  1.11.x is the stable branch.  The 
terminology can get confusing sometimes.

> Please comment, and feel free to test also :-).

...

> +AC_ARG_WITH(
> +  [pam],
> +  AC_HELP_STRING(
> +    [--with-pam],
> +    [Use to enable PAM support]))

--with should be used when the user can supply a different path to a 
library to be used.  For instance, `--with-pam=/usr/lib/pam'.  If you 
are just turning something on and off, you should use the 
--enable/--disable macro, `AC_ARG_ENABLE'.

> +AC_ARG_WITH(
> +  [broken-pam-appdata],
> +  AC_HELP_STRING(
> +    [--with-broken-pam-appdata],
> +    [Use if you have a broken PAM which calls the conversation function
> +    with a null application data pointer, Solaris 2.60 is a culprit here]))

This is _not_ the way to use configure unless a condition is totally 
undetectable.  The whole goal behind autoconf and configure is to 
autodetect conditions that affect your software rather than requiring 
the builder to know or requiring someone to collect the data per 
platform and associate it with a table somewhere.

I assume you could write a snippet of code in C to detect if the 
"application data pointer" is null?  Write that and then call 
AC_TRY_COMPILE, AC_TRY_LINK, or AC_TRY_RUN on the code as necessary. 
See the Autoconf manual for more on these macros and which will prove 
most appropriate for you, but avoid AC_TRY_RUN, if possible.

> Index: config.h.in
> ===================================================================
> RCS file: /cvs/cvs/config.h.in,v
> retrieving revision 1.1.1.1
> retrieving revision 1.3
> diff -u -r1.1.1.1 -r1.3
> --- config.h.in 12 Apr 2003 18:25:21 -0000      1.1.1.1
> +++ config.h.in 12 Apr 2003 19:28:23 -0000      1.3
> @@ -206,6 +206,12 @@
> /* Define to 1 if you have the <ndir.h> header file, and it defines `DIR'. */
> #undef HAVE_NDIR_H
>
> +/* Defined if you have pam */
> +#undef HAVE_PAM
> +
> +/* Defined if you have pam */
> +#undef HAVE_BROKEN_PAM_APPDATA

Define better comments in the calls to AC_DEFINE.

> Index: src/server.c
> ===================================================================
> RCS file: /cvs/cvs/src/server.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 server.c
> --- src/server.c        12 Apr 2003 18:25:22 -0000      1.1.1.1
> +++ src/server.c        13 Apr 2003 16:13:05 -0000
> @@ -5517,50 +5517,114 @@
>     return retval;
> }
>
> +#ifdef HAVE_PAM
>
> -/* Return a hosting username if password matches, else NULL. */
> -static char *
> -check_password (username, password, repository)
> -    char *username, *password, *repository;
> -{
> -    int rc;
> -    char *host_user = NULL;
> -    char *found_passwd = NULL;
> -    struct passwd *pw;
> +#include <security/pam_appl.h>
>
> -    /* First we see if this user has a password in the CVS-specific
> -       password file.  If so, that's enough to authenticate with.  If
> -       not, we'll check /etc/passwd. */
> +#define PAM_SERVICENAME "cvs"

Hrm.  Can we get $PROG (see src/sanity.sh) in here?  This is the program 
prefix defined by configure + the string `cvs'.  This is the actual name 
of the CVS executable since the user can set it at configure time.  It 
shouldn't be hard to use AC_DEFINE_UNQUOTED to define PAM_SERVICENAME to 
the correct string once you figure which shell variable configure is 
storing the value in.

> -    rc = check_repository_password (username, password, repository,
> -                                   &host_user);
> +struct cvs_pam_userinfo {
> +    char *username;
> +    char *password;
> +};
>
> -    if (rc == 2)
> -       return NULL;
> +#ifdef HAVE_BROKEN_PAM_APPDATA
> +struct cvs_pam_userinfo *global_userinfo;
> +#endif
>
> -    if (rc == 1)
> +static int
> +cvs_pam_conv(int num_msg,
> +        const struct pam_message **msg,
> +        struct pam_response **resp,
> +        void *appdata_ptr)
> +{
> +    int i;
> +    struct pam_response *response;
> +    struct cvs_pam_userinfo *ui = (struct cvs_pam_userinfo *)appdata_ptr;
> +
> +#ifdef HAVE_BROKEN_PAM_APPDATA
> +    if(!ui)
>     {
> -       /* host_user already set by reference, so just return. */
> -       goto handle_return;
> +    ui = global_userinfo;
>     }
> +#endif

Is the check for `if (!ui)' really necessary here?  In the 
HAVE_BROKEN_PAM_APPDATA case does PAM really _sometimes_ pass the 
pointer back?

> -    assert (rc == 0);
> +    if (!ui || !ui->username || !ui->password || !msg || !resp)
> +    {
> +    return PAM_CONV_ERR;
> +    }
>
> -    if (!system_auth)
> +    response = malloc(num_msg * sizeof(struct pam_response));

   response = xmalloc(num_msg * sizeof(struct pam_response));


CVS wraps the memory functions in error handling routines...

> +    if (!response)
>     {
> -       /* Note that the message _does_ distinguish between the case in
> -          which we check for a system password and the case in which
> -          we do not.  It is a real pain to track down why it isn't
> -          letting you in if it won't say why, and I am not convinced
> -          that the potential information disclosure to an attacker
> -          outweighs this.  */
> -       printf ("error 0 no such user %s in CVSROOT/passwd\n", username);
> +    return PAM_CONV_ERR;
> +    }

...which print error messages and exit on error and render this error 
handling code and error code return unecessary.

> +    for (i = 0; i < num_msg; i++)
> +    {
> +    response[i].resp_retcode = 0;
> +    response[i].resp = 0;
> +    switch(msg[i]->msg_style) 
> +    {
> +    case PAM_PROMPT_ECHO_ON:
> +        response[i].resp = strdup(ui->username);
> +        break;
> +    case PAM_PROMPT_ECHO_OFF:
> +        response[i].resp = strdup(ui->password);
> +        break;
> +    default:
> +        if(response)
> +        {
> +        free(response);
> +        }
> +        return PAM_CONV_ERR;
> +    }
> +    }

Does PAM really ask you to switch on a msg_style flag and not a username 
or password flag to determine what sort of data to pass?  If so, yuck!

> -       error_exit ();
> +    *resp = response;
> +    return PAM_SUCCESS;
> +}
> +
> +static int
> +check_system_password (username, password)
> +    char *username, *password;
> +{
> +    pam_handle_t *pamh = NULL;
> +    int retval;
> +    struct cvs_pam_userinfo ui = { username, password };
> +    struct pam_conv conv = { cvs_pam_conv, (void *)&ui };
> +
> +#ifdef HAVE_BROKEN_PAM_APPDATA
> +    global_userinfo = &ui;
> +#endif
> +
> +    retval = pam_start(PAM_SERVICENAME, username, &conv, &pamh);
> +
> +    if (retval == PAM_SUCCESS)
> +    {
> +    retval = pam_authenticate(pamh, 0);
>     }
>
> -    /* No cvs password found, so try /etc/passwd. */
> +    if (retval == PAM_SUCCESS)
> +    {
> +    retval = pam_acct_mgmt(pamh, 0);
> +    }
> +
> +    if (pam_end(pamh,retval) != PAM_SUCCESS)
> +    {
> +    printf("E Fatal error, aborting.\n
> +            pam failed to release authenticator\n");
> +    error_exit ();
>
> +    }
>
> +    return (retval == PAM_SUCCESS);       /* indicate success */
> +}
> +#else
> +static int
> +check_system_password (username, password)
> +    char *username, *password;
> +{
> +    char *found_passwd = NULL;
> +    struct passwd *pw;
> #ifdef HAVE_GETSPNAM
>     {
>         struct spwd *spw;
> @@ -5568,7 +5632,7 @@
>         spw = getspnam (username);
>         if (spw != NULL)
>         {
> -           found_passwd = spw->sp_pwdp;
> +       found_passwd = spw->sp_pwdp;

This change is a noop.  Please remove it.

>         }
>     }
> #endif
> @@ -5606,33 +5670,83 @@
>         /* user exists and has a password */
>         if (strcmp (found_passwd, crypt (password, found_passwd)) == 0)
>         {
> -           host_user = xstrdup (username);
> +       return 1;
>         }
>         else
>         {
> -           host_user = NULL;
> #ifdef LOG_AUTHPRIV
> -           syslog (LOG_AUTHPRIV | LOG_NOTICE,
> -                   "password mismatch for %s: %s vs. %s", username,
> -                   crypt(password, found_passwd), found_passwd);
> +       syslog (LOG_AUTHPRIV | LOG_NOTICE,
> +           "password mismatch for %s: %s vs. %s", username,
> +               crypt(password, found_passwd), found_passwd);

Another noop.

> #endif
> +    return 0;
>         }
> -       goto handle_return;
>     }
>
>     if (password && *password)
>     {
> -       /* user exists and has no system password, but we got
> -          one as parameter */
> -       host_user = xstrdup (username);
> +    return 1;
> +    }
> +
> +    return 0;
> +}
> +#endif
> +
> +/* Return a hosting username if password matches, else NULL. */
> +static char *
> +check_password (username, password, repository)
> +    char *username, *password, *repository;
> +{
> +    int rc;
> +    char *host_user = NULL;
> +
> +    /* First we see if this user has a password in the CVS-specific
> +       password file.  If so, that's enough to authenticate with.  If
> +       not, we'll check /etc/passwd. */
> +
> +    rc = check_repository_password (username, password, repository,
> +                                   &host_user);
> +
> +    if (rc == 2)
> +       return NULL;
> +
> +    if (rc == 1)
> +    {
> +       /* host_user already set by reference, so just return. */
>         goto handle_return;
>     }
>
> -    /* user exists but has no password at all */
> +    assert (rc == 0);
> +
> +    if (!system_auth)
> +    {
> +       /* Note that the message _does_ distinguish between the case in
> +          which we check for a system password and the case in which
> +          we do not.  It is a real pain to track down why it isn't
> +          letting you in if it won't say why, and I am not convinced
> +          that the potential information disclosure to an attacker
> +          outweighs this.  */
> +       printf ("error 0 no such user %s in CVSROOT/passwd\n", username);
> +
> +       error_exit ();
> +    }
> +
> +    /* No cvs password found, so try /etc/passwd. */
> +    if ( check_system_password(username, password) )
> +    {
> +    host_user = xstrdup (username);
> +    }
> +    else
> +    {
>     host_user = NULL;
> +    }
> +
> #ifdef LOG_AUTHPRIV
> +    if (!host_user)
> +    {
>     syslog (LOG_AUTHPRIV | LOG_NOTICE,
>             "login refused for %s: user has no password", username);
> +    }
> #endif
>
> handle_return:
>  

Lastly, your indentation appears to be off.  Each new block should be 
indented four spaces, with tab as an acceptable substitute for 8 spaces.

Derek

--
               *8^)

Email: derek@ximbiot.com

Get CVS support at <http://ximbiot.com>!
--
We shall have our follies without doubt.  Some one or more of them will always
be afloat.  But ours will be the follies of enthusiasm, not of bigotry, not of
Jesuitism.  Bigotry is the disease of ignorance, of morbid minds; enthusiasm of
the free and buoyant.  Education and free discussion are the antidotes of both.
We are destined to be a barrier against the return of ignorance and barbarism.

                        - Thomas Jefferson to John Adams, 1816