openssh unix dev - Sep 2002 - OpenSSH -current fails regression on Solaris 8, sshd dumps core

Hi All.

While working on something I noticed a regression failure on Solaris 8.
It turned out to be present in -cvs and wasn't due to my changes.

One of the tests that fail is basically:
ssh -2 -F $build/regress/ssh_proxy 999.999.999.999 true

The server reports:
sshd[20529]: Disconnecting: Command terminated on signal 11.

The culprit seems to be session.c line 1019 or so:

  snprintf(buf, sizeof buf, "%.50s %d %.50s %d",
    get_remote_ipaddr(), get_remote_port(),
    get_local_ipaddr(packet_get_connection_in()), get_local_port());

After poking around, it seems that:
1) get_local_ipaddr returns NULL
2) this NULL is passed to snprintf
3) which dereferences the NULL causing a SEGV

(get_local_ipaddr returns NULL because it calls get_socket_address which
calls getpeername on a non-socket.)

The NULL doesn't seem to bother snprintf on Linux or HP-UX. I don't know
if it's valid to pass a NULL as an argument to "%s".

The attached patch fixes this problem but introduces more inconsistency
into the get_[local|remote|peer]_[ipaddr|name] functions in canohost.c.
There's probably a neater way of doing this.

The patch has been regression tested on Solaris 8, HP-UX 11 & Redhat
7.3.

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
-------------- next part --------------
Index: canohost.c
==================================================================RCS file:
/cvs/openssh/canohost.c,v
retrieving revision 1.30
diff -u -r1.30 canohost.c
--- canohost.c	11 Jul 2002 03:56:47 -0000	1.30
+++ canohost.c	21 Sep 2002 14:28:42 -0000
@@ -246,10 +246,29 @@
 	return get_socket_address(socket, 1, NI_NUMERICHOST);
 }
 
-char *
-get_local_ipaddr(int socket)
+/*
+ * Returns the IP-address of the local host as a string.  The returned
+ * string must not be freed.
+ */
+
+const char *
+get_local_ipaddr(void)
 {
-	return get_socket_address(socket, 0, NI_NUMERICHOST);
+	static char *canonical_host_ip = NULL;
+
+	/* Check whether we have cached the ipaddr. */
+	if (canonical_host_ip == NULL) {
+		if (packet_connection_is_on_socket()) {
+			canonical_host_ip +			    get_socket_address(packet_get_connection_in(), 0,
NI_NUMERICHOST);
+			if (canonical_host_ip == NULL)
+				fatal_cleanup();
+		} else {
+			/* If not on socket, return UNKNOWN. */
+			canonical_host_ip = xstrdup("UNKNOWN");
+		}
+	}
+	return canonical_host_ip;
 }
 
 char *
Index: canohost.h
==================================================================RCS file:
/cvs/openssh/canohost.h,v
retrieving revision 1.8
diff -u -r1.8 canohost.h
--- canohost.h	4 Jul 2001 04:46:57 -0000	1.8
+++ canohost.h	21 Sep 2002 14:28:42 -0000
@@ -18,7 +18,7 @@
 
 char		*get_peer_ipaddr(int);
 int		 get_peer_port(int);
-char		*get_local_ipaddr(int);
+const char	*get_local_ipaddr(void);
 char		*get_local_name(int);
 
 int		 get_remote_port(void);
Index: session.c
==================================================================RCS file:
/cvs/openssh/session.c,v
retrieving revision 1.220
diff -u -r1.220 session.c
--- session.c	19 Sep 2002 01:50:49 -0000	1.220
+++ session.c	21 Sep 2002 14:28:43 -0000
@@ -1018,7 +1018,7 @@
 
 	snprintf(buf, sizeof buf, "%.50s %d %.50s %d",
 	    get_remote_ipaddr(), get_remote_port(),
-	    get_local_ipaddr(packet_get_connection_in()), get_local_port());
+	    get_local_ipaddr(), get_local_port());
 	child_set_env(&env, &envsize, "SSH_CONNECTION", buf);
 
 	if (s->ttyfd != -1)

Darren Tucker wrote:

[snip]

 >The NULL doesn't seem to bother snprintf on Linux or HP-UX. I don't
 >know if it's valid to pass a NULL as an argument to "%s".

