--===============1393502083==
Content-Type: multipart/signed; boundary="=-=-=";
micalg=pgp-sha1; protocol="application/pgp-signature"

--=-=-=
Content-Transfer-Encoding: quoted-printable

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 @@
>=20=20
> #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.=20=20

> +


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=3D52;
> + char cpool[52]=3D"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRST=

UVWXYZ";
> + char *otac_passphrase;
> +
> + /* use random # to select characters
> + to build one-time passphrase with them */=20
> +
> + 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=3Dmalloc(otac_length+1);
> + for (i=3D0;i > + ran =3D random();
> + otac_passphrase[i]=3Dcpool[ran%nchars];
> + }
> + strncat(otac_passphrase,"",1);


Why not just:

otac_passphrase[otac_length] =3D 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=3DOTAC_LEN;
> +
> + otac_fifo=3Dmalloc(otac_length);


Does this ever get freed?

> + otac_fifo=3Dgetenv("SSH_OTAC_FIFO");


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

> + if (otac_fifo) {
> + otac_passphrase=3Dmalloc(otac_length+1);
> + otac_passphrase=3Dgen_otac_passphrase(otac_length) ;
> + write_to_fifo(otac_passphrase,otac_fifo);
> + }
>=20=20
> gtk_init(&argc, &argv);
>=20=20
> @@ -213,8 +275,7 @@
> }
>=20=20
> setvbuf(stdout, 0, _IONBF, 0);
> - result =3D passphrase_dialog(message);
> + result =3D passphrase_dialog(message,otac_passphrase,otac_fif o);


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

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iQIVAwUBR0XqRMzS7ZTSFznpAQIUxxAAlhSsdRw/NMsR0TdTLLHXNkUjd/0mvF4D
8eb8/4LO16d3S8LGstaa9JfIRfvEhB+SAP8bub3oCRpCcfcPvSahkWV P09+n/GWh
PGaITjGSAInRHoqSYV5WILY1bCjssploo8Sqoi0K9cgaqAx+JG aiXlZtxH4UFPq7
4fB/D+3w7pMbDKw52dPT8isleKhBIWGB/a03g+3W8GkB4fSdoan4UYAP/jaNQHMj
7UkyEIMIFU4r06XcqxP87TocvCTpJtSHMhtljdhXVO8QpklMm0 xDXV1QEg8CiyrR
ClX/Kj3HvZe+zb164r/OHwbgfIJ2r9/U7OAxSWgZdPiWttuFQQtn6wgSa76agK9D
AWonOcMqlmor9xsMfHkLJL47p5HwcLZCCDJEbDY+3o5bFCus+5 TkaaR1nQCLIE77
Fc50OmVi2WgCI27wb3wtALNYkmJj9tfMgB6qsW3EGDTYMe34nM 9NgAYfvtzAtifk
c7B/SneTma5Ul9cvpGA+29Sx7gyIWq+y2hnBohmsBMaO2VFTa+h0WW n7QOC7thXy
FCj7dHag1r0/Nyeupcz6cmwaSrMROUH4MLMAflpaprzWQpGP/Z7JwbE1N3Iv5XQi
WxrVSXAE0IuxxXJvZHfd8WfNCXJVHUhsP/fo3rrtH9c7jg+5PpJAgFG5WCkePgz/
xRSSE2uz7EE=
=jJ6Y
-----END PGP SIGNATURE-----
--=-=-=--

--===============1393502083==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@mindrot.org
https://lists.mindrot.org/mailman/li...enssh-unix-dev

--===============1393502083==--