Re: PATCH - one time password/always prompt for password

[Derek Robert Price]

Brian Murphy wrote:

> Hi Derek,
>    you asked me to impliment something which would not send a password 
> with
> a pserver request and in this case the server could prompt the user and
> accept  input from the user directly.
>
> Here is my first attempt. It's not beautiful but it works.


Wow!  Thanks!  I didn't expect such a fast response.

> If you install the pam opie module and the opie client and server then 
> the following /etc/pam.d/cvs
> allows one time password support
>
> auth      sufficient        pam_opie.so
> auth      sufficient        pam_unix.so
> auth      required        pam_deny.so
>
> account         required        pam_unix.so
>
> All comments/testing welcome. 


Good - here it comes.  :)

First off, this needs documentation and ChangeLogs to go with it.

Second, it occurs to me that the -p option itself might be overkill - 
just like a standard client tries an empty password if it can't find one 
in ~/.cvspass for this server, it should be able to always prompt the 
user for a password if the empty pass fails _or_ if it recieves the 
OTP-needed response from the server.  Unless you want to argue about 
this, please disregard my comments regarding no_password_prompt and 
documenting -p below as the option and variable should be removed 
entirely anyhow.

> Index: src/client.c
>
> ===================================================================
> RCS file: /cvs/ccvs/src/client.c,v
> retrieving revision 1.347
> diff -u -r1.347 client.c
> --- src/client.c        23 Jul 2003 21:49:31 -0000      1.347
> +++ src/client.c        30 Jul 2003 20:08:17 -0000
> @@ -3226,7 +3226,11 @@
>         }
>
>         /* Get the password, probably from ~/.cvspass. */
> -       password = get_cvs_password ();
> +       if (no_password_prompt)

no_password_prompt is a confusing name.  What this option actually seems 
to be doing is stopping the password from being sent and _adding_ a 
prompt with every CVS command.

> +           password = "";
> +       else
> +           password = get_cvs_password ();
> +
>         username = root->username ? root->username : getcaller();
>
>         /* Send the empty string by default.  This is so anonymous CVS
> @@ -3254,7 +3258,8 @@
>         send_to_server_via(to_server, "\012", 1);
>
>         /* Paranoia. */
> -        memset (password, 0, strlen (password));
> +       if (!no_password_prompt)
> +           memset (password, 0, strlen (password));

It is not necessary to skip the memset() here - memset() can handle a 
string length of 0 just fine.

