[PATCH] one-time ssh-agent confirmation password

[Daniel Kahn Gillmor]

On Thu 2007-11-22 00:59:46 -0500, paul sery wrote:

> The patch (against 4.7p1) modifies gnome-ssh-askpass to optionally
> generate a one-time password and transmits it to the user via an
> out-of-band communication channel. If you can read the password and
> enter it back into the gnome-ssh-askpass dialog, ssh-agent is
> allowed to continue with the authentication process.

This is an interesting idea.  Thanks for publishing!  I haven't had
time to digest it enough to know if the general framework is something
i want, but here's a couple quick notes about the diff:

> --- gnome-ssh-askpass2.c.orig	2003-11-21 05:48:56.000000000 -0700
> +++ gnome-ssh-askpass2.c	2007-11-01 10:54:11.000000000 -0600
> @@ -38,6 +38,9 @@
>  
>  #define GRAB_TRIES	16
>  #define GRAB_WAIT	250 /* milliseconds */
> +#define OTAC_LEN         4  /* number of characters in otac passphrase */
> +#define OTAC_FIFO_LEN	128	/* number of characters in otac passphrase */

Both constants have the same comment, but have different values.  I
suspect one of the comments is wrong.  

> +

Why add the blank line?

> -passphrase_dialog(char *message)
> +passphrase_dialog(char *message, char *otac_passphrase, char *otac_fifo)

Your additions to this function (and elsewhere) seem to use spaces for
indentation.  Previous code in this file uses tabs for indentation.
Sticking to the coding convention will make your patch more appealing
to the original author.

> +/* generate the one-time agent confirm password */
> +char *
> +gen_otac_passphrase(int otac_length)
> +{

spaces v. tabs in this function, as well.

> +        int i,ran,nchars=52;
> +        char cpool[52]="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
> +        char *otac_passphrase;
> +
> +        /* use random # to select characters
> +           to build one-time passphrase with them */ 
> +
> +        srandom(time(0));

Seeding with the time (in seconds since the UNIX epoch) means that
every one of these one-time-passwords that happens in a given second
is going to use the same random password.  So that password will be
predictable -- probably not a property you intend the one time
passwords to have.

> +        otac_passphrase=malloc(otac_length+1);
> +        for (i=0;i<otac_length;i++) {
> +                ran = random();
> +                otac_passphrase[i]=cpool[ran%nchars];
> +        }
> +	strncat(otac_passphrase,"",1);

Why not just:

 otac_passphrase[otac_length] = 0;

> +
> +        return(otac_passphrase);
> +}

There's some memory management weirdness going on here.  You allocate
the otac_passphrase buffer in one function, pass it around through at
least two others, and rely on the others to free it.  While it looks
like you've cleared this one up properly, it's probably not best
practices.

>  int
>  main(int argc, char **argv)
>  {
> -	char *message;
> -	int result;
> +        char *message,*passphrase,*otac_passphrase,*otac_fifo;
> +        int result,otac_length=OTAC_LEN;
> +
> +	otac_fifo=malloc(otac_length);

Does this ever get freed?

> +        otac_fifo=getenv("SSH_OTAC_FIFO");

This looks like it overwrites the pointer to the buffer allocated
immediately above.

> +        if (otac_fifo) {
> +                otac_passphrase=malloc(otac_length+1);
> +                otac_passphrase=gen_otac_passphrase(otac_length);
> +                write_to_fifo(otac_passphrase,otac_fifo);
> +        }
>  
>  	gtk_init(&argc, &argv);
>  
> @@ -213,8 +275,7 @@
>  	}
>  
>  	setvbuf(stdout, 0, _IONBF, 0);
> -	result = passphrase_dialog(message);
> +	result = passphrase_dialog(message,otac_passphrase,otac_fifo);

if (!otac_fifo), then what is otac_passphrase set to when you pass it
in this function?  Passing uninitialized pointers is dangerous.

>  	g_free(message);
> -

Why make a whitespace change here?

Thanks again for publishing this idea.  For patches that you want
people to consider against OpenSSH, you probably want to post them to
the OpenSSH bugzilla (not just this mailing list):

 https://bugzilla.mindrot.org/

That makes your work easier to find for people looking for it later.

Regards,

        --dkg
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 826 bytes
Desc: not available
Url : http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20071122/226294ae/attachment-0001.bin