*** Bug 441 has been marked as a duplicate of this bug. ***

Created attachment 179 [details]
Original patch by Dan Astoorian

Created attachment 180 [details]
Ported Dan's patch to current tree

Ported to current. Moved option parsing code into parse_forward().

Created attachment 182 [details]
Implement optional address binding for port forwards

Re-added test for number of arguments to parse_forward.
Updated man page for ssh's -D option missed earlier.

i'll look into this.

Created attachment 217 [details]
Implement optional address binding for port forwards (update)

Updated based on some feedback from Markus:
* re-added incorrectly removed code
* simplified GatewayPorts/bindaddress handling
* reduced code duplication of parse_forward()
* use NULL or 127.0.0.1 instead of localhost

Created attachment 219 [details]
about attachment (id=217)

About attachment (id=217)

- ssh_config.5 should be updated too (options LocalForward and RemoteForward)
- we should set GatewayPorts to yes in ssh config to use new forward shema.

Created attachment:
- fix segfault, when we use LocalForward and RemoteForward in ssh config.
- make ssh option GatewayPorts obsolete (might this option is useless with new
forward shema?).
- correct forward messages in ssh.c

What about readconf methods:
  void add_local_forward(Options *, const char *, u_short, const char *,
u_short);
  void add_remote_forward(Options *, const char *, u_short, const char *,
u_short);
to be changed as follows:
  void add_local_forward(Options *, const Forward*);
  void add_remote_forward(Options *, const Forward*);
In first case developer should read C file to see agriment meaning. Second case
is more obvious.

no, GatewayPorts should still default to 'no', but with -L 
local:port:target:port the listen address should be overwritten.
without 'local:' we should still use GatewayPorts like before.

re attachment 219 [details]: 
1. please don't use strncat (use strlcpy or snprintf instead).
2. i don't think the patch to ssh.c is correct.

samples:
        LocalForward              10080 127.1.0.253:80
        LocalForward            *:10180 127.1.0.253:80
        LocalForward    127.1.0.1:10280 127.1.0.253:80
        LocalForward     10.0.0.1:10380 127.1.0.253:80

second line is equal to GatewayPorts=yes and -L port:target:port in unpached ssh.

GatewayPorts in server must exist, but in client hmm.
In patched version (patch 217+219) we can set forwards as above.
It is easy to use only LocalForward (as show above), instead both GatewayPorts
and LocalForward.

still we need to keep current -g behaviour for existing setups.

Created attachment 229 [details]
Implement optional address binding for port forwards (update 2)

* merge bug fixes from Rumen (RemoteForward and LocalForward options).
* replace strncat with snprintf as per Markus.	Note: used a static buffer.
* allow client to override GatewayPorts with a "*" listen address, also from
Markus.

Changing the arguments to add_local_forward() and add_remote_forward() will
mean more code to populate the structs, I don't think it's worth it. 

Markus, could you please elaborate on "i don't think the patch to ssh.c is
correct"?

Created attachment 321 [details]
Implement optional address binding for port forwards (update 3)

Updated to current tree.  Have compiled it but have done no further testing.

