kern: add a security knob to disable unprivileged access to kenv
AcceptedPublic
Actions

Authored by kevans on Sat, Jun 20, 3:42 AM.

Details

Reviewers

dch
ziaee
imp
zlei

Group Reviewers

manpages
secteam
Jails

Summary

We sometimes store sensitive things in the kenv that get zapped, but we
really shouldn't rely on that zapping to actually happen. Most
unprivileged processes don't really need to read from the kernel
environment in the first place, so add a knob that allows it to be
disabled.

Note that we consider jailed root to be unprivileged from this
perspective; they have their own meta/env concepts and we should
encourage users to take advantage of those for passing information to
jails.

"Hey we should do something about that": dch

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 74047
Build 70930: arc lint + arc unit

Event Timeline

kevans created this revision.Sat, Jun 20, 3:42 AM

Herald added subscribers: ziaee, olce, imp. · View Herald TranscriptSat, Jun 20, 3:42 AM

kevans requested review of this revision.Sat, Jun 20, 3:42 AM

Harbormaster completed remote builds in B74047: Diff 180131.Sat, Jun 20, 3:42 AM

Manpages looks good to me, thank you!

This revision is now accepted and ready to land.Sat, Jun 20, 3:51 AM

zlei added a subscriber: zlei.Sat, Jun 20, 7:16 AM

zlei added inline comments.

sys/kern/kern_environment.c
211	To match the previous behavior, I think `kenv_read_allowed()` should be move out of #ifdef PRESERVE_EARLY_KENV block.
sys/sys/priv.h
144	For the naming, what about `PRIV_KENV_GET`, to match `KENV_GET` sys call ? I think `KENV_DUMP` implies multiple `KENV_GET` .

jamie accepted this revision as: Jails.Sat, Jun 20, 1:34 PM

kevans marked an inline comment as done.Sat, Jun 20, 1:52 PM

kevans added inline comments.

sys/kern/kern_environment.c
211	Whoops, yes, sorry- fixed locally.
sys/sys/priv.h
144	I was trying to avoid naming it after a specific op because there's likely no future where we try to make the read size more granular, and READ maps pretty cleanly to both operations -- my concern is that one might be mislead into thinking KENV_DUMP isn't covered by PRIV_KENV_GET for whatever reason since it's a 1:1 name.

Sounds good. At some point, CRA issues may militate that future things like this be 'secure by default' rather than 'don't violate pola'. At some. point, we'll have to have this bigger conversation, but it shouldn't gate this now.

In D57697#1322931, @imp wrote:

Sounds good. At some point, CRA issues may militate that future things like this be 'secure by default' rather than 'don't violate pola'. At some. point, we'll have to have this bigger conversation, but it shouldn't gate this now.

Ah, I should have mentioned it in the commit message- my hope is to land the knob and MFC it, then propose default-off before 16.0.

What sensitive things are being exposed through kenv? Is there some subsystem which should be scrubbing its kenv entries during boot but isn't?

(I have no objection to the change, just wondering.)

In D57697#1322952, @markj wrote:

What sensitive things are being exposed through kenv? Is there some subsystem which should be scrubbing its kenv entries during boot but isn't?

(I have no objection to the change, just wondering.)

There's nothing being exposed today- we were chatting last night about how loader.conf is effectively unfiltered kenv and that's maybe not great, and also that we do things like transfer GELI passphrases from the loader with it. That gets cleared out by a SYSINIT (assuming geom_eli gets loaded) so that will be reliably done before we go multi-user, but we're a small oops away from exposing something we shouldn't if a user wants to try and use it for some other form of knowledge transfer.

what about bsdinstall's security screen?

In D57697#1322962, @freebsd_igalic.co wrote:

what about bsdinstall's security screen?

I will post a follow-up diff

Looks good to me.

sys/sys/priv.h
144	The kenv(2) says the action argument `KENV_GET`, KENV_GET Get the value of the variable with the given name. Use `Get` rather than `Read` to keep the terminology consistent.