openssh bugs - Jan 2015 - [Bug 2335] New: Config parser accepts ip/port in ListenAddress and PermitOpen

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

            Bug ID: 2335
           Summary: Config parser accepts ip/port in ListenAddress and
                    PermitOpen
           Product: Portable OpenSSH
           Version: 6.7p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 2525
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2525&action=edit
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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

--- Comment #1 from Jakub Jelen <jjelen at redhat.com> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2525|0                           |1
        is obsolete|                            |

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3137
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3137&action=edit
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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at dtucker.net

--- Comment #3 from Darren Tucker <dtucker at dtucker.net> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2915


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2915
[Bug 2915] Tracking bug for 8.0 release
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3137|0                           |1
        is obsolete|                            |
                 CC|                            |djm at mindrot.org
   Attachment #3231|                            |ok?(djm at mindrot.org)
              Flags|                            |

--- Comment #4 from Darren Tucker <dtucker at dtucker.net> ---
Created attachment 3231
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3231&action=edit
remove support for obsolete host/port notation

reworked the patch a little to apply to openbsd current.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #5 from Darren Tucker <dtucker at dtucker.net> ---
Patch applied and will be in the 8.0 release.

Thanks.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

--- Comment #6 from Darren Tucker <dtucker at dtucker.net> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3231|ok?(djm at mindrot.org)        |ok+
              Flags|                            |

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 3231
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3231
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

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

--- Comment #8 from Darren Tucker <dtucker at dtucker.net> ---
(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

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kurt at easygo.at

--- Comment #9 from Jakub Jelen <jjelen at redhat.com> ---
*** Bug 3010 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2335

Ahmed Sayeed <ahmedsayeed1982 at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ahmedsayeed1982 at yahoo.com

--- Comment #11 from Ahmed Sayeed <ahmedsayeed1982 at yahoo.com> ---
Possibly similar to 23220 however on 64-bit recent Debian sid with
trivial code I see : https://www.webb-dev.co.uk/category/crypto/

mimas$ 
mimas$ uname -a  http://www.compilatori.com/category/services/
Linux mimas 5.10.0-6-sparc64 #1 Debian 5.10.28-1 (2021-04-09) sparc64
GNU/Linux
mimas$ 
http://www.acpirateradio.co.uk/category/services/
mimas$ 
mimas$ /usr/bin/gcc --version 
http://www.logoarts.co.uk/category/services/
gcc (Debian 10.2.1-6) 10.2.1 20210110
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is
NO http://www.slipstone.co.uk/category/services/ 
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
PURPOSE.

mimas$  http://embermanchester.uk/category/services/

mimas$ 
mimas$ cat -n foo.c  http://connstr.net/category/services/
     1
     2  #include <stdio.h>
     3  #include <stdlib.h>
     4 http://joerg.li/category/services/
     5  int main(int argc, char **argv)
     6  {
     7      int a = 1;
     8 http://www.jopspeech.com/category/services/
     9      printf("a = %i\n", a);
    10 http://www.wearelondonmade.com/category/services/
    11      printf("&a = %p\n", &a);
    12
    13      return EXIT_SUCCESS;
    14 https://waytowhatsnext.com/category/crypto/
    15  }
    16
mimas$  http://www.iu-bloomington.com/category/crypto/

mimas$ 
mimas$ /usr/bin/gcc -std=iso9899:1999 -pedantic -pedantic-errors
-fno-builtin https://komiya-dental.com/category/crypto/  -g -m64 -O0
-mno-app-regs -mcpu=ultrasparc -mmemory-model=tso -o foo foo.c 
mimas$  http://www-look-4.com/category/services/

mimas$ 
mimas$ TERM=dumb LC_ALL=C /usr/bin/gdb ./foo
GNU gdb (Debian 10.1-2) 10.1.90.20210103-git

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.