| Summary: | channels.c:3258: suspicious code ? | ||
|---|---|---|---|
| Product: | Portable OpenSSH | Reporter: | David Binderman <dcb314> |
| Component: | Miscellaneous | Assignee: | Assigned to nobody <unassigned-bugs> |
| Status: | CLOSED FIXED | ||
| Severity: | minor | CC: | djm, jjelen |
| Priority: | P5 | ||
| Version: | 7.6p1 | ||
| Hardware: | Other | ||
| OS: | Linux | ||
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)
(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. Close all resolved bugs after release of OpenSSH 7.7. 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 The special cases are already documented in the comment above the function, but I'll add another comment. close bugs that were resolved in OpenSSH 8.5 release cycle |
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; }