openssh bugs - Jan 2015 - [Bug 2341] New: XQuartz X11 forwarding not working in OS X 10.10 Yosemite

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

            Bug ID: 2341
           Summary: XQuartz X11 forwarding not working in OS X 10.10
                    Yosemite
           Product: Portable OpenSSH
           Version: 6.7p1
          Hardware: All
                OS: Mac OS X
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jakob at kirei.se

bug report in: http://xquartz.macosforge.org/trac/ticket/1451
patch in
https://trac.macports.org/export/121205/trunk/dports/net/openssh/files/launchd.patch

I've tested the patch and it works.

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

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

Jakob Schlyter <jakob at kirei.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |low-hanging-fruit

--- Comment #1 from Jakob Schlyter <jakob at kirei.se> ---
Patch applies to 6.8p1 as well (just tested).

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

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

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

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

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2585
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2585&action=edit
OSX X11: if using launchd socket, remove the screen number

patch added for commenting.  I think we can make this less invasive,
though.

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2360

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

--- Comment #3 from Jakob Schlyter <jakob at kirei.se> ---
Any chance we can get this into 7.0?

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

Jakob Schlyter <jakob at kirei.se> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakob at kirei.se

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2443

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Move unfinished bugs from 6.9 (how did I miss these?) to 7.1

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2451


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2451
[Bug 2451] Bugs intended to be fixed in 7.2
-- 
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=2341

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2443                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2443
[Bug 2443] Bugs intended to be fixed for OpenSSH 7.1
-- 
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=2341

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Retarget to openssh-7.3

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2543


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 release
-- 
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=2341

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2451                        |

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
Retarget to openssh-7.3


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2451
[Bug 2451] Bugs intended to be fixed in 7.2
-- 
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=2341

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2594

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next release


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
-- 
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=2341

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next 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.

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

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
retarget unfinished bugs to next 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.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2543                        |


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2543
[Bug 2543] Tracking bug for OpenSSH 7.3 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.

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

--- Comment #11 from Jakob Schlyter <jakob at kirei.se> ---
Could we please ressolve this? It's been >1.5 years now...

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

--- Comment #12 from Jakob Schlyter <jakob at kirei.se> ---
Updated patch:
https://github.com/openssh/openssh-portable/compare/master...jschlyter:osx,
slightly less changes.

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2585|0                           |1
        is obsolete|                            |
                 CC|                            |djm at mindrot.org
   Attachment #2871|                            |ok?(djm at mindrot.org)
              Flags|                            |

--- Comment #13 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2871
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2871&action=edit
OSX X11: if using launchd socket, remove the screen number

Imported from
https://github.com/openssh/openssh-portable/compare/master...jschlyter:osx

This diff is a lot more palatable since it does not change the mainline
code and thus does not represent an increased maintenance burden.

What's the provenance of this diff?  I'd like to make sure it is
correctly attributed.

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

--- Comment #14 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2871
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2871
OSX X11: if using launchd socket, remove the screen number

