openssh bugs - Nov 2015 - [Bug 2511] New: Drop fine-grained privileges on Illumos/Solaris

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

            Bug ID: 2511
           Summary: Drop fine-grained privileges on Illumos/Solaris
           Product: Portable OpenSSH
           Version: 7.1p1
          Hardware: Other
                OS: Solaris
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: alex+mailinglists_openssh-dev at cooperi.net

Created attachment 2761
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2761&action=edit
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.)

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

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

Alex Wilson <alex+mailinglists_openssh-dev at cooperi.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|7.1p1                       |-current

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

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
   Attachment #2761|application/octet-stream    |text/plain
          mime type|                            |
   Attachment #2761|0                           |1
           is patch|                            |

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

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2761
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2761
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.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
put this on the list for openssh-7.2


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

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
oh, and you'll need to add sandbox-solaris.c to SSHDOBJS in Makefile.in
too

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

Alex Wilson <alex+mailinglists_openssh-dev at cooperi.net> changed:

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

--- Comment #4 from Alex Wilson <alex+mailinglists_openssh-dev at
cooperi.net> ---
Created attachment 2770
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2770&action=edit
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...)

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

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
(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 :/

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
   Attachment #2770|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2770
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2770
patch-v2

This looks okay to me. Darren, can you take a second look?

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

Alex Wilson <alex+mailinglists_openssh-dev at cooperi.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2770|ok?(dtucker at zip.com.au)     |
              Flags|                            |
   Attachment #2770|0                           |1
        is obsolete|                            |

--- Comment #7 from Alex Wilson <alex+mailinglists_openssh-dev at
cooperi.net> ---
Created attachment 2771
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2771&action=edit
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.

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

Alex Wilson <alex+mailinglists_openssh-dev at cooperi.net> changed:

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

--- Comment #8 from Alex Wilson <alex+mailinglists_openssh-dev at
cooperi.net> ---
Created attachment 2772
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2772&action=edit
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.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2772|0                           |1
           is patch|                            |
   Attachment #2772|application/octet-stream    |text/plain
          mime type|                            |

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2772|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2772
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2772
patch-v4

Darren, can you take a final look at this? It looks OK to me.

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2772|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #10 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2772
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2772
patch-v4

LGTM.  Ship it.

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

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
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?

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

--- Comment #12 from Damien Miller <djm at mindrot.org> ---
actually, it would pull in a bit of stuff - it calls into the PAM code.

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

--- Comment #13 from Darren Tucker <dtucker at zip.com.au> ---
(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.

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

Alex Wilson <alex+mailinglists_openssh-dev at cooperi.net> changed:

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

--- Comment #14 from Alex Wilson <alex+mailinglists_openssh-dev at
cooperi.net> ---
Created attachment 2774
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2774&action=edit
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@

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2774|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2774
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2774
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)

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2774|ok?(dtucker at zip.com.au)     |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=2511

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #16 from Damien Miller <djm at mindrot.org> ---
Committed - this will be in OpenSSH 7.2. Thanks for your patience :)

commit 4626cbaf78767fc8e9c86dd04785386c59ae0839
Author: Damien Miller <djm at 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@

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

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

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

--- Comment #17 from Tomas Kuthan <tomas.kuthan at oracle.com> ---
*** Bug 2299 has been marked as a duplicate of this bug. ***

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #18 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 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.