bug-cvs
[Top][All Lists]
Advanced

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

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


From: Brian Murphy
Subject: Re: PATCH - one time password/always prompt for password
Date: Thu, 31 Jul 2003 19:33:25 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030529

Derek Robert Price wrote:



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

Sure. This was just a draft.


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.

The idea with the -p option is to send a "NULL" password, not just a blank one. A blank password can be a valid one. The design idea is that -p sends blank in the password field of the authentication message of the pserver protocol, not the scrambled version of "". The change in the descramble function is to descramble this blank to a NULL because it doesn't start with 'A'. This means no change in present behaviour and less upset people ;-). This leads naturally to all the checking for nullness of password also.


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.

Yeah it should be password_prompt.



+        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.

The problem with it is that if I set password = " ";, a space, then I would be setting a constant string " " to zero and this "constant" string will be reused at other places in the code leading to very mysterious failures. I have learnt this lesson from experience and it is an awful bug to find.


# 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.

It doesn't have anything to do with OTP as such. "S" stands for secret. The server has asked for something which should not be echoed at the terminal - which could be just as useful for other things. Even if you don't use OTP the procedure works fine with PAM prompting if the unix password module is used for
instance.


+        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?

See above under NULLness ;-). Probably there should be a "B" option instead to indicate the nullness of the password so this doesn't get triggered by mistake and so that other errors in the protocol dont get hidden by them getting caught by the generality of this test.


    /* 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.

See above.


    {
        /* 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 ("").

No.


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)?

See the NULL discussion.


+            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.

ditto.


+    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.

ditto.


+    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,

No prob.

Hope I answered some of your questions in a satisfactory manner :-).

/Brian






reply via email to

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