Hi,  I believe the behavior of passing NULL to %s is undefined.
NULL can be (but isn't necessarily) a macro expanding to
(void*)0 and since sprintf is a variadic function, one would
need an explicit cast to the appropriate type.

[snip]

Hope this helps

-- 
Sam Reynolds
cloud at chool.com

On Sat, Sep 21, 2002 at 01:20:19PM -0500, Sam Reynolds wrote:

 >> Hi,  I believe the behavior of passing NULL to %s is undefined.


 >dunno about that either way


 >> NULL can be (but isn't necessarily) a macro expanding to
 >> (void*)0


 >No, it isn't.  NULL is guaranteed to be a macro expanding to an
 >"unadorned 0".
I don't believe in C such a guarantee is given.
7.17.3 in C99 says:
"... NULL which expands to an implementation-defined null
pointer constant..."
6.23.2.3.3 explains that a null pointer constant is:
"An integer constant expression with the value 0, or such
an expression cast to type void *"

 >> and since sprintf is a variadic function, one would
 >> need an explicit cast to the appropriate type.


 >No, one doesn't.  A variadic function interprets pointers based on the
 >format string, not based on the type given to the compiler.

One does for two reasons. The first being a situation where my format
string is input from the user, clearly the compiler can't know what the
args are going to be at compile time. Second, if I write my own
variadic function the compiler will not know what the arguments to
my function are if I don't explicitly cast them (if they are void*).

 >/fc
Hope this helps,

-- 
Sam Reynolds
cloud at chool.com

On Sun, Sep 22, 2002 at 01:24:24AM +1000, Darren Tucker
wrote:
> After poking around, it seems that:
> 1) get_local_ipaddr returns NULL
> 2) this NULL is passed to snprintf
> 3) which dereferences the NULL causing a SEGV
> 
> (get_local_ipaddr returns NULL because it calls get_socket_address which
> calls getpeername on a non-socket.)
thanks.  fixed a little different and cover the other case.
the canohost interface needs to be reworked.

Index: canohost.c
==================================================================RCS file:
/cvs/src/usr.bin/ssh/canohost.c,v
retrieving revision 1.33
diff -u -r1.33 canohost.c
--- canohost.c	9 Jul 2002 11:56:27 -0000	1.33
+++ canohost.c	23 Sep 2002 20:16:38 -0000
@@ -196,18 +196,12 @@
 
 	if (remote) {
 		if (getpeername(socket, (struct sockaddr *)&addr, &addrlen)
-		    < 0) {
-			debug("get_socket_ipaddr: getpeername failed: %.100s",
-			    strerror(errno));
+		    < 0)
 			return NULL;
-		}
 	} else {
 		if (getsockname(socket, (struct sockaddr *)&addr, &addrlen)
-		    < 0) {
-			debug("get_socket_ipaddr: getsockname failed: %.100s",
-			    strerror(errno));
+		    < 0)
 			return NULL;
-		}
 	}
 	/* Get the address in ascii. */
 	if (getnameinfo((struct sockaddr *)&addr, addrlen, ntop, sizeof(ntop),
@@ -221,13 +215,21 @@
 char *
 get_peer_ipaddr(int socket)
 {
-	return get_socket_address(socket, 1, NI_NUMERICHOST);
+	char *p;
+
+	if ((p = get_socket_address(socket, 1, NI_NUMERICHOST)) != NULL)
+		return p;
+	return xstrdup("UNKNOWN");
 }
 
 char *
 get_local_ipaddr(int socket)
 {
-	return get_socket_address(socket, 0, NI_NUMERICHOST);
+	char *p;
+
+	if ((p = get_socket_address(socket, 0, NI_NUMERICHOST)) != NULL)
+		return p;
+	return xstrdup("UNKNOWN");
 }
 
 char *

On Sun, Sep 22, 2002 at 04:59:30PM -0500, Sam Reynolds wrote:

[snip]

 >>  >> and since sprintf is a variadic function, one would
 >>  >> need an explicit cast to the appropriate type.
 >>
 >>  >No, one doesn't.  A variadic function interprets pointers
based on
 >the
 >>  >format string, not based on the type given to the compiler.
 >>
 >> One does for two reasons. The first being a situation where my format
 >> string is input from the user, clearly the compiler can't know
what >the
 >> args are going to be at compile time.


 >In which case, how are you going to know what to cast to?
Oops, what I said there isn't true :)
What I intended to say was "The first being a situation where my format
string is input from the user, clearly the compiler can't know the
*format string* at compile time.

As for how you will cast, (you'll prolly have to read the format string)
but whatever you decide, you will still have decide for each of your
sprintf calls what they should be cast to, and when you actually call
the function, the decision will have been made, and it is based on
the types of the arguments passed in (be it by cast or not) that the
compiler knows how to deal with the function.


 >> Second, if I write my own
 >> variadic function the compiler will not know what the arguments to
 >> my function are if I don't explicitly cast them (if they are
void*).


 >huh?
Suppose I write a variadic function called addem().  It takes some
number of ints adds them, and returns their sum.  No format string
here, the compiler still has to know how to setup the function, so
it looks at the types of the arguments you pass in, and sets it up
from that.


 >/fc

Hope this helps

-- 
Sam Reynolds
cloud at chool.com