Bug 1157

Created attachment 1067 [details]
Here's a commercial key with DOS-style line endings, which chokes ssh-keygen

Created attachment 1068 [details]
Handle keys with CR termination only.

Please try this patch.

Comment on attachment 1068 [details]
Handle keys with CR termination only.

Ignore this patch, it breaks other key types.

Created attachment 1069 [details]
Make ssh-keygen accept CR, LF or CRLF in keys.

Please try this patch instead.

This works for the DOS (CR/LF) case, and still works for the UNIX (LF) case, but doesn't work for the (Classic) Mac (CR) case.

Created attachment 1070 [details]
Make ssh-keygen accept CR, LF or CRLF in keys take 2.

Alright, please try this one.  It's not all that elegant because fgets slurps the whole thing into the buffer when it doesn't see what it considers a newline so it seeks back for each subsequent line.

The fgets probably wants replacing with a variant of fgetln that accepts all of CR, LF and CRLF.

Comment on attachment 1069 [details]
Make ssh-keygen accept CR, LF or CRLF in keys.

> 	while (fgets(line, sizeof(line), fp)) {
>-		if (!(p = strchr(line, '\n'))) {
>+		p = strchr(line, '\n');
>+		q = strchr(line, '\r');


Why not just:

if ((p = strchr(line, '\n')) != NULL)
        *p = '\0';
if ((p = strchr(line, '\r')) != NULL)
        *p = '\0';

(In reply to comment #7)
> Why not just:
> 
> if ((p = strchr(line, '\n')) != NULL)
>         *p = '\0';
> if ((p = strchr(line, '\r')) != NULL)
>         *p = '\0';

Well, for one reason, if there's no '\r' in the string, p will now be NULL, and then when we deref it...

(In reply to comment #8)
> Well, for one reason, if there's no '\r' in the string, p will now be NULL, and
> then when we deref it...

... although this:
      if (p > line && p[-1] == '\\')
could be restructured to this:
      len = strlen(line);
      if (len > 0 && line[len - 1] == '\\')
which might even work :-)

Do y'all have a consensus on the preferred patch I should test?

Created attachment 1074 [details]
Make ssh-keygen accept CR, LF or CRLF in keys take 3.

Created attachment 1078 [details]
A sample key with Mac line breaks

With patch #1074, ssh-keygen -i fails on this file.

Patch 1074 works on UNIX & DOS files, but breaks in the Mac case ("unget error").

pepper@pepperbook:~/port/openssh$ ./ssh-keygen -i -f ~/Desktop/ssh-keygen-bug/id_dsa_2048_a.pub.mac
unget: Unknown error: 0

I attached a sample key with Mac line breaks.
ssh-keygen -i also breaks on < (with or without "-f -"). Is this intended to work?

Created attachment 1086 [details]
Make ssh-keygen accept CR, LF or CRLF in keys take 4

OK, this one works with the sample key.  Sooner or later I'll run out of wrong ways to do this :-)

As far as redirection with "-", it's not a supported syntax (for the most part, it works with -T but nothing else).  You can try "ssh-keygen -i -f /dev/stdin", assuming your OS supports it.

The latest patch ("take 4") works for all 3 cases (UNIX, DOS, & Mac). Thanks!

Comment on attachment 1086 [details]
Make ssh-keygen accept CR, LF or CRLF in keys take 4

Looks good, a couple of nits:

>+static int
>+get_line(FILE *fp, char *line, size_t len)
>+{
>+	int c;
>+	size_t pos = 0;
>+
>+	if (len > INT_MAX)
>+		return -1;

I don't think this is necessary in the context.

>+		line[pos++] = c;
>+		line[pos] = '\0';
>+	}
>+	return 0;

Maybe this function should return the length of the string, that would save the strlen() later:

>+	while (get_line(fp, line, sizeof(line)) == 0) {
>+		len = strlen(line);
>+		if (line[len - 1] == '\\')
> 			escaped++;

Regress test? :)

(In reply to comment #16)
> >+	if (len > INT_MAX)
> >+		return -1;
> 
> I don't think this is necessary in the context.

I was being paranoid.

> Maybe this function should return the length of the string, that would
> save the strlen() later:

Good idea, I'll change it.

> Regress test? :)

Some people are never happy :-)  Actually I already have one but posting it as a diff is useless because of the line-break differences in the test data.

Created attachment 1087 [details]
Make ssh-keygen accept CR, LF or CRLF in keys take 5, with feedback from djm

(In reply to comment #18)
> Created an attachment (id=1087) [edit]
> Make ssh-keygen accept CR, LF or CRLF in keys take 5, with feedback from djm

Where do I get v1.141, which patch #5 is against? I don't see it at <http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/ssh-keygen.c>, and the patch doesn't apply against v1.135 from
openssh-4.3.tar.gz or openssh-SNAP-20060213.tar.gz.


Thanks,
Chris

(In reply to comment #19)
> Where do I get v1.141, which patch #5 is against?

Try: http://cvsweb.mindrot.org/index.cgi/openssh/ssh-keygen.c

Not sure why it's not in the snaps, though.

Created attachment 1091 [details]
Regress test for this (against -portable)

In reply to comment #16)
> Regress test? :)

Happy now?  :-)

Comment on attachment 1087 [details]
Make ssh-keygen accept CR, LF or CRLF in keys take 5, with feedback from djm

looks sane

Comment on attachment 1091 [details]
Regress test for this (against -portable)

ok

Thanks all.  Patches #1087 and #1091 have been applied and will be in 4.4.

With the release of 4.4, we believe that this bug is now closed.  For information about the release please see http://www.openssh.com/txt/release-4.4 .