Bug 1565

Summary:

ssh-keyscan doesn't like comment-lines

Product:

Portable OpenSSH

Reporter:

Martin Schuster <schuster>

Component:

Miscellaneous

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

CLOSED FIXED

Severity:

minor

CC:

djm, dtucker, joachim

Priority:

Keywords:

low-hanging-fruit

Version:

5.1p1

Hardware:

All

OS:

Linux

Bug Depends on:

Bug Blocks:

1708

Attachments:

Description	Flags
Patch to fix integer overflow in fgets() wrapper	none
/home/djm/keyscan-uncrazy.diff	none
/home/djm/keyscan-uncrazy.diff	none
/home/djm/keyscan-uncrazy.diff	dtucker: ok+
Patch to fix ssh-keyscan	none

Description Martin Schuster 2009-03-04 20:00:19 AEDT

ssh-keyscan can take an existing known_hosts file as input, but only if it contains no comment-lines.

To reproduce:
echo '#' > khtest
ssh-keyscan -f khtest

Result:
getaddrinfo #: Name or service not known

Expected:
Nothing happens

Comment 1 Joachim Schipper 2010-03-07 08:56:52 AEDT

Created attachment 1806 [details]
Patch to fix integer overflow in fgets() wrapper

Comment 2 Joachim Schipper 2010-03-07 08:58:07 AEDT

As described in http://mid.gmane.org/20100306210548.GA32662@polymnia.sshunet.nl, ssh-keyscan may suffer an integer overflow when run on a file with ridiculously (> 2GB) long lines. The attached patch fixes this and also allows comments.

Comment 3 Damien Miller 2010-06-18 13:40:03 AEST

Created attachment 1868 [details]
/home/djm/keyscan-uncrazy.diff

use read_keyfile_line()\n\nWe already have a fgets() wrapper, let's use it.

Comment 4 Damien Miller 2010-06-18 13:41:55 AEST

Created attachment 1869 [details]
/home/djm/keyscan-uncrazy.diff

revised diff

Comment 5 Damien Miller 2010-06-18 13:42:58 AEST

Comment on attachment 1869 [details]
/home/djm/keyscan-uncrazy.diff

ugh, attached the wrong diff twice :(

Comment 6 Damien Miller 2010-06-18 13:44:02 AEST

Created attachment 1870 [details]
/home/djm/keyscan-uncrazy.diff

The original diff didn't correctly handle the case of "ssh-keyscan -f -" (it would SEGV or EINVAL on fopen). This one uses our existing wrapper for fgets().

Comment 7 Joachim Schipper 2010-06-18 19:56:01 AEST

Created attachment 1875 [details]
Patch to fix ssh-keyscan

The attached patch is a slight alteration of your (Damien's) patch.

- these lines are not related to SSH_MAX_PUBKEY_BYTES, so just hardcode some reasonable value;
- linenum should be per-file, not over all files;
- fatal() on long lines instead of silently ignoring them.

Comment 8 Darren Tucker 2010-06-22 14:54:29 AEST

(In reply to comment #7)
> Created attachment 1875 [details]
> Patch to fix ssh-keyscan
> 
> The attached patch is a slight alteration of your (Damien's) patch.
> 
> - these lines are not related to SSH_MAX_PUBKEY_BYTES, so just hardcode
> some reasonable value;
> - linenum should be per-file, not over all files;

These are both valid points.

> - fatal() on long lines instead of silently ignoring them.

This one I don't care so much about.

Comment 9 Damien Miller 2010-06-22 14:55:43 AEST

I agree with Darren. The corresponding patch has been committed and will be in OpenSSH-5.6. Thanks!

Comment 10 Damien Miller 2011-01-24 12:33:55 AEDT

Move resolved bugs to CLOSED after 5.7 release