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.
Patch applies to 6.8p1 as well (just tested).
Created attachment 2585 [details] OSX X11: if using launchd socket, remove the screen number patch added for commenting. I think we can make this less invasive, though.
Any chance we can get this into 7.0?
Move unfinished bugs from 6.9 (how did I miss these?) to 7.1
Retarget to openssh-7.3
retarget unfinished bugs to next release
Could we please ressolve this? It's been >1.5 years now...
Updated patch: https://github.com/openssh/openssh-portable/compare/master...jschlyter:osx, slightly less changes.
Created attachment 2871 [details] 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.
Comment on attachment 2871 [details] 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.
Comment on attachment 2871 [details] 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)
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
Almost 2 years now, still not fixed. What's holding this back?
Created attachment 2915 [details] 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?
Comment on attachment 2915 [details] 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?
Created attachment 2916 [details] 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.
Comment on attachment 2916 [details] OSX X11: if using launchd socket, remove the screen number >+ if (stat(path, &sbuf) == 0) { >+ return 1; >+ } hemi-demi-nitpick: remove braces
Created attachment 2917 [details] OSX X11: if using launchd socket, remove the screen number (In reply to Damien Miller from comment #21) > hemi-demi-nitpick: remove braces done.
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.
for the next respin of the patch: also put the new function behind #ifdef APPLE, otherwise it causes "unused function" warnings on everything else.
(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
Any updates? Anything I should do?
Patch applied, thanks for your patience.
closing resolved bugs as of 8.6p1 release