>  	if (compat20) {
> -		const char *address_to_bind = "0.0.0.0";
> +		const char *address_to_bind;
> +		if (listen_host == NULL)
> +			address_to_bind = "127.0.0.1";

maybe this should be "localhost" as the server will do name resolution on this
and we don't want to limit ourselves to ipv4. alternately we could send two
requests, one for INADDR_LOOPBACK, the other for IN6ADDR_LOOPBACK_INIT

Alternately, we could intercept hostnames of "localhost" and interpret these as
a request to bind to the result of getaddrinfo() with the AI_PASSIVE flag set.

*** Bug 876 has been marked as a duplicate of this bug. ***

*** Bug 876 has been marked as a duplicate of this bug. ***

Created attachment 666 [details]
Update to OpenBSD -current

This updates the patch to OpenBSD -current, so we can start work on getting 
this in. The issues stopping this from going in:

- This needs a regress test, including tests for backwards compat
- The manpage changes need to be redone (I haven't even tried to sync them)
- Resolve the 127.0.0.1 issue
- Lots of testing, including manual testing of the ~C commandline

This would be really good for 3.9, so let's get to it :)

Created attachment 782 [details]
forward-bind.sh: regression test for binding port forwards to addresses

Current limitations of test:
- no testing of IPv6
- no testing of backwards compat with <=3.9

(In reply to comment #18)
> - Resolve the 127.0.0.1 issue

I just reread the spec (draft-secsh-connect23) and it's not clear how to do that
in a conforming way.

Recapping, the issue is: "on the client, how do you do you specify a port
forward that listens on the IPv4 and IPv6 loopacks on the server (ie by passing
a hostname of NULL and hint of AI_PASSIVE to getaddrinfo)."

The spec currently says:
[quote]
            byte      SSH_MSG_GLOBAL_REQUEST
            string    "tcpip-forward"
            boolean   want reply
            string    address to bind (e.g., "0.0.0.0")
            uint32    port number to bind

   The 'address to bind' and 'port number to bind' specify the IP
   address and port to which the socket to be listened is bound.  The
   address should be "0.0.0.0" if connections are allowed from anywhere.
[/quote]

There was also a proposal from Niels Möller
(http://thread.gmane.org/gmane.ietf.secsh/2273) to use the string to "" to
specify listening on any address, although it appears that no consensus was reached.

As I read the spec, the options for the client currently are:
1) specify an address string of "127.0.0.1"
2) specify an address string of "localhost"

and optionally in the server
3) special-case a string of "localhost" to mean calling getaddrinfo with a host
of NULL and AI_PASSIVE in the hints.

1) is more likely to work since it doesn't rely on a nameservice, however 2)
will probably give you both IPv4 and IPv6 loopbacks if the host supports them.

Since 2) depends on the nameservice, however it seems that using it for this
introduces another point of attack (ie if the server doesn't have 'localhost' in
its hosts file, and an attacker spoofs 'localhost.example.com' to be the host's
external IP, other users may unknowingly be binding their port forwards to the
external address.

Because of this, I'm leaning towards option 1).  It's consistent with the
current spec of "0.0.0.0" for wildcard bind (ie it's equivalent to what you'd
see from netstat for IPv4 only).  Opinions sought.

Thinking about it, I wonder if the spec should say that a string of "" means
"bind to whatever loopbacks you have" and a string of "*" means "bind to every
address family you support".

Created attachment 783 [details]
forward-bind.sh: regression test for binding port forwards to addresses

Now with backwards compatibility testing and a free set of steak knives.

Created attachment 784 [details]
Update patch to OpenBSD -current

Update patch to -current (fixed trivial breakage on usage, otherwise
unchanged).

OK, here is a concrete proposal and a rationale:

I don't think the risks of sending "localhost" are too great; if a system is so
badly misconfigured to be susceptible to attack via this method, then it has far
greater problems than we can work around. Anyway, it isn't a common problem (see
below.)

Special-casing "0.0.0.0" and "127.0.0.1" seems ugly: if we do that they how can
you remotely specify an IPv4-only listener for any or loopback?

So, I think:

1. The server should special-case the empty string to be a wildcard (NULL,
AI_PASSIVE) bind
2. The server should special-case "localhost" to be a loopback (NULL, 0) bind
3. The client should send the empty string for wildcard binds
4. The client should send "localhost" for loopback binds

Some interoperabiliy considerations of this approach

a. For OpenSSH old client, OpenSSH new server

The client always sends "0.0.0.0", so this will break IPv6 for 
the gateway_ports case.

b. For OpenSSH new client, OpenSSH old server

The server doesn't care what is sent, so this will work (as far as it works now,
anyway).

c. For other client, OpenSSH new server

OpenSSH sshd will do what the client tells it to :) Potential problems as per
"a" above.

d. For OpenSSH new client, other server

This is the only case there Darren's concern over the sending of "localhost"
manifests, because OpenSSH hasn't (and won't) interpret the name.

We may break gateway_ports rforwards on other servers if they haven't
special-cased the empty string to mean a wildcard bind. An easy workaround is to
just specify a bind address manually

The regress tests should test SSHv1 connections too.

Comment on attachment 784 [details]
Update patch to OpenBSD -current

>Index: ssh.c
>===================================================================
>RCS file: /cvs/src/usr.bin/ssh/ssh.c,v
>retrieving revision 1.230
>diff -u -p -r1.230 ssh.c
>--- ssh.c	7 Nov 2004 17:57:30 -0000	1.230
>+++ ssh.c	24 Jan 2005 06:15:41 -0000

