Re: PAM authentication patch - v2

[Brian Murphy]

Derek Robert Price wrote:

> Brian, your patch looked good, though I haven't attempted to install 
> it yet, but it will still need manual (doc/cvs.texinfo) additions 
> before it can be committed.
How is this?

/Brian

Index: config.h.in
===================================================================
RCS file: /cvs/cvs/config.h.in,v
retrieving revision 1.1.1.2
retrieving revision 1.6
diff -u -r1.1.1.2 -r1.6
--- config.h.in 13 Apr 2003 20:34:13 -0000      1.1.1.2
+++ config.h.in 15 Apr 2003 12:49:15 -0000      1.6
@@ -206,6 +206,9 @@
 /* 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 to 1 if you have the `putenv' function. */
 #undef HAVE_PUTENV
 
Index: configure.in
===================================================================
RCS file: /cvs/cvs/configure.in,v
retrieving revision 1.1.1.2
retrieving revision 1.9
diff -u -r1.1.1.2 -r1.9
--- configure.in        13 Apr 2003 20:34:14 -0000      1.1.1.2
+++ configure.in        15 Apr 2003 16:06:30 -0000      1.9
@@ -741,6 +741,37 @@
 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
 dnl Give the confiscator control over whether the server code is compiled
 dnl
Index: doc/cvs.texinfo
===================================================================
RCS file: /cvs/cvs/doc/cvs.texinfo,v
retrieving revision 1.1.1.2
retrieving revision 1.3
diff -u -r1.1.1.2 -r1.3
--- doc/cvs.texinfo     13 Apr 2003 20:34:16 -0000      1.1.1.2
+++ doc/cvs.texinfo     16 Apr 2003 16:25:45 -0000      1.3
@@ -2489,13 +2489,41 @@
 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 - then
+cvs will use that 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. CVS needs an "auth" 
+and "account" module in the PAM configuration file. 
+Using PAM gives the system administrator much more 
+flexibility in how cvs users are authenticated but 
+no more security than other methods, see below.
+
+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 - this is impossible to do 
+for a user authenticated via cvs' private password file
+without total access to the @file{CVSROOT/passwd} file 
+, i.e. the user needs all rights to the repository to 
+allow password change which in my experience means 
+the password never gets changed, see below. Users are
+much more willing to change their password regularly
+if they only have to remember one. 
 
 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/cvs/src/server.c,v
retrieving revision 1.1.1.2
retrieving revision 1.15
diff -u -r1.1.1.2 -r1.15
--- src/server.c        13 Apr 2003 20:34:20 -0000      1.1.1.2
+++ src/server.c        15 Apr 2003 18:46:08 -0000      1.15
@@ -5548,71 +5548,119 @@
     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. */
+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;
 
-    rc = check_repository_password (username, password, repository,
-                                   &host_user);
+    assert (ui && ui->username && ui->password && msg && resp);
 
-    if (rc == 2)
-       return NULL;
+    response = xmalloc(num_msg * sizeof(struct pam_response));
+    memset(response, 0, num_msg * sizeof(struct pam_response));
 
-    if (rc == 1)
+    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(program_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)
     {
        printf ("E Fatal error, aborting.\n\
-error 0 %s: no such user\n", username);
+               error 0 %s: no such user\n", username);
 
        error_exit ();
     }
@@ -5636,34 +5684,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: