Bug 2786

Summary:

New OpenSSH fails to parse public keys with bogus whitespace

Product:

Portable OpenSSH

Reporter:

Jakub Jelen <jjelen>

Component:

sshd

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

CLOSED FIXED

Severity:

enhancement

CC:

djm, dtucker

Priority:

Version:

7.5p1

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

2782

Attachments:

Description	Flags
do not be strict about whitespace in public keys	none
refactor sshkey_read, allow more whitespace	none

Description Jakub Jelen 2017-09-28 00:54:32 AEST

Created attachment 3062 [details]
do not be strict about whitespace in public keys

OpenSSH prior 6.7 were more benevolent about whitespace around public key blob. The first of the following keys is parsed properly, but the second is not:

ssh-rsa AAAAB3NzaC1yc2EAAAAB.... blah@example.com
ssh-rsa  AAAAB3NzaC1yc2EAAAAB.... blah@example.com

This was changed in the commit [1], which moved away from uudecode() function (which was skipping bogus whitespace) to sshbuf_b64tod() function (which is already getting zero-length buffer from sshkey_read(), because strchr(cp, ' ') returns the immediately following space in the above example).

The documentation is clear that there should be only single whitespace, but being benevolent about the whitespace is good, especially in case of handling public keys, which can come from various sources, for example users pasting them through web interface (github, gitlab, ...). The attached one-line patch is skipping the whitespace and returns to the old behavior.

This bug is based on the Red Hat bugzilla [2].

[1] https://github.com/openssh/openssh-portable/commit/8668706d
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1493406

Comment 1 Damien Miller 2018-02-23 16:17:43 AEDT

Created attachment 3128 [details]
refactor sshkey_read, allow more whitespace

IMO sshkey_read() needs a more comprehensive refactor since it's quite crufty and internally inconsistent, especially in how it allowed tab characters in some places and not others.

This moves the big switch statement to the start of the function; it was previously used to select v1/v2 key parsing but now only functions as a test of key->type. Moving it allows the the whole function to be outdented.

It switches from strchr to strcspn to find spaces, and now accepts tabs anywhere where whitespace is allowed.

Finally, it no longer modifies its argument during parsing but I'll commit the flipping of its argument to const until later to avoid colliding other in-progress diffs.

Comment 2 Damien Miller 2018-03-18 17:10:52 AEDT

This is committed and will be in 7.7

Comment 3 Damien Miller 2021-04-23 15:11:12 AEST

closing resolved bugs as of 8.6p1 release