[Bug 1393] patch modifies gnome-ssh-askpass to optionally use one-time password

[bugzilla-daemon at bugzilla.mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=1393





--- Comment #2 from Darren Tucker <dtucker at zip.com.au>  2007-11-26 03:06:15 ---
(From update of attachment 1383)
>+#define OTAC_FIFO_LEN	32	/* max fifo name length */

This should probably be MAXPATHLEN, (however see next comment).

>+	/* generate and transmit otac passphrase if env var set */
>+	otac_fifo=malloc(OTAC_FIFO_LEN);
>+	otac_fifo=getenv("SSH_OTAC_FIFO");

You malloc otac_fifo, then immedately overwrite it with the return
value from getenv.  The malloc is unnecessary (and a memory leak).

>+char *
>+write_otac_to_fifo(char *otac_fifo) 

Since this is not exported, you can declare it static like the existing
functions.  Also, if you move it to before its caller you will not need
a declaration line for it.

>+	int i,ran,nchars=52,otac_length=OTAC_PWD_LEN;
>+	char cpool[52]="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";

The "52" is a magic number.  You can do without it by doing something
like:

  char cpool[]="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
  size_t nchars = sizeof(cpool) - 1;

>+	srandom(time(0));

The time is not a secret.  You should use arc4random instead of random.
 The downside of that is that you will need to link with
libopenbsd-compat, libssh and libcrypto on platforms that do not have a
native arc4trandom.

Native arc4random does not require seeding, and the one in the compat
library will seed itself if required.

>+      otac_passphrase=malloc(otac_length+1);

The return value of the malloc is not being checked for failure.

>+	for (i=0;i<otac_length;i++) {
>+		ran = random();
>+		otac_passphrase[i]=cpool[ran%nchars];

Because 2^32 is not divisible by nchars, the passphrase will have a
tiny bias.

>+	otac_passphrase[otac_length] = 0;

Nit: strings are terminated by '\0' not 0.

>+	/* write otac password to fifo */
>+	if ( (out=fopen(otac_fifo,"w")) == NULL) {
>+		mkfifo(otac_fifo, 0660);
>+		out=fopen(otac_fifo,"w");
>+	}

There is no guarantee that this ends up opening the fifo.  The first
fopen can fail for some reason other than the fifo not existing (eg
permissions).  The mkfifo could also fail (permissions again) and the
second fopen could also fail (eg permissions, or a race).

You might also like to match the format of the existing code better as
it makes it easier to read (eg spaces between terms).

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.