openssh bugs - Dec 2017 - [Bug 2804] New: channels.c:3258: suspicious code ?

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

            Bug ID: 2804
           Summary: channels.c:3258: suspicious code ?
           Product: Portable OpenSSH
           Version: 7.6p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: minor
          Priority: P5
         Component: Miscellaneous
          Assignee: unassigned-bugs at mindrot.org
          Reporter: dcb314 at hotmail.com

channels.c:3258]: (warning) The comparison operator in
'strcmp(listen_addr,"localhost") != 0' should maybe be
'==' instead,
currently the expression 'strcmp(listen_addr,"127.0.0.1") ==
0' is
redundant.

Source code is

        } else if (strcmp(listen_addr, "localhost") != 0 ||
            strcmp(listen_addr, "127.0.0.1") == 0 ||
            strcmp(listen_addr, "::1") == 0) {
            /* Accept localhost address when GatewayPorts=yes */
            addr = listen_addr;
        }

Maybe better code

        } else if (strcmp(listen_addr, "localhost") == 0 ||
            strcmp(listen_addr, "127.0.0.1") == 0 ||
            strcmp(listen_addr, "::1") == 0) {
            /* Accept localhost address when GatewayPorts=yes */
            addr = listen_addr;
        }

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

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
         Resolution|---                         |WORKSFORME
             Status|NEW                         |RESOLVED

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
AFAIK this is actually correct and support the case of "localhost"
requesting both IPv4 and IPv6 loopback addresses as well as
GatewayPorts=clientspecified

First, this code is only reachable for GatewayPorts=clientspecified or
we're on the client side (where the client gets to specify the listen
address always). Here we want to allow arbitrary listen addresses, but
"localhost" needs special-casing because it should yield listeners
that
respond to both IPv4 and IPv6 connections.

So:

1) if listen_addr=="::1" or "127.0.0.1" we accept it as addr

2) If listen_addr=="localhost" we leave addr=NULL and *wildcardp is
set
to 0.

3) If listen_addr is anything else, then we accept it as addr.

Later, addr/wildcard is used like this:

3294    /* Determine the bind address, cf. channel_fwd_bind_addr()
comment */
3295    addr = channel_fwd_bind_addr(fwd->listen_host, &wildcard,
3296        is_client, fwd_opts);
3297    debug3("%s: type %d wildcard %d addr %s", __func__,
3298        type, wildcard, (addr == NULL) ? "NULL" : addr);
3299  
3300    /*
3301     * getaddrinfo returns a loopback address if the hostname is
3302     * set to NULL and hints.ai_flags is not AI_PASSIVE
3303     */
3304    memset(&hints, 0, sizeof(hints));
3305    hints.ai_family = ssh->chanctxt->IPv4or6;
3306    hints.ai_flags = wildcard ? AI_PASSIVE : 0;
3307    hints.ai_socktype = SOCK_STREAM;
3308    snprintf(strport, sizeof strport, "%d", fwd->listen_port);
3309    if ((r = getaddrinfo(addr, strport, &hints, &aitop)) != 0) {

so listen_addr == localhost yields getaddrinfo(NULL,...) that give us
sockets for both IPv4 and IPv6

(yes, this is confusing)

-- 
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=2804

--- Comment #2 from David Binderman <dcb314 at hotmail.com> ---
(In reply to Damien Miller from comment #1)
> AFAIK this is actually correct
Righto.
> (yes, this is confusing)
So it could do with simplifying.

The code I mentioned does this kind of thing:

  else if (x != A 
    || x == B
    || x == C)

That can be simplified downto

  else if (x != A)

with no loss of functionality.

-- 
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=2804

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after release of OpenSSH 7.7.

-- 
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=2804

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |REOPENED
         Resolution|WORKSFORME                  |---
                 CC|                            |jjelen at redhat.com

--- Comment #4 from Jakub Jelen <jjelen at redhat.com> ---
Damien,
could we simplify this code as explained in the comment #2 and possibly
add a comment so it is clear for future readers?

This popped up in results of our static analyzers scanner as a
suspicious code again.

https://github.com/openssh/openssh-portable/blob/master/channels.c#L3356

-- 
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=2804

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
The special cases are already documented in the comment above the
function, but I'll add another comment.

-- 
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=2804

Damien Miller <djm at mindrot.org> changed:

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

-- 
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=2804

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
close bugs that were resolved in OpenSSH 8.5 release cycle

-- 
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.