> # else /* ! AUTH_CLIENT_SUPPORT */
>         error (1, 0, "INTERNAL ERROR: This client does not support pserver 
> authentication");
> # endif /* AUTH_CLIENT_SUPPORT */
> @@ -3304,6 +3309,12 @@
>                 fprintf (stderr, "%s\n", read_buf + 2);
>
>                 /* Continue with the authentication protocol.  */
> +           }
> +           else if (strncmp (read_buf, "S ", 2) == 0)
> +           {

The "S" response is going to need to be documented in 
doc/cvsclient.texi.  I think I might like another response name, like 
"OTP-needed", or perhaps send-password, given that the server should 
have just tried an empty password, as noted below.

> +               char *response = getpass(read_buf + 2);
> +               send_to_server_via(to_server, response, 0);
> +               send_to_server_via(to_server, "\012", 1);
>             }
>             else if (strncmp (read_buf, "error ", 6) == 0)
>             {
> Index: src/cvs.h
> ===================================================================
> RCS file: /cvs/ccvs/src/cvs.h,v
> retrieving revision 1.269
> diff -u -r1.269 cvs.h
> --- src/cvs.h   25 Jul 2003 20:17:06 -0000      1.269
> +++ src/cvs.h   30 Jul 2003 20:08:18 -0000
> @@ -347,6 +347,7 @@
> extern char *Tmpdir, *Editor;
> extern int cvsadmin_root;
> extern char *CurDir;
> +extern int no_password_prompt;
> extern int really_quiet, quiet;
> extern int use_editor;
> extern int cvswrite;
> Index: src/login.c
> ===================================================================
> RCS file: /cvs/ccvs/src/login.c,v
> retrieving revision 1.77
> diff -u -r1.77 login.c
> --- src/login.c 29 Jul 2003 14:00:10 -0000      1.77
> +++ src/login.c 30 Jul 2003 20:08:18 -0000
> @@ -508,6 +508,11 @@
>     if (argc < 0)
>         usage (login_usage);
>
> +    if (no_password_prompt)
> +    {
> +       error (1, 0, "can't use the `login' command with the -p option (no 
> password)");

This will need rephrasing after the name of the no_password_prompt 
variable is changed.

> +    }
> +
>     if (current_parsed_root->method != pserver_method)
>     {
>         error (0, 0, "can only use `login' command with the 'pserver' method");
> Index: src/main.c
> ===================================================================
> RCS file: /cvs/ccvs/src/main.c,v
> retrieving revision 1.189
> diff -u -r1.189 main.c
> --- src/main.c  24 Jul 2003 13:44:19 -0000      1.189
> +++ src/main.c  30 Jul 2003 20:08:19 -0000
> @@ -37,6 +37,7 @@
> int use_editor = 1;
> int use_cvsrc = 1;
> int cvswrite = !CVSREAD_DFLT;
> +int no_password_prompt = 0;
> int really_quiet = 0;
> int quiet = 0;
> int trace = 0;
> @@ -403,7 +404,7 @@
>     int help = 0;               /* Has the user asked for help?  This
>                                    lets us support the `cvs -H cmd'
>                                    convention to give help for cmd. */
> -    static const char short_options[] = "+QqrwtnRvb:T:e:d:Hfz:s:xa";
> +    static const char short_options[] = "+pQqrwtnRvb:T:e:d:Hfz:s:xa";

This new option will need to be added to the usage message.

>     static struct option long_options[] =
>     {
>         {"help", 0, NULL, 'H'},
> @@ -519,6 +520,9 @@
>             case 3:
>                 /* --allow-root */
>                 root_allow_add (optarg);
> +               break;
> +           case 'p':
> +               no_password_prompt = 1;
>                 break;
>             case 'Q':
>                 really_quiet = 1;
> Index: src/scramble.c
> ===================================================================
> RCS file: /cvs/ccvs/src/scramble.c,v
> retrieving revision 1.17
> diff -u -r1.17 scramble.c
> --- src/scramble.c      23 Jul 2003 20:42:26 -0000      1.17
> +++ src/scramble.c      30 Jul 2003 20:08:19 -0000
> @@ -114,15 +114,7 @@
>     /* For now we can only handle one kind of scrambling.  In the future
>        there may be other kinds, and this `if' will become a `switch'.  */
>     if (str[0] != 'A')
> -#ifndef DIAGNOSTIC
> -       error (1, 0, "descramble: unknown scrambling method");
> -#else  /* DIAGNOSTIC */
> -    {
> -       fprintf (stderr, "descramble: unknown scrambling method\n", str);
> -       fflush (stderr);
> -       exit (EXIT_FAILURE);
> -    }
> -#endif  /* DIAGNOSTIC */
> +       return NULL;

What's all this, then?  At the least it looks like this deserves to be 
in a separate patch?

>     /* Method `A' is symmetrical, so scramble again to decrypt. */
>     s = scramble (str + 1);
> Index: src/server.c
> ===================================================================
> RCS file: /cvs/ccvs/src/server.c,v
> retrieving revision 1.309
> diff -u -r1.309 server.c
> --- src/server.c        25 Jul 2003 16:25:25 -0000      1.309
> +++ src/server.c        30 Jul 2003 20:08:22 -0000
> @@ -5231,7 +5231,8 @@
>
>         /* Verify blank passwords directly, otherwise use crypt(). */
>         if ((found_password == NULL)
> -           || ((strcmp (found_password, crypt (password, found_password))
> +	    || (password && 
> +		(strcmp (found_password, crypt (password, found_password))
>                  == 0)))

It still isn't possible for the server to get here with a NULL password.

>         {
>             /* Give host_user_ptr permanent storage. */
> @@ -5278,8 +5279,11 @@
>     int i;
>     struct pam_response *response;
>     struct cvs_pam_userinfo *ui = (struct cvs_pam_userinfo *)appdata_ptr;
> +    char *tmp = NULL;
> +    size_t tmp_allocated = 0;
> +    int len;
>
> -    assert (ui && ui->username && ui->password && msg && resp);
> +    assert (ui && ui->username && msg && resp);
>
>     response = xmalloc(num_msg * sizeof(struct pam_response));
>     memset(response, 0, num_msg * sizeof(struct pam_response));
> @@ -5294,11 +5298,21 @@
>                 break;
>             /* PAM wants a password */
>             case PAM_PROMPT_ECHO_OFF:
> -               response[i].resp = xstrdup(ui->password);
> +               if (ui->password)

In the case you are worried about, password will not be NULL, but empty 
("").

Also, are you certain you should exclude even an empty password from a 
test against the PAM database?  Is it valid via PAM to try the empty 
password then come back and try asking the user for a password (probably 
only when the password is empty)?

> +                   response[i].resp = xstrdup(ui->password);
> +               else {
> +                   printf("S %s\n", msg[i]->msg);
> +                   fflush (stdout);
> +                   getnline( &tmp, &tmp_allocated, PATH_MAX, stdin );
> +                   len = strlen(tmp);
> +                   if (len>1 && tmp[strlen(tmp)-1] == '\n')
> +                       tmp[strlen(tmp)-1] = '\0';
> +                   response[i].resp = tmp;
> +               }
>                 break;
>             case PAM_ERROR_MSG:
>             case PAM_TEXT_INFO:
> -               printf("E %s\n",msg[i]->msg);
> +               printf("E %s\n", msg[i]->msg);
>                 break;
>             /* PAM wants something we don't understand - bail out */
>             default:
> @@ -5372,6 +5386,13 @@
>     }
> #endif
>
> +    if (password == NULL) {

Again, password can't be NULL here, but it might be empty and an empty 
password might be valid.

> +       printf ("E Fatal error, aborting.\n"
> +               "error 0 got NULL password. Try using cvs without the -p 
> option\n");
> +
> +       error_exit ();
> +    }
> +
>     if (found_passwd == NULL && (pw = getpwnam (username)) != NULL)
>         found_passwd = pw->pw_passwd;
>
> @@ -5644,13 +5665,15 @@
>     /* We need the real cleartext before we hash it. */
>     descrambled_password = descramble (password);
>     host_user = check_password (username, descrambled_password, repository);
> +    if (descrambled_password != NULL) {

descrambled_password can not be NULL either.

> +       memset (descrambled_password, 0, strlen (descrambled_password));
> +       free (descrambled_password);
> +    }
>     if (host_user == NULL)
>     {
> #ifdef HAVE_SYSLOG_H
>         syslog (LOG_DAEMON | LOG_NOTICE, "login failure (for %s)", repository);
> #endif
> -       memset (descrambled_password, 0, strlen (descrambled_password));
> -       free (descrambled_password);
>     i_hate_you:
>         printf ("I HATE YOU\n");
>         fflush (stdout);
> @@ -5659,8 +5682,6 @@
>            yet.  */
>         error_exit ();
>     }
> -    memset (descrambled_password, 0, strlen (descrambled_password));
> -    free (descrambled_password);
>
>     /* Don't go any farther if we're just responding to "cvs login". */
>     if (verify_and_exit)
>  

Thanks again,

Derek

--
               *8^)

Email: derek@ximbiot.com

Get CVS support at <http://ximbiot.com>!
--
Boy:  A noise with dirt on it