openssh bugs - Aug 2013 - [Bug 2140] New: Capsicum support for FreeBSD 10 (-current)

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

            Bug ID: 2140
           Summary: Capsicum support for FreeBSD 10 (-current)
           Product: Portable OpenSSH
           Version: -current
          Hardware: All
                OS: FreeBSD
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: loganaden at gmail.com

Created attachment 2326
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2326&action=edit
openssh-capsicum

Recently, I started playing around with FreeBSD 10.

Following a discussion on capsicum-mailing:

https://lists.cam.ac.uk/pipermail/cl-capsicum-discuss/2013-August/msg00000.html

I took pjd@ and des@ patches for the older openssh in freebsd-base, and
started working on implementing it on openssh-current.

The only change I made is use extern for pmonitor.

The patch works fine when I try to use connect() in the child process
as it fails, as connect() isn't listed in capabilities.conf.

It's still a WiP as capsicum is still a moving target.

Feedback appreciated.

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

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

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2326
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2326
openssh-capsicum

Looks good - a couple of small things.
>Index: sandbox-capsicum.c
>==================================================================>RCS
file: sandbox-capsicum.c
>diff -N sandbox-capsicum.c
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ sandbox-capsicum.c	7 Aug 2013 19:39:21 -0000
>@@ -0,0 +1,90 @@
>+
Please add a license block here.
http://www.openbsd.org/cgi-bin/cvsweb/src/share/misc/license.template?rev=1.3;content-type=text%2Fplain
is our preferred one.
>+/* Capsicum sandbox that sets zero nfiles, nprocs and filesize rlimits,
>+ * limits file descriptors on monitoring object,
>+ * and switches to capability mode
>+*/
Minor style nit. The first line of a multiline comment should be "/*"
by itself.
The last line's '*' should be aligned to the previous line's
(i.e. add
a space at the start of the line).
>+struct ssh_sandbox {
>+	struct monitor *monitor;
This isn't used and can be removed.
>+extern struct monitor *pmonitor;
This can go too.
>+	box->monitor = pmonitor;
and this.

>+	if (cap_rights_limit(box->monitor->m_recvfd, CAP_READ | CAP_WRITE)
== -1)
>+		fatal("%s: failed to limit the network socket", __func__);
>+	if (cap_rights_limit(box->monitor->m_log_sendfd, CAP_WRITE) == -1)
>+		fatal("%s: failed to limit the logging socket", __func__);
Are there any other fds open at this point? How about 0, 1 and 2 -
could they be limited?

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

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
             Blocks|                            |2130

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Put this on the map for 6.4

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

--- Comment #3 from Loganaden Velvindron <loganaden at gmail.com> ---
(In reply to Damien Miller from comment #1)
> Comment on attachment 2326 [details]
> openssh-capsicum
> 
> Looks good - a couple of small things.
> 
> >Index: sandbox-capsicum.c
> >==================================================================>
>RCS file: sandbox-capsicum.c
> >diff -N sandbox-capsicum.c
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> >+++ sandbox-capsicum.c	7 Aug 2013 19:39:21 -0000
> >@@ -0,0 +1,90 @@
> >+
> 
> Please add a license block here.
> http://www.openbsd.org/cgi-bin/cvsweb/src/share/misc/license.
> template?rev=1.3;content-type=text%2Fplain is our preferred one.
The diff is based on an older patch for OpenSSH written by des at freebsd.

http://people.freebsd.org/~pjd/patches/openssh-capsicum.patch

I added his copyright.
> 
> >+/* Capsicum sandbox that sets zero nfiles, nprocs and filesize
rlimits,
> >+ * limits file descriptors on monitoring object,
> >+ * and switches to capability mode
> >+*/
> 
> Minor style nit. The first line of a multiline comment should be
> "/*" by itself.
Corrected.
> The last line's '*' should be aligned to the previous
line's (i.e.
> add a space at the start of the line).
> 
> >+struct ssh_sandbox {
> >+	struct monitor *monitor;
> 
> This isn't used and can be removed.
> 
> >+extern struct monitor *pmonitor;
> 
> This can go too.
> 
> >+	box->monitor = pmonitor;
> 
> and this.
> 
Removed and tested on FreeBSD 10 ALPHA.
> 
> >+	if (cap_rights_limit(box->monitor->m_recvfd, CAP_READ |
CAP_WRITE) == -1)
> >+		fatal("%s: failed to limit the network socket", __func__);
> >+	if (cap_rights_limit(box->monitor->m_log_sendfd, CAP_WRITE) ==
-1)
> >+		fatal("%s: failed to limit the logging socket", __func__);
> 
> Are there any other fds open at this point? How about 0, 1 and 2 -
> could they be limited?
Yep, we can limit them completely.

No read and write possible on 0,1 & 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=2140

Loganaden Velvindron <loganaden at gmail.com> changed:

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

--- Comment #4 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2352
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2352&action=edit
capsicum

Tested on FreeBSD 10 Alpha 5

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2352
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2352
capsicum

This looks good to me. Darren can give a second opinion.

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

--- Comment #6 from Loganaden Velvindron <loganaden at gmail.com> ---
(In reply to Damien Miller from comment #5)
> Comment on attachment 2352 [details]
> capsicum
> 
> This looks good to me. Darren can give a second opinion.
I'll keep in sync with FreeBSD 10 development branch as changes are
made to capsicum to make sure that the patch works. FreeBSD 10 is
slated to be released around the 24th of November.

Thank you for reviewing this fairly large diff :-)

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

--- Comment #7 from Loganaden Velvindron <loganaden at gmail.com> ---
Hold on with the patch.

There are issues with it on the latest builds. Capsicum had been
changed recently, and it looks like the patch no longer works as
expected.

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

Loganaden Velvindron <loganaden at gmail.com> changed:

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

--- Comment #8 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2364
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2364&action=edit
capsicum

We need to reference pmonitor in sshd.c.

I used extern and then assigned it to box->monitor.

Alternatively, we can use des & pjd's approach which is to pass the
monitor struct in ssh_sandbox_init(struct monitor), and then limits the
descriptors rights.

What do you guys think ?

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

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2364
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2364
capsicum
>+struct ssh_sandbox {
>+	struct monitor *monitor;
>+	pid_t child_pid;
>+};
>+
>+extern struct monitor *pmonitor;
>+struct ssh_sandbox *
>+ssh_sandbox_init(void)
>+{
>+	struct ssh_sandbox *box;
>+
>+	/*
>+	 * Strictly, we don't need to maintain any state here but we need
>+	 * to return non-NULL to satisfy the API.
>+	 */
>+	debug3("%s: preparing capsicum sandbox", __func__);
>+	box = xcalloc(1, sizeof(*box));
>+	box->monitor = pmonitor;
I think it would be a better idea to just record the fd numbers
themselves in the struct rather than the monitor address.

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

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
(In reply to Loganaden Velvindron from comment #8)
> Alternatively, we can use des & pjd's approach which is to pass the
> monitor struct in ssh_sandbox_init(struct monitor), and then limits
> the descriptors rights.
Sorry, I missed this comment. Yes, I think passing the pointer to the
monitor is a good idea. Then ssh_sandbox_init() can store the fd
numbers from 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=2140

Loganaden Velvindron <loganaden at gmail.com> changed:

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

--- Comment #11 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2365
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2365&action=edit
capsicum

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

--- Comment #12 from Loganaden Velvindron <loganaden at gmail.com> ---
(In reply to Damien Miller from comment #10)
> (In reply to Loganaden Velvindron from comment #8)
> > Alternatively, we can use des & pjd's approach which is to
pass the
> > monitor struct in ssh_sandbox_init(struct monitor), and then limits
> > the descriptors rights.
> 
> Sorry, I missed this comment. Yes, I think passing the pointer to
> the monitor is a good idea. Then ssh_sandbox_init() can store the fd
> numbers from it.
Modified as requested.

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

Loganaden Velvindron <loganaden at gmail.com> changed:

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

--- Comment #13 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2371
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2371&action=edit
FreeBSD 10-BETA2

Tested on latest FreeBSD 10 BETA 2 snapshot.

Diff was adjusted slightly based on latest changes.

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

--- Comment #14 from Loganaden Velvindron <loganaden at gmail.com> ---
ping6 dtucker :-) ?

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

--- Comment #15 from Loganaden Velvindron <loganaden at gmail.com> ---
ping6 ?

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

--- Comment #16 from Loganaden Velvindron <loganaden at gmail.com> ---
Adjusted the diff for FreeBSD 10 RC3 release which is going to be
released on the 3rd of January.

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

Loganaden Velvindron <loganaden at gmail.com> changed:

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

--- Comment #17 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2397
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2397&action=edit
FreeBSD 10 RC3

Tested on FreeBSD 10 RC3

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

--- Comment #18 from Loganaden Velvindron <loganaden at gmail.com> ---
Tested on FreeBSD 10 RC 4

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

--- Comment #19 from Loganaden Velvindron <loganaden at gmail.com> ---
Tested on FreeBSD 10RC5

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

--- Comment #20 from Loganaden Velvindron <loganaden at gmail.com> ---
As discussed with the FreeBSD developers, no more API changes. FreeBSD
10 RC5 is the last RC. Assuming no showstopper, it's going to be
released as it is.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #21 from Damien Miller <djm at mindrot.org> ---
Patch applied. This will be in OpenSSH-6.5 - thanks!

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

--- Comment #22 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2398
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2398&action=edit
minor nits for capsicum

-Fix descriptions for stdin and stdout in error paths.

- use < 0 when calling cap_enter() so that it matches what dhclient and
other demons are doing on FreeBSD.

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

--- Comment #23 from Darren Tucker <dtucker at zip.com.au> ---
also applied, thanks.

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

--- Comment #24 from Loganaden Velvindron <loganaden at gmail.com> ---
There's still an issue. I'll upload a new diff in a few hours.

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

--- Comment #25 from Loganaden Velvindron <loganaden at gmail.com> ---
As discussed with pjd at FreeBSD:

There's an issue on FreeBSD where a CRIOGET ioctl is failing due to
setrlimit applied with NOFILE.

The commit message is not correct.

From:
http://svnweb.freebsd.org/base/head/crypto/openssh/servconf.c?view=log&pathrev=251088

Revert a local change that sets the default for UsePrivilegeSeparation
to
"sandbox" instead of "yes".  In sandbox mode, the privsep
child is
unable
to load additional libraries and will therefore crash when trying to
take
advantage of crypto offloading on CPUs that support it.

So comment it 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=2140

--- Comment #26 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2401
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2401&action=edit
disable setrlimit(RLIMIT_NOFILE, 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=2140

--- Comment #27 from Loganaden Velvindron <loganaden at gmail.com> ---
Created attachment 2405
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2405&action=edit
minor fixes

make the return value check for cap_rights_limit() consistent with the
man pages and also include a proper check for errno.

Discussed with Pawel J Dawidek (pjd at freebsd.org) and based on his diff.

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

--- Comment #28 from Damien Miller <djm at mindrot.org> ---
applied - thanks

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

Damien Miller <djm at mindrot.org> changed:

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

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