Re: PAM authentication patch - v2

[Mark D. Baushke]

Hi Brian,

Brian Murphy <brian@murphy.dk> writes:

> Mark D. Baushke wrote:
> 
> >A possible replacement for the awkard parts of the paragraph might be
> >the following:
> >
> >
> Sounded fine to me and now included in the attached patch.
> 
> >Note: Somewhere along the line, it may be desirable to mark the PAM
> >feature as an experimental feature that may not make it into a fully
> >released version of cvs and suggest positive feedback of the feature be
> >sent to the either the bug-cvs or info-cvs e-mail addresses.
> >
> >
> Also done.
> 
> Also I fixed a stupid configure error which meant the default PAM
> service name
> was "".
> 
> I hope this makes everyone happy ;-).
 
The patch applies cleanly and auto-detects PAM support. I have fully
tested it on FreeBSD (where is passes all of the regression tests in
sanity.sh), and test-compiled it on Redhat 7.3 GNU/Linux and Solaris 7.

I suspect that the AC_HELP_STRING for --enable-pam should really
indicate that it will be enabled if it is detected. 

> +    [Use to enable system authentication with PAM instead of using the 
> +    simple getpwnam interface.  This allows authentication (in theory) 
> +    with any PAM module, e.g. on systems with shadow passwords or via 
> LDAP]), ,

The text probably needs to be rewritten as something like this:

    [Use to enable system authentication with PAM instead of using the
    simple getpwnam interface (default). This allows authentication (in
    theory) with any PAM module (e.g., on systems with shadow passwords
    or via LDAP). Use --disable-pam to disable this feature.]), ,

To be honest, I am not in favor of this being the default (this is
probably not a big surprise to you). I'd rather that the HAVE_PAM logic
be separate from USE_PAM logic. 

It may be that I am only stating the minority opinion, but I think it is
worth raising this point for consideration.

> /Brian
> ? contrib/pam

BTW: You will probably want to send the contrib/pam directory contents
separately or diff them each against /dev/null or something... I suppose
that Derek could create the pam directory in contrib to make life easier
for you.
        
        -- Mark

