Paths

Table of Contentst

[sshd 7.8p1] avoid to violate capability mode
ClosedPublic
Actions

Authored by naito.yuichiro_gmail.com on Sep 12 2018, 3:53 AM.

Details

Reviewers

oshogbo
emaste
cem
des

Commits

rS339216: sshd: address capsicum issues

Summary

This is a fix for PR #231172 and for updating D17056 against to OpenSSH-7.8p1.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231172
https://reviews.freebsd.org/D17056

Sandboxed child process authenticate client request that violates some capabilities.
We fix following problems.

login_getpwclass(3) is not allowed in capability mode.

-> We add a wrapper function to proxy the function.

accessing timezone file is not allowed in capability mode.

-> cache timezone data to call caph_cache_tzdata().

reverse resolve hostname from IP address is not allowed in capability mode.

-> reverse resolve hostname before entering capability mode.

Test Plan

Test case

Successfully login via ssh with sysctl kern.trap_enotcap=1

set kern.trap_enotcap=1

$ su root  -c "sysctl kern.trap_enotcap=1"

ssh login

$ ssh localhost

expect successfully login

Regression Tests

a. Following sshd option works.

UseDNS=yes
UseDNS=no

b. login.conf permission works.

:host.allow=myhost:
:host.deny=myhost:
:times.allow=0900-1700:
:times.deny=0900-1700:

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

naito.yuichiro_gmail.com created this revision.Sep 12 2018, 3:53 AM

naito.yuichiro_gmail.com mentioned this in D17056: [sshd] add wrapper function of login_getpwclass.

Functionality looks good to me. Some minor style suggestions follow.

monitor.c
733–735	style nits: '!=0' should have space between '!=' and '0' in 2/3 of these lines
sshbuf-getput-basic.c
475–476	style(9) multi-line comments look like: /* * foo * bar */
530–540	I suggest just using sshbuf_free_passwd(). calloc() initial pointers will be zero and free(NULL) is well defined.

This revision is now accepted and ready to land.Sep 12 2018, 4:09 AM

naito.yuichiro_gmail.com added inline comments.Sep 12 2018, 10:40 AM

monitor.c
733–735	Oh, I made a stupid mistake. I'll fix this.
sshbuf-getput-basic.c
475–476	Thanks! I'll fix this.
530–540	My memory was outdated... Any way, using sshbuf_free_passwd() makes my code shorter and looks fine. And I'll add NULL check at the first of `sshbuf_free_passwd()`. Because I feel scared if NULL pointer is passed to the argument,

Fixed commented issues.

This revision now requires review to proceed.Sep 12 2018, 10:43 AM

naito.yuichiro_gmail.com added inline comments.Sep 12 2018, 10:45 AM

monitor_wrap.c
293	I changed code style as same as `sshbuf_free_passwd()` No functional change.

Looks great to me.

monitor_wrap.c
293	One more style(9) nit: separate lines for conditionals and statements, i.e.: if (lc == NULL) return;
sshbuf-getput-basic.c
544	same style nit :)

This revision is now accepted and ready to land.Sep 12 2018, 2:49 PM

naito.yuichiro_gmail.com added inline comments.Sep 13 2018, 1:36 AM

monitor_wrap.c
293	OK. I'll fix this.
sshbuf-getput-basic.c
544	I'll fix this, too.

Fixed code style issues

This revision now requires review to proceed.Sep 13 2018, 1:37 AM

👍

This revision is now accepted and ready to land.Sep 13 2018, 1:39 AM

I would strongly recommend submitting the sshbuf_{get,put,free}_passwd() part of this patch upstream.

Closed by commit rS339216: sshd: address capsicum issues (authored by emaste). · Explain WhyOct 6 2018, 9:33 PM

This revision was automatically updated to reflect the committed changes.

In D17128#371823, @des wrote:

I would strongly recommend submitting the sshbuf_{get,put,free}_passwd() part of this patch upstream.

Was this done?

In D17128#608395, @bdrewery wrote:

In D17128#371823, @des wrote:

I would strongly recommend submitting the sshbuf_{get,put,free}_passwd() part of this patch upstream.

Was this done?