Page MenuHomeFreeBSD

[sshd 7.8p1] avoid to violate capability mode
ClosedPublic

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

Details

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.

  1. login_getpwclass(3) is not allowed in capability mode.

    -> We add a wrapper function to proxy the function.
  1. accessing timezone file is not allowed in capability mode.

    -> cache timezone data to call caph_cache_tzdata().
  1. 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

  1. set kern.trap_enotcap=1
$ su root  -c "sysctl kern.trap_enotcap=1"
  1. ssh login
$ ssh localhost
  1. expect successfully login

Regression Tests

a. Following sshd option works.

  1. UseDNS=yes
  2. UseDNS=no

b. login.conf permission works.

  1. :host.allow=myhost:
  2. :host.deny=myhost:
  3. :times.allow=0900-1700:
  4. :times.deny=0900-1700:

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem accepted this revision.Sep 12 2018, 4:09 AM

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

monitor.c
733–735 ↗(On Diff #47939)

style nits: '!=0' should have space between '!=' and '0' in 2/3 of these lines

sshbuf-getput-basic.c
475–476 ↗(On Diff #47939)

style(9) multi-line comments look like:

/*
 * foo
 * bar
 */
530–540 ↗(On Diff #47939)

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
monitor.c
733–735 ↗(On Diff #47939)

Oh, I made a stupid mistake.
I'll fix this.

sshbuf-getput-basic.c
475–476 ↗(On Diff #47939)

Thanks! I'll fix this.

530–540 ↗(On Diff #47939)

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
monitor_wrap.c
293 ↗(On Diff #47945)

I changed code style as same as sshbuf_free_passwd()
No functional change.

cem accepted this revision.Sep 12 2018, 2:49 PM

Looks great to me.

monitor_wrap.c
293 ↗(On Diff #47945)

One more style(9) nit: separate lines for conditionals and statements, i.e.:

if (lc == NULL)
    return;
sshbuf-getput-basic.c
544 ↗(On Diff #47945)

same style nit :)

This revision is now accepted and ready to land.Sep 12 2018, 2:49 PM
monitor_wrap.c
293 ↗(On Diff #47945)

OK. I'll fix this.

sshbuf-getput-basic.c
544 ↗(On Diff #47945)

I'll fix this, too.

Fixed code style issues

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

👍

This revision is now accepted and ready to land.Sep 13 2018, 1:39 AM
des accepted this revision.EditedOct 5 2018, 2:18 PM

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, committed by ). · Explain WhyOct 6 2018, 9:33 PM
This revision was automatically updated to reflect the committed changes.