openssh bugs - Nov 2016 - [Bug 2636] New: Fix X11 forwarding, when ::1 is not configured

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

            Bug ID: 2636
           Summary: Fix X11 forwarding, when ::1 is not configured
           Product: Portable OpenSSH
           Version: 7.3p1
          Hardware: Sparc
                OS: Solaris
            Status: NEW
          Severity: minor
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: tomas.kuthan at oracle.com

When IPv6 loopback is disabled on a host, X11 forwarding fails.

        tomas at tkuthan-cz:~$ ipadm show-addr lo0
        ADDROBJ           TYPE     STATE        ADDR
        lo0/v4            static   ok           127.0.0.1/8
        tomas at tkuthan-cz:~$ ssh -XY localhost xterm
        X11 forwarding request failed on channel 0
        xterm: Xt error: Can't open display: 
        xterm: DISPLAY is not set
        tomas at tkuthan-cz:~$

Syslog shows the following error:

Nov  9 15:36:39 tkuthan-cz sshd[5046]: [ID 800047 auth.error] error:
Failed to allocate internet-domain X11 display socket

This is caused by bind() failing with EADDRNOTAVAIL on ::1 in
x11_create_display_inet.

When this particular error is returned by bind, it is safe to continue
with the next address returned by getaddrinfo(), because in that case
there is no risk of forwarded X11 connections being hijacked
(CVE-2008-1483).

Binding to 127.0.0.1 succeeds and makes X11 forwarding work again.

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

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

Tomas Kuthan <tomas.kuthan at oracle.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tomas.kuthan at oracle.com

--- Comment #1 from Tomas Kuthan <tomas.kuthan at oracle.com> ---
Created attachment 2888
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2888&action=edit
When bind returns AEDDRNOTAVAIL, continue with next address

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

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

--- Comment #2 from Tomas Kuthan <tomas.kuthan at oracle.com> ---
To reproduce - remove IPv6 loopback interface:

ipadm delete-addr lo0/v6

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

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

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #3 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Tomas Kuthan from comment #0)
[...]
> When this particular error is returned by bind, it is safe to
> continue with the next address returned by getaddrinfo(), because in
> that case there is no risk of forwarded X11 connections being
> hijacked (CVE-2008-1483).
No, there is still a risk, eg if the IPv6 address loopback is added
after a connection is made.

getaddrinfo w/AI_PASSIVE should not return non-existent addresses. 
Quoting RFC3493:

   If the AI_PASSIVE flag is specified, the returned address
information
   shall be suitable for use in binding a socket for accepting incoming
   connections for the specified service (i.e., a call to bind()).

In this case the returned address is not suitable to bind because it'll
never work (unless you race bring up the interface).

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

--- Comment #4 from Tomas Kuthan <tomas.kuthan at oracle.com> ---
(In reply to Darren Tucker from comment #3)
...
> getaddrinfo w/AI_PASSIVE should not return non-existent addresses. 
> Quoting RFC3493:
> 
>    If the AI_PASSIVE flag is specified, the returned address
> information
>    shall be suitable for use in binding a socket for accepting
> incoming
>    connections for the specified service (i.e., a call to bind()).
> 
> In this case the returned address is not suitable to bind because
> it'll never work (unless you race bring up the interface).
Hi Darren,

I don't think RFC 3493 says, that bind() on address returned by
getaddrinfo() has to succeed. I think it specifies what address should
be returned for NULL hostname based on AI_PASSIVE flag.

   If the AI_PASSIVE flag is specified, the returned address
information
   shall be suitable for use in binding a socket for accepting incoming
   connections for the specified service (i.e., a call to bind()).  In
   this case, if the nodename argument is null, then the IP address
   portion of the socket address structure shall be set to INADDR_ANY
   for an IPv4 address or IN6ADDR_ANY_INIT for an IPv6 address.  If the
   AI_PASSIVE flag is not specified, the returned address information
   shall be suitable for a call to connect() (for a connection-mode
   protocol) or for a call to connect(), sendto() or sendmsg() (for a
   connectionless protocol).  In this case, if the nodename argument is
   null, then the IP address portion of the socket address structure
   shall be set to the loopback address.  This flag is ignored if the
   nodename argument is not null.

But I agree, the possibility of an IPv6 interface added after the IPv4
port was bound is troubling. In that case some user could hijack the
traffic.

I don't think it is directly exploitable; adding an IP interface can
only be done by a privileged user, who already has all sorts of powers.
But still, there is a a theoretical chance that an unprivileged
attacker could exploit a condition that they did not cause.

If we choose not to fix it this way, we should at least set
hints.ai_family = options.address_family, so that users can make X11
forwarding work w/o IPv6 interface by setting AddressFamily=inet.

Would you accept a patch for that?

Thanks,
Tomas

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

--- Comment #5 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Tomas Kuthan from comment #4)
[...]
> If we choose not to fix it this way, we should at least set
> hints.ai_family = options.address_family, so that users can make X11
> forwarding work w/o IPv6 interface by setting AddressFamily=inet.
> 
> Would you accept a patch for that?
I thought that happened already.   sshd calls

  channel_set_af(options.address_family);

fairly early, which stores it in channel.c's "static int IPv4or6",
which gets passed to getaddrinfo via hints.ai_family.  Does that not
work?

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

Tomas Kuthan <tomas.kuthan at oracle.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|NEW                         |RESOLVED

--- Comment #6 from Tomas Kuthan <tomas.kuthan at oracle.com> ---
(In reply to Darren Tucker from comment #5)
...
> I thought that happened already.   sshd calls
> 
>   channel_set_af(options.address_family);
> 
> fairly early, which stores it in channel.c's "static int
IPv4or6",
> which gets passed to getaddrinfo via hints.ai_family.  Does that not
> work?
You are right, it does work. I must have done something wrong when
testing that earlier, my bad.

In that case there is nothing to be done, I am closing the bug. Sorry
for the noise.

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

Damien Miller <djm at mindrot.org> changed:

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

Parke <parke.nexus at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |parke.nexus at gmail.com
           See Also|                            |https://bugzilla.mindrot.or
                   |                            |g/show_bug.cgi?id=2143

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

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