Created attachment 2761 [details] patch On Illumos/Solaris we can drop fine-grained privileges using setppriv, both for the sshd sandbox and also where appropriate in other utilities like sftp-server and ssh-agent. This has a lot of cross-over with work to add pledge(2) calls to OpenSSH code. Entering this bug against sshd, since the sandbox component of this is almost certainly the most important from a security perspective. Discussed on mailinglist (openssh-unix-dev) thread on 12 Nov 2015. Attached patch was against openssh-portable at 3ddd15e (Darren Tucker: Add a null implementation of pledge.)
Comment on attachment 2761 [details] patch >--- a/configure.ac >+++ b/configure.ac ... >+ AC_ARG_WITH([solaris-privs], >+ [ --with-solaris-privs Enable Solaris/Illumos privileges (experimental)], >+ [ >+ AC_CHECK_FUNC([setppriv], >+ [ AC_CHECK_HEADERS([priv.h]) Should the following two AC_DEFINEs be conditional on priv.h being found? >+ AC_DEFINE([NO_UID_RESTORATION_TEST], [1], >+ [Define to disable UID restoration test]) >+ AC_DEFINE([USE_SOLARIS_PRIVS], [1], >+ [Define if you have Solaris privileges]) >+ SP_MSG="yes" ], ) SP_MSG is for "Solaris project support" - did you intend to provide a message in the configure summary section? If so, you should use a different variable. >+elif test "x$sandbox_arg" = "xsolaris" || \ >+ ( test -z "$sandbox_arg" && test "x$ac_cv_func_setppriv" = "xyes" ) ; then >+ test "x$ac_cv_func_setppriv" != "xyes" && \ Rather than repeating the tests in the above block, it's probably more robust to set a shell variable there and test it here. E.g. elif test "x$sandbox_arg" = "xsolaris" || test "x$SOLARIS_PRIVS" = "xyes" >--- a/openbsd-compat/port-solaris.c >+++ b/openbsd-compat/port-solaris.c >+void >+solaris_drop_fork_privs(void) >+{ >+ priv_set_t *pset = NULL; >+ >+ if ((pset = priv_allocset()) == NULL) >+ fatal("priv_allocset: %s", strerror(errno)); >+ >+ /* Start with "basic" and drop everything we don't need. */ >+ priv_basicset(pset); >+ >+ priv_delset(pset, PRIV_PROC_EXEC); >+ priv_delset(pset, PRIV_PROC_FORK); >+ priv_delset(pset, PRIV_FILE_LINK_ANY); >+ priv_delset(pset, PRIV_PROC_INFO); >+ priv_delset(pset, PRIV_PROC_SESSION); These calls should be checked for failure. >+ if (setppriv(PRIV_SET, PRIV_PERMITTED, pset)) >+ fatal("setppriv: %s", strerror(errno)); >+ if (setppriv(PRIV_SET, PRIV_LIMIT, pset)) >+ fatal("setppriv: %s", strerror(errno)); >+ if (setppriv(PRIV_SET, PRIV_INHERITABLE, pset)) >+ fatal("setppriv: %s", strerror(errno)); Coalesce these calls? I.e. if (setppriv(PRIV_SET, PRIV_PERMITTED, pset) != 0 || setppriv(PRIV_SET, PRIV_LIMIT, pset) != 0 || setppriv(PRIV_SET, PRIV_INHERITABLE, pset) != 0) fatal("setppriv: %s", strerror(errno)); same for solaris_drop_fork_net_privs() and the sandbox.
put this on the list for openssh-7.2
oh, and you'll need to add sandbox-solaris.c to SSHDOBJS in Makefile.in too
Created attachment 2770 [details] patch-v2 Re: the missing sandbox-solaris.o in Makefile.in, and the re-use of SP_MSG, those were silly mistakes because I didn't copy that fix across from my build machine after I spotted it there. My apologies. I've attached a new patch (against current git master 39736be) with these issues fixed. I've condensed the checks for setppriv and priv.h to set a $SOLARIS_PRIVS shell variable as you suggested, which is then re-used by the two checks. The ifs for setppriv, and the new ifs introduced for priv_delset have been condensed into || as you suggested. priv_delset can only fail if the argument given to it is invalid, but extra checks can never hurt. I have also moved the calls to platform_drop_x_privs() to be colocated with the new pledge() calls where possible, and noted in the comments above them (in platform.c) that they should match the pledge() they sit next to as much as possible. I did have one other question/comment -- from what I can tell, the pledge() call in ssh-agent seems to be broader than it needs to be: it's currently allowing "exec", but the pledge() call happens after the final exec() that the ssh-agent can do. Am I mistaken on this? If I am, then the code in this patch should also avoid dropping "exec" (currently it drops it). (Oh, and this patch is definitely identical to the one on my build/test machine this time...)
(In reply to Alex Wilson from comment #4) > I did have one other question/comment -- from what I can tell, the > pledge() call in ssh-agent seems to be broader than it needs to be: > it's currently allowing "exec", but the pledge() call happens after > the final exec() that the ssh-agent can do. Am I mistaken on this? > If I am, then the code in this patch should also avoid dropping > "exec" (currently it drops it). Unfortunately the agent can still exec() at this point: if the user adds a PKCS#11 token then ssh-pkcs11-helper will be executed. I've thought about doing this unconditionally when ssh-agent is started, but was put off by running an extra process that will never be used by 99.9% of users. Alternatives including adding a command-line option to ssh-agent to enable PKCS#11 support (would break existing setups), adding an option to disable PKCS#11 support so a stronger pledge could be used (not "secure by default", nobody would use it) or extending pledge to permit the whitelisting of execl() targets (not going to happen for a while, if ever). None of the alternatives were particularly appealing, so we punted and used a more permissive pledge policy :/
Comment on attachment 2770 [details] patch-v2 This looks okay to me. Darren, can you take a second look?
Created attachment 2771 [details] patch-v3 (In reply to Damien Miller from comment #5) > > Unfortunately the agent can still exec() at this point: if the user > adds a PKCS#11 token then ssh-pkcs11-helper will be executed. > Ah. Of course. I haven't been testing with a pkcs#11 token, though we do support a few of them on Illumos, so perhaps I should see if I can dig one up for future testing. I have attached a v3 patch, with this fixed up so that the ssh-agent retains the right to use exec(). I also renamed the solaris_drop_*_privs() functions to make it a bit clearer what the 3 of them actually are.
Created attachment 2772 [details] patch-v4 One last amendment, after a colleague reminded me of a fix that I should have merged into this patch. It fixes the case where a user (for some reason) decides they want to let sftp-server log in as root and they wish to have root's ability to read and write any file on the system. Privilege code that starts with priv_basicset() implicitly drops all of root's special rights (including these "DAC" filesystem rights), so this amendment changes the sftp-server to explicitly retain those particular parts of root (if it has them) while still dropping everything else. As I understand it, the other places this patch injects priv drops (for the ssh-agent, client mux and daemon sandbox) are fine with dropping all special root abilities if they are started with any of them, so those functions don't need to change.
Comment on attachment 2772 [details] patch-v4 Darren, can you take a final look at this? It looks OK to me.
Comment on attachment 2772 [details] patch-v4 LGTM. Ship it.
hmm, that latest patch doesn't build: gcc -o ssh ssh.o readconf.o clientloop.o sshtty.o sshconnect.o sshconnect1.o sshconnect2.o mux.o roaming_common.o roaming_client.o -L. -Lopenbsd-compat/ -Wl,-z,relro -Wl,-z,now -Wl,-z,noexecstack -fstack-protector-all -pie -lssh -lopenbsd-compat -lselinux -lcrypto -ldl -lutil -lz -lcrypt -lresolv -Wl,-Bsymbolic-functions -Wl,-z,relro -lgssapi_krb5 -lkrb5 -lk5crypto -lcom_err mux.o: In function `mux_client_request_stdio_fwd': /usr/local/google/home/djm/cvs/openssh/mux.c:2005: undefined reference to `platform_drop_mux_privs' mux.o: In function `mux_client_request_session': /usr/local/google/home/djm/cvs/openssh/mux.c:1856: undefined reference to `platform_drop_mux_privs' It looks like we need to add platform.o to LIBSSH_OBJS in Makefile.in. I guess this is okay - so far that file has been sshd-only, but it doesn't look too big. What do you think, Darren?
actually, it would pull in a bit of stuff - it calls into the PAM code.
(In reply to Damien Miller from comment #12) > actually, it would pull in a bit of stuff - it calls into the PAM > code. maybe move it to platform-pledge.c or platform-privdrop.c or something? Adding all those extra link-time dependencies sounds like a future headache.
Created attachment 2774 [details] patch-v5 v5 uploaded. Moved and renamed functions into platform-pledge.c, libssh now only links in this .o instead of platform.o. Also updated comment after patch to ssh-agent pledge by doug@
Comment on attachment 2774 [details] patch-v5 Looks ok to me and has the advantage of building :) Darren, want to give it a final OK? (Mostly for the idea of platform-pledge.c, since you suggested it)
Committed - this will be in OpenSSH 7.2. Thanks for your patience :) commit 4626cbaf78767fc8e9c86dd04785386c59ae0839 Author: Damien Miller <djm@mindrot.org> Date: Fri Jan 8 14:24:56 2016 +1100 Support Illumos/Solaris fine-grained privileges Includes a pre-auth privsep sandbox and several pledge() emulations. bz#2511, patch by Alex Wilson. ok dtucker@
*** Bug 2299 has been marked as a duplicate of this bug. ***
Close all resolved bugs after 7.3p1 release