bug-cvs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PAM authentication patch - v2


From: Derek Robert Price
Subject: Re: PAM authentication patch - v2
Date: Mon, 14 Apr 2003 10:54:36 -0400
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.2) Gecko/20030208 Netscape/7.02

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: address@hidden

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







reply via email to

[Prev in Thread] Current Thread [Next in Thread]