Bug 2140

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.

>+/* 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?

Put this on the map for 6.4

(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@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.

Created attachment 2352 [details]
capsicum

Tested on FreeBSD 10 Alpha 5

Comment on attachment 2352 [details]
capsicum

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

(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 :-)

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.

Created attachment 2364 [details]
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 ?

Comment on attachment 2364 [details]
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.

(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.

Created attachment 2365 [details]
capsicum

(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.

Created attachment 2371 [details]
FreeBSD 10-BETA2

Tested on latest FreeBSD 10 BETA 2 snapshot.

Diff was adjusted slightly based on latest changes.

ping6 dtucker :-) ?

ping6 ?

Adjusted the diff for FreeBSD 10 RC3 release which is going to be released on the 3rd of January.

Created attachment 2397 [details]
FreeBSD 10 RC3

Tested on FreeBSD 10 RC3

Tested on FreeBSD 10 RC 4

Tested on FreeBSD 10RC5

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.

Patch applied. This will be in OpenSSH-6.5 - thanks!

Created attachment 2398 [details]
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.

also applied, thanks.

There's still an issue. I'll upload a new diff in a few hours.

As discussed with pjd@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.

Created attachment 2401 [details]
disable setrlimit(RLIMIT_NOFILE, 0)

Created attachment 2405 [details]
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@freebsd.org) and based on his diff.

applied - thanks

Close all resolved bugs after 7.3p1 release