...

>@@ -845,14 +866,19 @@ ssh_init_forwarding(void)
> 
> 	/* Initiate remote TCP/IP port forwardings. */
> 	for (i = 0; i < options.num_remote_forwards; i++) {
>-		debug("Connections to remote port %d forwarded to local address %.200s:%d",
>-		    options.remote_forwards[i].port,
>-		    options.remote_forwards[i].host,
>-		    options.remote_forwards[i].host_port);
>+		listen_host = options.remote_forwards[i].listen_host;
>+		if (listen_host && listen_host[0] == '\0' && !options.gateway_ports)
>+			listen_host = NULL;

This check looks bogus: it is trying to apply the client's gateway_ports
setting to remote forward sent to the server. I think it should be removed,
right?

(In reply to comment #25)
> This check looks bogus: it is trying to apply the client's gateway_ports
> setting to remote forward sent to the server. I think it should be removed,
> right?

I think it's valid.  It seems intutive to do:
$ ssh -oGatewayPorts=yes -R 1234:foo.example.com:1234 server
and expect it to be a wildcard listen (server permitting, of course).

Using the client's GatewayPorts as the default for requests sent to the server
too seems sane.  It can always be overridden per-forward with something like -R
*:1234:foo.example.com:1234 or -R localhost:1234:foo.example.com:1234.

I don't like it because it controls for -L port forwards as well and we don't
want to encourage people to put "GatewayPorts yes" at the top of their
ssh_config just to change the remote default.

It also means there's no way to turn it on per-host if that's what you really want.

I don't mind much either way, as long as the documentation is clear (and you
field any "GatewayPorts doesn't work with remote forwards" bug reports :-)

(In reply to comment #28)
> It also means there's no way to turn it on per-host if that's what you really
want.

I think your can:

$ ssh -R:8000:localhost:80 foo
$ ssh -R*:8000:localhost:80 foo
-> enables wildcard listen on the server (modulo the server's gateway_ports setting)

$ ssh -R 8000:localhost:80 foo 
-> defaults to localhost listen on the server

Am I missing something?

Created attachment 791 [details]
Latest version

Latest version. Incorporates suggested special-case address_to_bind strings,
adds KNF and fixes a few bugs.

(i'll get interdiff set up on bugzilla soon so it will be possible to see
changes between these diffs)

(In reply to comment #29)
> $ ssh -R 8000:localhost:80 foo 
> -> defaults to localhost listen on the server

Yeah, but right now that defaults to a wildcard listen on the server, modulo
GatewayPorts on the server.  (Whether or not this is a good idea is another
matter, but that's what it does right now.)  Checking the client's GatewayPorts
would be one way of maintaining the current behavior if that's what they really
wanted.

Maybe GatewayPorts on the client could have another couple of options?
GatewayPorts yes | no | clientonly | serveronly

(In reply to comment #31)

I don't think we need a new option for this. We can either:

1. Retain the current behaviour of rforwards to a server with gatewayports=yes
being wildcard by default.

2. Change the behaviour and document it.

I like (2) because we have the opportunity to present a safer default, but we
wouldn't be going backwards if we did (1).

(In reply to comment #32)
> I like (2) because we have the opportunity to present a safer default, but we
> wouldn't be going backwards if we did (1).

Except that there's more incentive to enable GatewayPorts=yes on the server
since per-interface binds are potentially more useful.

I dunno.  "Secure by default" when in doubt?

Created attachment 792 [details]
Man page updates

nroff is pain.

Created attachment 806 [details]
Latest diff

This is the current version of the diff, including a couple of small changes
suggested by dtucker@

Created attachment 834 [details]
Revised patch

Revise the patch again, based on comments from markus@

Among a few other tweaks, this patch adds manpage descriptions for the extra
arguments for -L and LocalForward. It also special-cases the bind_address
"0.0.0.0" for older OpenSSH versions, which will retain the current behaviour
for the old client + new server case.

Patch applied. Thanks to all who helped.

With the release of OpenSSH 4.0, these bugs are now closed. For details, see:
http://www.openssh.com/txt/release-4.0