Bug 2335

Summary:

Config parser accepts ip/port in ListenAddress and PermitOpen

Product:

Portable OpenSSH

Reporter:

Jakub Jelen <jjelen>

Component:

sshd

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

CLOSED FIXED

Severity:

enhancement

CC:

ahmedsayeed1982, djm, dtucker, kurt

Priority:

Version:

6.7p1

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

2915

Attachments:

Description	Flags
Make config parser more strict to ip:port values	none
New version of patch for OpenSSH 7.7p1 (prerelease)	none
remove support for obsolete host/port notation	djm: ok+

Description Jakub Jelen 2015-01-09 22:10:03 AEDT

Created attachment 2525 [details]
Make config parser more strict to ip:port values

According to manual pages above mentioned options in sshd_config accept only values in format ip:port, but parser used in code also accept ip/port which can lead to unexpected results when someone doesn't understand what he is doing. Great example is our bugzilla [1].

Shortly problem was using ListenAddress 192.168.1.0/24 which ended in converting number 24 into port and in SELinux denial.

This behaviour can be prevented by appended patch, which is accepting only valid values according to manual pages. This is done in function hpdelim, which is used only for parsing above mentioned ListenAddress and PermitOpen (same syntax according to man pages).

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1130733

Comment 1 Jakub Jelen 2016-09-09 01:09:11 AEST

Hello,
can we fix also this, since we are changing the configuration parsers?

    192.168.1.0/24

is certainly not a valid syntax for IP and port pair.

Comment 2 Jakub Jelen 2018-03-27 01:10:32 AEDT

Created attachment 3137 [details]
New version of patch for OpenSSH 7.7p1 (prerelease)

The old patch does not apply anymore since it is now used also for other things so I put together a new version with hpdelim2(). See attached patch.

Any chance getting this finally fixed. Simple test cases, that fail without this patch:

./sshd -f /dev/null -T -oPermitOpen=localhost/222 -oHostKey=regress/rsa| grep 222
./sshd -f /dev/null -T -oListenAddress=localhost/222 -oHostKey=regress/rsa| grep 222

Comment 3 Darren Tucker 2019-01-23 21:31:47 AEDT

Some git archaeology reveals this was added in 2001 to support IPv6 addresses with ports:

https://github.com/openssh/openssh-portable/commit/d71ba5771b5c67b4efd3294ecb85dc4d10d03265

at the time it was documented in sshd(8).  These days it's not in line with the relevant standards (eg https://tools.ietf.org/html/rfc5952#section-6) so we should probably remove it.

Comment 4 Darren Tucker 2019-01-23 21:46:09 AEDT

Created attachment 3231 [details]
remove support for obsolete host/port notation

reworked the patch a little to apply to openbsd current.

Comment 5 Darren Tucker 2019-01-24 08:51:21 AEDT

Patch applied and will be in the 8.0 release.

Thanks.

Comment 6 Darren Tucker 2019-01-24 13:38:19 AEDT

BTW: all of the diffs using hpdelim2 had a bug, fortunately not critical: in the common case where there's no delimiter hpdelim2 does not populate the 2nd arg so you would see spurious failures if the uninitialised memory happened to contain "/".  Fixed in a follow up commit.

Comment 7 Damien Miller 2019-01-26 17:04:49 AEDT

Comment on attachment 3231 [details]
remove support for obsolete host/port notation

ok, but two nits:


>+char	*hpdelim2(char **, char *);

Maybe hpdelim_char() would be a little more descriptive?

>--- servconf.c	19 Jan 2019 21:37:48 -0000	1.346
>+++ servconf.c	23 Jan 2019 10:44:04 -0000
...
>@@ -1251,8 +1251,10 @@ process_server_config_line(ServerOptions
> 			port = 0;
> 			p = arg;
> 		} else {
>-			p = hpdelim(&arg);
>-			if (p == NULL)
>+			char ch;
>+			arg2 = NULL;

Either newline after "char ch" or move it to the declarations of the start of the function

Comment 8 Darren Tucker 2019-01-26 17:09:45 AEDT

(In reply to Damien Miller from comment #7)
> Maybe hpdelim_char() would be a little more descriptive?

Maybe, but this only (re) adds the prototype, the function has existed under this name for a while:
https://github.com/openssh/openssh-portable/commit/887669ef032d63cf07f53cada216fa8a0c9a7d72

> Either newline after "char ch" or move it to the declarations of the
> start of the function

Already fixed in https://github.com/openssh/openssh-portable/commit/281ce042579b834cdc1e74314f1fb2eeb75d2612

Comment 9 Jakub Jelen 2019-05-16 01:15:56 AEST

*** Bug 3010 has been marked as a duplicate of this bug. ***

Comment 10 Damien Miller 2021-04-23 15:08:46 AEST

closing resolved bugs as of 8.6p1 release

Comment 11 Ahmed Sayeed 2021-10-14 01:40:37 AEDT

[spam removed]