ssh-keygen: sanitize ANSI escape sequences in key comment

[Roland Hieber]

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 ***@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]: Loading Image...

[Roland Hieber]

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

...he wrote without attaching the patch...

Sorry.

- Roland

[Étienne Buira]

Post by Roland Hieber

Post by Roland Hieber
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.

Post by Roland Hieber
+ char * pc = NULL;

nitpick: char *pc (without space)?

Post by Roland Hieber
+
+ 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.

[Damien Miller]

Post by Roland Hieber

Post by Roland Hieber
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

[Roland Hieber]

Post by Damien Miller
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.

Thanks, I didn't know about strvis() yet, but it does what I want :)
The new patch is at https://bugzilla.mindrot.org/show_bug.cgi?id=2520

- Roland