>+		if (0 == stat(path, &sbuf)) {
this is counter to the usual style (same below).
>+					is_path_to_socket=1;
needs spaces

We should fix those before committing, other than that, looks
reasonable.

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

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2871
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2871
OSX X11: if using launchd socket, remove the screen number

IMO this might be better off as a separate function. Also a couple of
nits:
> strlcpy(path, display, sizeof(path))
should check for truncation

The tests like (0 == stat(path, &sbuf)) are a bit unidiomatically
backwards-ordered
> +			if (dot) {
style(9) says (dot != NULL)

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

--- Comment #16 from Jakob Schlyter <jakob at kirei.se> ---
I've made is_path_to_xsocket() a separate function now. This code
originated from Apple (ref:
https://opensource.apple.com/source/OpenSSH/OpenSSH-195.40.1/openssh/channels.c).

https://github.com/openssh/openssh-portable/compare/master...jschlyter:osx

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

--- Comment #17 from Jakob Schlyter <jakob at kirei.se> ---
Almost 2 years now, still not fixed. What's holding this back?

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2871|ok?(djm at mindrot.org)        |ok+
              Flags|                            |

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2871|0                           |1
        is obsolete|                            |
   Attachment #2915|                            |ok?(djm at mindrot.org)
              Flags|                            |

--- Comment #18 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2915
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2915&action=edit
OSX X11: if using launchd socket, remove the screen number

(In reply to Jakob Schlyter from comment #17)
> Almost 2 years now, still not fixed. What's holding this back?
Well, there was no patch that had the issues addressed and had been
tested.  This takes your last patch and fixes djm's comment#15.  Could
you please try it?

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2915|ok?(djm at mindrot.org)        |ok+
              Flags|                            |

--- Comment #19 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2915
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2915
OSX X11: if using launchd socket, remove the screen number

This is ok, my comments are just nitpicks:
>+	if (stat(path, &sbuf) == 0) {
>+		return 1;
no need to wrap the remainder in and else block after return here.
>+	} else {
>+		char *dot = strrchr(path, '.');
I'd stick "char *dot" at the start of the function and do
"if ((dot = strrchr(path, '.')) != NULL {"
on one line, but that's nitpicking :)

> #ifdef __APPLE__
>-	if (strncmp(display, "/tmp/launch", 11) == 0) {
>-		sock = connect_local_xsocket_path(display);
>-		if (sock < 0)
>-			return -1;
>+	/* Check if display is a path to a socket (as set by launchd). */
>+	{
>+		char path[PATH_MAX];
> 
>-		/* OK, we now have a connection to the display. */
>-		return sock;
>+		if (is_path_to_xsocket(display, path, sizeof(path))) {
>+			debug("x11_connect_display: $DISPLAY is launchd");
>+
>+			/* Create a socket. */
>+			sock = connect_local_xsocket_path(path);
>+			if (sock < 0)
>+				return -1;
>+
>+			/* OK, we now have a connection to the display. */
>+			return sock;
also nitpicking: maybe we should move this whole lot into a separate
function?

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

--- Comment #20 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2916
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2916&action=edit
OSX X11: if using launchd socket, remove the screen number

(In reply to Damien Miller from comment #19)
> no need to wrap the remainder in and else block after return here.
done
> I'd stick "char *dot" at the start of the function and do
> "if ((dot = strrchr(path, '.')) != NULL {"
> on one line, but that's nitpicking :)
done

[...]
> also nitpicking: maybe we should move this whole lot into a separate
> function?
left this one for now.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2916|                            |ok+
              Flags|                            |

--- Comment #21 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2916
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2916
OSX X11: if using launchd socket, remove the screen number
>+	if (stat(path, &sbuf) == 0) {
>+		return 1;
>+	}
hemi-demi-nitpick: remove braces

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2915|0                           |1
        is obsolete|                            |

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2916|0                           |1
        is obsolete|                            |

--- Comment #22 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2917
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2917&action=edit
OSX X11: if using launchd socket, remove the screen number

(In reply to Damien Miller from comment #21)
> hemi-demi-nitpick: remove braces
done.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2917|                            |ok+
              Flags|                            |

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

--- Comment #23 from Darren Tucker <dtucker at zip.com.au> ---
Comment from Ron Frederick on openssh-unix-dev@
(https://lists.mindrot.org/pipermail/openssh-unix-dev/2016-December/035584.html):

"""
Looking at this patch, it seems to me that it introduces a possible
exploit. The new code calls stat() on whatever string is set as the
display value, even before checking for display values that are meant
to refer to remote network hosts. If ???ssh??? is run in a directory
which happens to have a file/pipe/socket named to match one of those
network display values, this new code would return that it should
connect to this local socket rather than the remote host when doing the
forwarding.

While checking for ???/tmp/launch??? as a prefix is a problem now that
MacOS is putting these local sockets in paths starting with
???/private/tmp/com.apple.launchd???, I think this new code should at a
minimum require that the path start with a leading ???/??? before
treating it as a local socket and doing a stat() on it.
"""

Sorry but this is now too late for 7.4.

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2594, 2360                  |2647


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2360
[Bug 2360] Bugs intended to be fixed in 6.9
https://bugzilla.mindrot.org/show_bug.cgi?id=2594
[Bug 2594] Tracking bug for OpenSSH 7.4 release
https://bugzilla.mindrot.org/show_bug.cgi?id=2647
[Bug 2647] Tracking bug for OpenSSH 7.5 release
-- 
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=2341

--- Comment #24 from Darren Tucker <dtucker at zip.com.au> ---
for the next respin of the patch: also put the new function behind
#ifdef APPLE, otherwise it causes "unused function" warnings on
everything else.

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

--- Comment #25 from Jakob Schlyter <jakob at kirei.se> ---
(In reply to Darren Tucker from comment #24)
> for the next respin of the patch: also put the new function behind
> #ifdef APPLE, otherwise it causes "unused function" warnings on
> everything else.
https://github.com/jschlyter/openssh-portable/commit/3100451bf5a0d2a23a0f927f603ddf0172a43763

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

--- Comment #26 from Jakob Schlyter <jakob at kirei.se> ---
Any updates? Anything I should do?

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

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

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

--- Comment #27 from Darren Tucker <dtucker at zip.com.au> ---
Patch applied, thanks for your patience.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #28 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.