openssh unix dev - Jan 2016 - ssh-keygen: sanitize ANSI escape sequences in key comment

Hi,

Today I fiddled around a bit with my OpenSSH public key files, and I noticed
that ssh-keygen prints most non-printable characters in the comment as-is when
showing the fingerprint of a key. This can lead to confusing output on the
terminal when the comment contains ANSI escape characters which are interpreted
by the terminal. The attached public key file serves as an example, which, when
fingerprinted on my Linux terminal, looks like this:

$ ssh-keygen -E sha256 -lf test.pub
1024 MD5:de:ad:be:ef:00:7h:15:15:af:0r:6e:d0:ha:5h:00:00 nobody at example.org
(RSA)

... in nice rainbow colors (see [0] for a screenshot). Also note that a SHA256
hash was requested whereas the output is an MD5 hash (which also contains
invalid characters, so it cannot really be an MD5 hash...), but you get the
point that, in general, this technique can be used to suppress the real
fingerprint of a key and let the user see a different one.

For this reason, I suggest applying the attached patch (based on commit 271df81
from the OpenSSH Portable GitHub repository), which replaces escape characters
(0x1b) in the key comment with a full stop prior to printing it to the terminal.
This should serve as a sufficient workaround for the obfuscating escape
behaviour of the underlying terminal emulator.

Since this is my first patch to OpenSSH, I'm very open for feedback :-)

Regards,

 - Roland

[0]: https://rohieb.name/stuff/Selection_609.png
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test.pub
Type: application/vnd.ms-publisher
Size: 410 bytes
Desc: not available
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20160102/b8bf66f5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20160102/b8bf66f5/attachment-0001.bin>

On 02.01.2016 22:12, Roland Hieber wrote:
> Since this is my first patch to OpenSSH, I'm very open for feedback :-)
...he wrote without attaching the patch...

Sorry.

 - Roland
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sshkeygen-sanitize-ansi-escapes.patch
Type: text/x-patch
Size: 1091 bytes
Desc: not available
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20160102/600d0c25/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20160102/600d0c25/attachment-0003.bin>

On Sat, Jan 02, 2016 at 10:20:15PM +0100, Roland Hieber
wrote:
> On 02.01.2016 22:12, Roland Hieber wrote:
> > Since this is my first patch to OpenSSH, I'm very open for
feedback :-)
> 
> ...he wrote without attaching the patch...
Hi, and thank you for pointing that out.
> +	char * pc = NULL;
nitpick: char *pc (without space)?
> +
> +	while ((pc = strchr(comment, '\x1b'))) {
> +		*pc = '.';
> +	}
> +
Why not adding the escape char to reject list in sshkey_try_load_public
(authfile.c)?

Makes me think that it would be safer to use strspn with a conservative
accept set, or scan all chars for isalnum(c) || isblank(c) ||
ispunct(c).

Just my two cents.

On Sat, 2 Jan 2016, Roland Hieber wrote:
> On 02.01.2016 22:12, Roland Hieber wrote:
> > Since this is my first patch to OpenSSH, I'm very open for
feedback :-)
> 
> ...he wrote without attaching the patch...
> 
> Sorry.
No problem :) Could I ask you to file a bug at https://bugzilla.mindrot.org/ ?

I think we should use strnvis* with VIS_SAFE|VIS_OCTAL flags instead of
explicit white/blacklists of characters.

-d


*
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/stravis.3?query=strnvis&sec=3