openssh bugs - Feb 2006 - [Bug 1157] ssh-keygen doesn't handle DOS line breaks

http://bugzilla.mindrot.org/show_bug.cgi?id=1157

           Summary: ssh-keygen doesn't handle DOS line breaks
           Product: Portable OpenSSH
           Version: 3.8.1p1
          Platform: All
               URL: http://openssh.org/txt/draft-ietf-secsh-publickeyfile-
                    02.txt
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: ssh-keygen
        AssignedTo: bitbucket at mindrot.org
        ReportedBy: pepper at rockefeller.edu


ssh-keygen cannot import files with DOS-style line endings (CR/LF). This
appears to be a violation of draft-ietf-secsh-publickeyfile-02, which says all
line-break styles MUST be supported.

I initially noticed this with a key created on VMS, using DOS-style line
breaks, and confirmed with a brand-new key created with ssh-3.2.9.1. ssh-keygen
was able to import the key with default UNIX-style line endings, but I got
"uudecode failed." with DOS-style line breaks.


pepper at salt:~/.ssh2$ ssh-keygen -i -f id_dsa_2048_a.pub.dos 
uudecode failed.


3.1 Line termination Characters

   In order to achieve the goal of being able to exchange public key
   files between servers, implementations are REQUIRED to read files
   using any of the common line termination sequence, <CR>, <LF> or
   <CR><LF>.

   Implementations may generate files using which ever line termination
   convention is most convenient


My system is:

pepper at salt:~/.ssh$ sw_vers
ProductName:    Mac OS X
ProductVersion: 10.4.4
BuildVersion:   8G32
pepper at salt:~/.ssh$ uname -a
Darwin salt.rockefeller.edu 8.4.0 Darwin Kernel Version 8.4.0: Tue Jan  3
18:22:10 PST 2006; root:xnu-792.6.56.obj~1/RELEASE_PPC Power Macintosh powerpc




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #1 from pepper at rockefeller.edu  2006-02-15 08:20 -------
Created an attachment (id=1067)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1067&action=view)
Here's a commercial key with DOS-style line endings, which chokes ssh-keygen




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #2 from dtucker at zip.com.au  2006-02-15 12:36 -------
Created an attachment (id=1068)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1068&action=view)
Handle keys with CR termination only.

Please try this patch.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #1068 is|0                           |1
           obsolete|                            |




------- Comment #3 from dtucker at zip.com.au  2006-02-15 12:51 -------
(From update of attachment 1068)
Ignore this patch, it breaks other key types.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #4 from dtucker at zip.com.au  2006-02-15 14:38 -------
Created an attachment (id=1069)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1069&action=view)
Make ssh-keygen accept CR, LF or CRLF in keys.

Please try this patch instead.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #5 from pepper at rockefeller.edu  2006-02-15 15:18 -------
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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #6 from dtucker at zip.com.au  2006-02-15 20:06 -------
Created an attachment (id=1070)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1070&action=view)
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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #7 from djm at mindrot.org  2006-02-15 22:50 -------
(From update of attachment 1069)
> 	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';




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #8 from dtucker at zip.com.au  2006-02-15 23:00 -------
(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...




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #9 from dtucker at zip.com.au  2006-02-15 23:06 -------
(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 :-)




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #10 from pepper at rockefeller.edu  2006-02-21 17:14 -------
Do y'all have a consensus on the preferred patch I should test?




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #1069 is|0                           |1
           obsolete|                            |
Attachment #1070 is|0                           |1
           obsolete|                            |




------- Comment #11 from dtucker at zip.com.au  2006-02-21 21:41 -------
Created an attachment (id=1074)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1074&action=view)
Make ssh-keygen accept CR, LF or CRLF in keys take 3.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #12 from pepper at rockefeller.edu  2006-02-23 02:20 -------
Created an attachment (id=1078)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1078&action=view)
A sample key with Mac line breaks

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




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #13 from pepper at rockefeller.edu  2006-02-23 02:22 -------
Patch 1074 works on UNIX & DOS files, but breaks in the Mac case
("unget
error").

pepper at 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?




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #1074 is|0                           |1
           obsolete|                            |




------- Comment #14 from dtucker at zip.com.au  2006-02-26 16:32 -------
Created an attachment (id=1086)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1086&action=view)
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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #15 from pepper at rockefeller.edu  2006-02-27 12:48 -------
The latest patch ("take 4") works for all 3 cases (UNIX, DOS, &
Mac). Thanks!




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #16 from djm at mindrot.org  2006-02-27 22:29 -------
(From update of attachment 1086)
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? :)




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #17 from dtucker at zip.com.au  2006-02-27 22:47 -------
(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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
Attachment #1086 is|0                           |1
           obsolete|                            |




------- Comment #18 from dtucker at zip.com.au  2006-02-27 23:23 -------
Created an attachment (id=1087)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1087&action=view)
Make ssh-keygen accept CR, LF or CRLF in keys take 5, with feedback from djm




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #19 from pepper at rockefeller.edu  2006-02-28 12:45 -------
(In reply to comment #18)
> Created an attachment (id=1087)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1087&action=view)
[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




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #20 from dtucker at zip.com.au  2006-02-28 12:51 -------
(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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157





------- Comment #21 from dtucker at zip.com.au  2006-03-07 23:44 -------
Created an attachment (id=1091)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1091&action=view)
Regress test for this (against -portable)

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




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


djm at mindrot.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1087|                            |ok+
               Flag|                            |




------- Comment #22 from djm at mindrot.org  2006-03-12 15:36 -------
(From update of attachment 1087)
looks sane




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


djm at mindrot.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1091|                            |ok+
               Flag|                            |




------- Comment #23 from djm at mindrot.org  2006-03-12 15:39 -------
(From update of attachment 1091)
ok




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=1157


dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |1155
              nThis|                            |
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED




------- Comment #24 from dtucker at zip.com.au  2006-03-13 19:50 -------
Thanks all.  Patches #1087 and #1091 have been applied and will be in 4.4.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.