On Wed, 17 Feb 2016, Alex Wilson wrote:> On 2/17/16 2:04 PM, Alex Wilson wrote: > > I've attached a patch... > > > > Also at > > https://us-east.manta.joyent.com/arekinath/public/openssh-wip-fix-for-sol10-privs.patch > > If you are having trouble getting the patch out of the email. > > Also, as for Damien's patch, you will want to regenerate configure etc. > This doesn't go on top of his suggestion (I don't want to just disable > this new code for Solaris 10 if possible, it would be better to fix it > up so that you can have sandbox support as well)I don't have any Solaris systems around to test, so I'm pretty much flying blind. I'd love to be able to ship something that works for both 10 and 11 though, so if you are both able to come up with a patch including the autoconf gunk that works then I'll get it committed. -d
On Thu, Feb 18, 2016 at 9:31 AM, Damien Miller <djm at mindrot.org> wrote:> On Wed, 17 Feb 2016, Alex Wilson wrote: > >> On 2/17/16 2:04 PM, Alex Wilson wrote: >> > I've attached a patch... >> > >> >> Also at >> >> https://us-east.manta.joyent.com/arekinath/public/openssh-wip-fix-for-sol10-privs.patch >> >> If you are having trouble getting the patch out of the email. >> >> Also, as for Damien's patch, you will want to regenerate configure etc. >> This doesn't go on top of his suggestion (I don't want to just disable >> this new code for Solaris 10 if possible, it would be better to fix it >> up so that you can have sandbox support as well) > > I don't have any Solaris systems around to test, so I'm pretty much flying > blind. I'd love to be able to ship something that works for both 10 and 11 > though, so if you are both able to come up with a patch including the > autoconf gunk that works then I'll get it committed.I do have a SPARC I can blow the dust off and test with so I'll take a look at this one. After a quick look at the patch, I'm wondering if it could be simplified by adding an implementation of priv_basicset inside #ifndef HAVE_PRIV_BASICSET and doing away with the inline ifdefs? -- Darren Tucker (dtucker at zip.com.au) GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69 Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
On Thu, Feb 18, 2016 at 10:22 AM, Darren Tucker <dtucker at zip.com.au> wrote: [...]> After a quick look at the patch, I'm wondering if it could be > simplified by adding an implementation of priv_basicset inside #ifndef > HAVE_PRIV_BASICSET and doing away with the inline ifdefs?After a read of the man pages it seems that priv_str_to_set allocates and priv_basicset doesn't so it's not a drop in replacement, however factoring the basicset code out still seems to be cleaner, eg (completely untested): diff --git a/configure.ac b/configure.ac index b4c0aaa..d910f53 100644 --- a/configure.ac +++ b/configure.ac @@ -898,6 +898,7 @@ mips-sony-bsd|mips-sony-newsos4) fi AC_CHECK_FUNC([setppriv], [ AC_CHECK_HEADERS([priv.h], [ + AC_CHECK_FUNCS([priv_basicset]) SOLARIS_PRIVS="yes" ]) ]) diff --git a/openbsd-compat/port-solaris.c b/openbsd-compat/port-solaris.c index 962cd16..4a42a36 100644 --- a/openbsd-compat/port-solaris.c +++ b/openbsd-compat/port-solaris.c @@ -233,6 +233,22 @@ solaris_set_default_project(struct passwd *pw) # include <priv.h> # endif +priv_set_t * +solaris_basic_privset(void) +{ + priv_set_t *pset; + +#ifdef HAVE_PRIV_BASICSET + if ((pset = priv_allocset()) == NULL) + fatal("priv_allocset: %s", strerror(errno)); + priv_basicset(pset); +#else + if ((pset = priv_str_to_set("basic", ",", NULL)) == NULL) + fatal("priv_str_to_set: %s", strerror(errno)); +#endif + return pset; +} + void solaris_drop_privs_pinfo_net_fork_exec(void) { @@ -254,11 +270,9 @@ solaris_drop_privs_pinfo_net_fork_exec(void) * etc etc). */ - if ((pset = priv_allocset()) == NULL || - (npset = priv_allocset()) == NULL) + if ((pset = priv_allocset()) == NULL) fatal("priv_allocset: %s", strerror(errno)); - - priv_basicset(npset); + npset = solaris_basic_privset(); if (priv_addset(npset, PRIV_FILE_CHOWN) != 0 || priv_addset(npset, PRIV_FILE_DAC_READ) != 0 || @@ -294,11 +308,8 @@ solaris_drop_privs_root_pinfo_net(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); + pset = solaris_basic_privset(); if (priv_delset(pset, PRIV_FILE_LINK_ANY) != 0 || priv_delset(pset, PRIV_NET_ACCESS) != 0 || @@ -319,11 +330,9 @@ solaris_drop_privs_root_pinfo_net_exec(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); + pset = solaris_basic_privset(); if (priv_delset(pset, PRIV_FILE_LINK_ANY) != 0 || priv_delset(pset, PRIV_NET_ACCESS) != 0 || diff --git a/openbsd-compat/port-solaris.h b/openbsd-compat/port-solaris.h index b077e18..3a41ea8 100644 --- a/openbsd-compat/port-solaris.h +++ b/openbsd-compat/port-solaris.h @@ -26,8 +26,11 @@ void solaris_contract_pre_fork(void); void solaris_contract_post_fork_child(void); void solaris_contract_post_fork_parent(pid_t pid); void solaris_set_default_project(struct passwd *); +# ifdef USE_SOLARIS_PRIVS +priv_set_t *solaris_basic_privset(void); void solaris_drop_privs_pinfo_net_fork_exec(void); void solaris_drop_privs_root_pinfo_net(void); void solaris_drop_privs_root_pinfo_net_exec(void); +# endif /* USE_SOLARIS_PRIVS */ #endif diff --git a/sandbox-solaris.c b/sandbox-solaris.c index 98714e1..8e81c2b 100644 --- a/sandbox-solaris.c +++ b/sandbox-solaris.c @@ -48,16 +48,15 @@ ssh_sandbox_init(struct monitor *monitor) struct ssh_sandbox *box = NULL; box = xcalloc(1, sizeof(*box)); - box->pset = priv_allocset(); + + /* Start with "basic" and drop everything we don't need. */ + box->pset = solaris_basic_privset(); if (box->pset == NULL) { free(box); return NULL; } - /* Start with "basic" and drop everything we don't need. */ - priv_basicset(box->pset); - /* Drop everything except the ability to use already-opened files */ if (priv_delset(box->pset, PRIV_FILE_LINK_ANY) != 0 || priv_delset(box->pset, PRIV_NET_ACCESS) != 0 || -- Darren Tucker (dtucker at zip.com.au) GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69 Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
Darren Tucker wrote:> On Thu, Feb 18, 2016 at 9:31 AM, Damien Miller <djm at mindrot.org> wrote: >> On Wed, 17 Feb 2016, Alex Wilson wrote: >> >>> On 2/17/16 2:04 PM, Alex Wilson wrote: >>>> I've attached a patch... >>>> >>> Also at >>> >>> https://us-east.manta.joyent.com/arekinath/public/openssh-wip-fix-for-sol10-privs.patch >>> >>> If you are having trouble getting the patch out of the email. >>> >>> Also, as for Damien's patch, you will want to regenerate configure etc. >>> This doesn't go on top of his suggestion (I don't want to just disable >>> this new code for Solaris 10 if possible, it would be better to fix it >>> up so that you can have sandbox support as well) >> I don't have any Solaris systems around to test, so I'm pretty much flying >> blind. I'd love to be able to ship something that works for both 10 and 11 >> though, so if you are both able to come up with a patch including the >> autoconf gunk that works then I'll get it committed. > I do have a SPARC I can blow the dust off and test with so I'll take a > look at this one. > > After a quick look at the patch, I'm wondering if it could be > simplified by adding an implementation of priv_basicset inside #ifndef > HAVE_PRIV_BASICSET and doing away with the inline ifdefs? >I've got operational SPARC systems that I can test on, so that's not a problem, but we're running only Solaris 10 at this point, nothing any older, and most of it can't run Solaris 11 either. Alex Wilson: my problem with patches turned out to be a problem with an old version of autoconf, such that autoreconf was hosing things up. -- Jeff Wieland | Purdue University Network Systems Administrator | ITIS UNIX Platforms Voice: (765)496-8234 | 155 S. Grant Street FAX: (765)496-1380 | West Lafayette, IN 47907
On 2/17/16 3:54 PM, Darren Tucker wrote:> On Thu, Feb 18, 2016 at 10:22 AM, Darren Tucker <dtucker at zip.com.au> wrote: > [...] >> After a quick look at the patch, I'm wondering if it could be >> simplified by adding an implementation of priv_basicset inside #ifndef >> HAVE_PRIV_BASICSET and doing away with the inline ifdefs? > > After a read of the man pages it seems that priv_str_to_set allocates > and priv_basicset doesn't so it's not a drop in replacement, however > factoring the basicset code out still seems to be cleaner, eg > (completely untested): >That patch looks nicer to me, too. It compiles on S10 and Illumos, but I haven't fully tested it yet (the S10 box I found currently has a broken OpenSSL which I'm trying to figure out) For older S10 I've also had to add this patch (below), since it seems PRIV_NET_ACCESS is also a newer addition (it came in sometime in the osol era apparently) diff --git a/openbsd-compat/port-solaris.c b/openbsd-compat/port-solaris.c index 4a42a36..40285b7 100644 --- a/openbsd-compat/port-solaris.c +++ b/openbsd-compat/port-solaris.c @@ -282,13 +282,17 @@ solaris_drop_privs_pinfo_net_fork_exec(void) fatal("priv_addset: %s", strerror(errno)); if (priv_delset(npset, PRIV_FILE_LINK_ANY) != 0 || - priv_delset(npset, PRIV_NET_ACCESS) != 0 || priv_delset(npset, PRIV_PROC_EXEC) != 0 || priv_delset(npset, PRIV_PROC_FORK) != 0 || priv_delset(npset, PRIV_PROC_INFO) != 0 || priv_delset(npset, PRIV_PROC_SESSION) != 0) fatal("priv_delset: %s", strerror(errno)); +# if defined(PRIV_NET_ACCESS) + if (priv_delset(npset, PRIV_NET_ACCESS) != 0) + fatal("priv_delset: %s", strerror(errno)); +# endif + if (getppriv(PRIV_PERMITTED, pset) != 0) fatal("getppriv: %s", strerror(errno)); @@ -312,11 +316,15 @@ solaris_drop_privs_root_pinfo_net(void) pset = solaris_basic_privset(); if (priv_delset(pset, PRIV_FILE_LINK_ANY) != 0 || - priv_delset(pset, PRIV_NET_ACCESS) != 0 || priv_delset(pset, PRIV_PROC_INFO) != 0 || priv_delset(pset, PRIV_PROC_SESSION) != 0) fatal("priv_delset: %s", strerror(errno)); +# if defined(PRIV_NET_ACCESS) + if (priv_delset(pset, PRIV_NET_ACCESS) != 0) + fatal("priv_delset: %s", strerror(errno)); +# endif + if (setppriv(PRIV_SET, PRIV_PERMITTED, pset) != 0 || setppriv(PRIV_SET, PRIV_LIMIT, pset) != 0 || setppriv(PRIV_SET, PRIV_INHERITABLE, pset) != 0) @@ -335,12 +343,16 @@ solaris_drop_privs_root_pinfo_net_exec(void) pset = solaris_basic_privset(); if (priv_delset(pset, PRIV_FILE_LINK_ANY) != 0 || - priv_delset(pset, PRIV_NET_ACCESS) != 0 || priv_delset(pset, PRIV_PROC_EXEC) != 0 || priv_delset(pset, PRIV_PROC_INFO) != 0 || priv_delset(pset, PRIV_PROC_SESSION) != 0) fatal("priv_delset: %s", strerror(errno)); +# if defined(PRIV_NET_ACCESS) + if (priv_delset(pset, PRIV_NET_ACCESS) != 0) + fatal("priv_delset: %s", strerror(errno)); +# endif + if (setppriv(PRIV_SET, PRIV_PERMITTED, pset) != 0 || setppriv(PRIV_SET, PRIV_LIMIT, pset) != 0 || setppriv(PRIV_SET, PRIV_INHERITABLE, pset) != 0) diff --git a/sandbox-solaris.c b/sandbox-solaris.c index 8e81c2b..a1506d6 100644 --- a/sandbox-solaris.c +++ b/sandbox-solaris.c @@ -59,7 +59,6 @@ ssh_sandbox_init(struct monitor *monitor) /* Drop everything except the ability to use already-opened files */ if (priv_delset(box->pset, PRIV_FILE_LINK_ANY) != 0 || - priv_delset(box->pset, PRIV_NET_ACCESS) != 0 || priv_delset(box->pset, PRIV_PROC_EXEC) != 0 || priv_delset(box->pset, PRIV_PROC_FORK) != 0 || priv_delset(box->pset, PRIV_PROC_INFO) != 0 || @@ -67,7 +66,12 @@ ssh_sandbox_init(struct monitor *monitor) free(box); return NULL; } - +# if defined(PRIV_NET_ACCESS) + if (priv_delset(box->pset, PRIV_NET_ACCESS) != 0) { + free(box); + return NULL; + } +# endif /* These may not be available on older Solaris-es */ # if defined(PRIV_FILE_READ) && defined(PRIV_FILE_WRITE) if (priv_delset(box->pset, PRIV_FILE_READ) != 0 ||