openssh bugs - Oct 2007 - [Bug 1380] New: incorrect check for strlen(fwd->connect_host) in parse_forward()

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

           Summary: incorrect check for strlen(fwd->connect_host) in
                    parse_forward()
    Classification: Unclassified
           Product: Portable OpenSSH
           Version: 4.7p1
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P3
         Component: ssh
        AssignedTo: bitbucket at mindrot.org
        ReportedBy: Jan.Pechanec at Sun.COM


there are 2 issues for hostname len check in parse_forward()

(a) the len is checked against NI_MAXHOST while it should be checked
against (SSH_CHANNEL_PATH_LEN - 1).

(b) the check should be also performed against listen_host when in
remote fwd mode; otherwise hostname of any length is sent over

The check against connect_host is already in
channel_setup_fwd_listener(). I think that correct way is to remove the
check from parse_forward() completely and put a new check against
listen_host to channel_request_remote_forwarding().

patch attached.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

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





--- Comment #1 from Jan Pechanec <Jan.Pechanec at Sun.COM>  2007-10-23
04:27:25 ---
Created an attachment (id=1367)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1367)
fix for the bug

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

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


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org




--- Comment #2 from Damien Miller <djm at mindrot.org>  2007-11-03
11:12:08 ---
I think it is best to perform the checks in parse_forward too, as this
is called before the connection is established. It is annoying to have
to authenticate before you find out the errors in your configuration or
commandline.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Jan Pechanec <jp at devnull.cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jp at devnull.cz




--- Comment #3 from Jan Pechanec <jp at devnull.cz>  2007-11-04 05:13:12
---
a good point. It might be worth to do more changes then.

parse_forward() doesn't know whether it processes a local or remote
spec so it doesn't know which one of the two address strings is going
to be sent over. However, it doesn't matter because maximum domain name
length is 255 octets (RFC 2181) and the macro SSH_CHANNEL_PATH_LEN is
already defined as 256. Maybe, channels.h could define:

#define MAX_DOMAIN_NAME_LEN     255
#define SSH_CHANNEL_PATH_LEN    MAX_DOMAIN_NAME_LEN

and then use MAX_DOMAIN_NAME_LEN in parse_forward() on both strings
because it's not about setting a channel path there yet but use
SSH_CHANNEL_PATH_LEN in both forward functions where the string is
actually sent over; in theory the channel path length could be shorter
than MAX_DOMAIN_NAME_LEN.

and use +1 for '\0' rather than -1 were it defined as 256.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
             Blocks|                            |1452




--- Comment #4 from Darren Tucker <dtucker at zip.com.au>  2008-06-14
10:29:37 ---
Target 5.1 release.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1367|0                           |1
        is obsolete|                            |
   Attachment #1539|                            |ok?
               Flag|                            |




--- Comment #5 from Damien Miller <djm at mindrot.org>  2008-07-04
14:14:06 ---
Created an attachment (id=1539)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1539)
revised patch

I think this would be better. btw, there was a bug in your change to
channels.c:channel_request_remote_forwarding() - it returned 0
(success) instead of -1 (failure) when a too-long listen_host was
supplied.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1539|0                           |1
        is obsolete|                            |
   Attachment #1539|ok?                         |
               Flag|                            |




--- Comment #6 from Damien Miller <djm at mindrot.org>  2008-07-04
16:48:52 ---
Created an attachment (id=1540)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1540)
revised revised patch

my last patch has a bug: listen_host may be NULL in
channel_request_remote_forwarding()

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1540|                            |ok?
               Flag|                            |




-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380





--- Comment #7 from Jan Pechanec <Jan.Pechanec at Sun.COM>  2008-07-04
20:19:10 ---
I'm wondering, after reading the forwarding section of 4254 again, is
it worth to have SSH_CHANNEL_PATH_LEN at all? 4254 talks about domain
name only, not mentioning any limitation, which implies that one should
rely on the existing spec, RFC 2181. Unless there is any need to limit
the domain name length further, which I doubt, there is no need for 2
macros.

that way, SSH_MAX_DOMAIN_LEN may be the only macro defined and used (in
the last revision of the patch, both macros are used which might be
confusing for the reader of the code).

I would also vote for defining it to 255, use str[SSH_MAX_DOMAIN_LEN +
1] for definition, and "strlen(xxx) > SSH_MAX_DOMAIN_LEN)" in
comparisons. It seems more logical and mainly, more readable. However,
that's already nit picking.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1452                        |1481




--- Comment #8 from Damien Miller <djm at mindrot.org>  2008-07-04
22:06:54 ---
actually, c->path can be used for real pathnames too (e.g. auth
sockets) the constant name is wrong too. I think I'll bump this to
openssh-5.2, so we can just make it properly dynamic.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380





--- Comment #9 from Damien Miller <djm at mindrot.org>  2009-01-13
17:28:14 ---
Could you refresh my memory as to why NI_MAXHOST is inappropriate?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380





--- Comment #10 from Jan Pechanec <Jan.Pechanec at Sun.COM>  2009-01-14
04:26:34 ---
(In reply to comment #9)
> Could you refresh my memory as to why NI_MAXHOST is inappropriate?
trying to remember... I'd say it may be the other way around. The point
which I hasn't explained was that on one side the hostname length is
checked against NI_MAXHOST in parse_forward() while on the other side
it's checked against SSH_CHANNEL_PATH_LEN in channel.c's function
channel_setup_fwd_listener().

NI_MAXHOST is usually defined as 1025 (RFC 2553, but obsoleted by 3493
that doesn't even define it). SSH_CHANNEL_PATH_LEN is defined as 255
and used just once.

So, I suggest the same value should be used on both sides.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1540|0                           |1
        is obsolete|                            |
   Attachment #1540|ok?                         |
               Flag|                            |




--- Comment #11 from Damien Miller <djm at mindrot.org>  2009-01-14
12:58:18 ---
Created an attachment (id=1591)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1591)
make c->path dynamic and nuke SSH_CHANNEL_PATH_LEN

Thanks - that refreshed my memory. This is what I had planned on doing:
get rid of SSH_CHANNEL_PATH_LEN entirely by making c->path a dynamic
string.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Tim Rice <tim at multitalents.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tim at multitalents.net




--- Comment #12 from Tim Rice <tim at multitalents.net>  2009-01-14
16:52:43 ---
(In reply to comment #11)
> Created an attachment (id=1591)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1591)
[details]
> make c->path dynamic and nuke SSH_CHANNEL_PATH_LEN
> 
> Thanks - that refreshed my memory. This is what I had planned on doing:
> get rid of SSH_CHANNEL_PATH_LEN entirely by making c->path a dynamic
> string.
A quick look at the code turned up
+    u_char *p, dest_addr[255+1], ntop[INET6_ADDRSTRLEN];
We'll need to account for machines that don't have INET6_ADDRSTRLEN

Maybe just adding
#ifndef INET6_ADDRSTRLEN                /* for non IPv6 machines */
#define INET6_ADDRSTRLEN 46
#endif
like we do in sshconnect.c

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

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




--- Comment #13 from Damien Miller <djm at mindrot.org>  2009-01-22
20:50:55 ---
patch applied - this will be in openssh-5.2

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1380


Damien Miller <djm at mindrot.org> changed:

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




--- Comment #14 from Damien Miller <djm at mindrot.org>  2009-02-23
13:35:35 ---
Close bugs fixed/reviewed for openssh-5.2 release

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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.