> Index: config.h.in
> ===================================================================
> RCS file: /cvs/ccvs/config.h.in,v
> retrieving revision 1.74
> diff -u -r1.74 config.h.in
> --- config.h.in       19 Mar 2003 21:13:29 -0000      1.74
> +++ config.h.in       27 Apr 2003 20:00:22 -0000
> @@ -206,6 +206,12 @@
>  /* Define to 1 if you have the <ndir.h> header file, and it defines `DIR'. */
>  #undef HAVE_NDIR_H
>  
> +/* Defined to 1 if you use PAM system authentication instead of getpwnam */
> +#undef HAVE_PAM
> +
> +/* Define if you want a hardcoded PAM service name */
> +#undef PAM_SERVICE_NAME
> +
>  /* Define to 1 if you have the `putenv' function. */
>  #undef HAVE_PUTENV
>  
> Index: configure.in
> ===================================================================
> RCS file: /cvs/ccvs/configure.in,v
> retrieving revision 1.188
> diff -u -r1.188 configure.in
> --- configure.in      11 Apr 2003 17:36:00 -0000      1.188
> +++ configure.in      27 Apr 2003 20:00:23 -0000
> @@ -544,6 +544,24 @@
>  dnl
>  
>  
> +
> +dnl
> +dnl --with-hardcoded-pam-service-name
> +dnl
> +AC_ARG_WITH(
> +  [hardcoded-pam-service-name],
> +  AC_HELP_STRING(
> +    [--with-hardcoded-pam-service-name],
> +    [use this to hard code a service name for PAM cvs authentication 
> +    (defaults to the name cvs is invoked as)]))
> +
> +if test -n "$with_hardcoded_pam_service_name"; then
> +  AC_DEFINE_UNQUOTED(PAM_SERVICE_NAME, "$with_hardcoded_pam_service_name",
> +    [Define to hardcode a service name for PAM])
> +fi
> +
> +
> +
>  dnl
>  dnl Find a temporary directory
>  dnl
> @@ -739,6 +757,37 @@
>         the CVS client disabled (--disable-client)])
>    fi
>  fi
> +
> +
> +
> +dnl
> +dnl Check if PAM authentication is enabled
> +dnl
> +AC_ARG_ENABLE(
> +  [pam],
> +  AC_HELP_STRING(
> +    [--enable-pam],
> +    [Use to enable system authentication with PAM instead of using the 
> +    simple getpwnam interface.  This allows authentication (in theory) 
> +    with any PAM module, e.g. on systems with shadow passwords or via 
> LDAP]), ,
> +  [enable_pam=yes]
> +  )
> +
> +if test yes = $enable_pam; then
> +  AC_CHECK_HEADER(security/pam_appl.h, 
> +    AC_DEFINE(HAVE_PAM, 1, 
> +    [Define to enable system authentication with PAM instead of using the 
> +    simple getpwnam interface.  This allows authentication (in theory) 
> +    with any PAM module, e.g. on systems with shadow passwords or via LDAP])
> +    AC_CHECK_LIB(pam, pam_start, [LIBS="${LIBS} -lpam"],
> +      AC_MSG_ERROR([Could not find PAM libraries but the headers exist.
> +      Give the --disable-pam option to compile without PAM support (or fix
> +      your broken configuration)])
> +    ),
> +    AC_MSG_WARN([PAM authentication disabled - could not find PAM headers])
> +  )
> +fi
> +
>  
>  
>  dnl
> Index: doc/cvs.texinfo
> ===================================================================
> RCS file: /cvs/ccvs/doc/cvs.texinfo,v
> retrieving revision 1.565
> diff -u -r1.565 cvs.texinfo
> --- doc/cvs.texinfo   31 Mar 2003 19:29:48 -0000      1.565
> +++ doc/cvs.texinfo   27 Apr 2003 20:00:32 -0000
> @@ -2489,13 +2489,104 @@
>  the username and password using the operating system's
>  user-lookup routines (this "fallback" behavior can be
>  disabled by setting @code{SystemAuth=no} in the
> -@sc{cvs} @file{config} file, @pxref{config}).  Be
> -aware, however, that falling back to system
> +@sc{cvs} @file{config} file, @pxref{config}).
> +
> +The default fallback behaviour is to look in 
> +@file{/etc/passwd} for this system password but if your
> +system has PAM - Pluggable Authentication Modules - 
> +and @sc{cvs} is configured to use it at compile time 
> +then it will be used instead. This means that with a 
> +global configuration file usually @file{/etc/pam.conf}
> +or possibly @file{/etc/pam.d/cvs}
> +you can tell cvs to use LDAP or normal UNIX passwd 
> +authentication or many other possibilities - see your
> +PAM documentation for details. 
> +
> +Note that PAM is an experimental feature so feedback is encouraged. 
> +Please send a mail to one of the @sc{cvs} mailing lists
> +@code{bugs-cvs@@gnu.org} or @code{info-cvs@@gnu.org} if you use the 
> +@sc{cvs} PAM support.
> +
> +Using PAM gives the system administrator much more 
> +flexibility in how cvs users are authenticated but 
> +no more security than other methods, see below. 
> +
> +CVS needs an "auth" and "account" module in the 
> +PAM configuration file. A typical PAM configuration 
> +would therefore have the following lines 
> +in @file{/etc/pam.conf} to emulate the standard @sc{cvs} 
> +system @file{/etc/passwd} authentication:
> +
> +@example
> +cvs  auth        required    pam_unix.so
> +cvs  account     required    pam_unix.so
> +@end example
> +
> +The the equivalent @file{/etc/pam.d/cvs} would contain
> +
> +@example
> +auth     required    pam_unix.so
> +account          required    pam_unix.so
> +@end example
> +
> +Some systems require a full path to the module so that
> +@file{pam_unix.so} (Linux) would become something like 
> +@file{/usr/lib/security/$ISA/pam_unix.so.1} (Sun Solaris).
> +See the @file{contrib/pam} subdirectory of the @sc{cvs}
> +source distribution for further example configurations.
> +
> +The PAM service name given above as "cvs" is just
> +the service name in the default configuration. 
> +The PAM service name is in fact the same as the name 
> +the @sc{cvs} binary is invoked as. This means that you
> +can have several different authentication configurations. 
> +You can also chose at compile time to remove this 
> +flexibility and hard code a PAM service name into the 
> +binary by configuring using the hardcoded-pam-service-name
> +option thus:
> +
> +@example
> +./configure --with-hardcoded-pam-service-name="cvs"
> +@end example
> +
> +substituting "cvs" for whatever you wish the service name
> +to be. No matter how the binary is now invoked it will always
> +use the same service name, "cvs" in this case.
> +
> +Be aware, however, that falling back to system
>  authentication might be a security risk: @sc{cvs}
>  operations would then be authenticated with that user's
>  regular login password, and the password flies across
>  the network in plaintext.  See @ref{Password
>  authentication security} for more on this.
> +This may be more of a problem with PAM authentication
> +because it is likely that the source of the system 
> +password is some central authentication service like
> +LDAP which is also used to authenticate other services.
> +
> +On the other hand, PAM makes it very easy to change your password
> +regularly. If they are given the option of a one-password system for
> +all of their activities, users are often more willing to change their
> +password on a regular basis.
> +
> +In the non-PAM configuration where the password is stored in the
> +@file{CVSROOT/passwd} file, it is difficult to change passwords on a
> +regular basis since only administrative users (or in some cases
> +processes that act as an administrative user) are typicaly given
> +access to modify this file. So, either there needs to be some
> +hand-crafted web page or set-uid program to update the file, or the
> +update needs to be done by submitting a request to an administrator do
> +perform the duty by hand. In the first case, having to remember to
> +update a separate password on a periodic basis can be difficult. In
> +the second case, the manual nature of the change will typically mean
> +that the password will not be changed unless it is absolutely
> +necessary.
> +
> +Note that PAM administrators should probably avoid configuring
> +one-time-passwords (OTP) for @sc{cvs} authentication/authorization. If
> +OTPs are desired, the administrator may wish to encourage the use of
> +one of the other Client/Server access methods. See the section on
> +@pxref{Remote repositories} for a list of other methods.
>  
>  Right now, the only way to put a password in the
>  @sc{cvs} @file{passwd} file is to paste it there from
> Index: src/server.c
> ===================================================================
> RCS file: /cvs/ccvs/src/server.c,v
> retrieving revision 1.290
> diff -u -r1.290 server.c
> --- src/server.c      17 Mar 2003 06:32:11 -0000      1.290
> +++ src/server.c      27 Apr 2003 20:00:35 -0000
> @@ -5548,66 +5548,118 @@
>      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. */
> +#ifndef PAM_SERVICE_NAME
> +#define PAM_SERVICE_NAME program_name
> +#endif
>  
> -    rc = check_repository_password (username, password, repository,
> -                                 &host_user);
> +struct cvs_pam_userinfo {
> +    char *username;
> +    char *password;
> +};
> +
> +static int
> +cvs_pam_conv(num_msg, msg, resp, appdata_ptr)
> +    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;
>  
> -    if (rc == 2)
> -     return NULL;
> +    assert (ui && ui->username && ui->password && msg && resp);
>  
> -    if (rc == 1)
> +    response = xmalloc(num_msg * sizeof(struct pam_response));
> +    memset(response, 0, num_msg * sizeof(struct pam_response));
> +
> +    for (i = 0; i < num_msg; i++)
>      {
> -     /* host_user already set by reference, so just return. */
> -     goto handle_return;
> +     switch(msg[i]->msg_style) 
> +     {
> +         /* PAM wants a username */
> +         case PAM_PROMPT_ECHO_ON:
> +             response[i].resp = xstrdup(ui->username);
> +             break;
> +         /* PAM wants a password */
> +         case PAM_PROMPT_ECHO_OFF:
> +             response[i].resp = xstrdup(ui->password);
> +             break;
> +         case PAM_ERROR_MSG:
> +         case PAM_TEXT_INFO:
> +             printf("E %s\n",msg[i]->msg);
> +             break;
> +         /* PAM wants something we don't understand - bail out */
> +         default:
> +             goto cleanup;
> +     }
>      }
>  
> -    assert (rc == 0);
> +    *resp = response;
> +    return PAM_SUCCESS;
>  
> -    if (!system_auth)
> +cleanup:
> +    for (i = 0; i < num_msg; i++)
>      {
> -     /* 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);
> +     if (response[i].resp)
> +     {
> +         free(response[i].resp);
> +         response[i].resp = 0;
> +     }
> +    }
> +    free(response);
> +    return PAM_CONV_ERR;
> +}
> +
> +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 };
>  
> +    retval = pam_start(PAM_SERVICE_NAME, username, &conv, &pamh);
> +
> +    if (retval == PAM_SUCCESS)
> +     retval = pam_authenticate(pamh, 0);
> +
> +    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 ();
>      }
>  
> -    /* No cvs password found, so try /etc/passwd. */
> -
> +    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;
>  
>       spw = getspnam (username);
>       if (spw != NULL)
> -     {
>           found_passwd = spw->sp_pwdp;
> -     }
>      }
>  #endif
>  
>      if (found_passwd == NULL && (pw = getpwnam (username)) != NULL)
> -    {
>       found_passwd = pw->pw_passwd;
> -    }
>  
>      if (found_passwd == NULL)
>      {
> @@ -5636,34 +5688,74 @@
>      {
>       /* 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);
>  #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);
> +#ifdef LOG_AUTHPRIV
> +    syslog (LOG_AUTHPRIV | LOG_NOTICE,
> +         "user %s authenticated because of blank system password",
> +         username);
> +#endif
> +    return 1;
> +}
> +#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;
> +
> +    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 ();
>      }
>  
> -    /* user exists but has no password at all */
> -    host_user = NULL;
> +    /* 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
> -    syslog (LOG_AUTHPRIV | LOG_NOTICE,
> -         "login refused for %s: user has no password", username);
> +    if (!host_user)
> +     syslog (LOG_AUTHPRIV | LOG_NOTICE,
> +             "login refused for %s: user has no password", username);
>  #endif
>  
>  handle_return: