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.
Created attachment 1367 [details] fix for the bug
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.
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.
Target 5.1 release.
Created attachment 1539 [details] 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.
Created attachment 1540 [details] revised revised patch my last patch has a bug: listen_host may be NULL in channel_request_remote_forwarding()
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.
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.
Could you refresh my memory as to why NI_MAXHOST is inappropriate?
(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.
Created attachment 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.
(In reply to comment #11) > Created an attachment (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
patch applied - this will be in openssh-5.2
Close bugs fixed/reviewed for openssh-5.2 release