Page MenuHomeFreeBSD
Differential D1345

Capsicum support for jot(1)
ClosedPublic
Actions

Authored by cem on Dec 20 2014, 7:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 10:11 AM
Unknown Object (File)
Oct 23 2024, 11:19 PM
Unknown Object (File)
Oct 20 2024, 6:11 AM
Unknown Object (File)
Sep 19 2024, 4:01 PM
Unknown Object (File)
Sep 19 2024, 10:41 AM
Unknown Object (File)
Sep 19 2024, 10:41 AM
Unknown Object (File)
Sep 19 2024, 10:41 AM
Unknown Object (File)
Sep 19 2024, 10:40 AM
View All Files
Subscribers
emaste
imp

Details

Reviewers
emaste
allanjude
trasz
brueffer
jonathan
pjd
rwatson
Commits
rS307660: Capsicum support for jot(1)
Summary

Limit descriptors and enter capability mode in jot(1)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 5228
Build 5374: CI src buildJenkins

Event Timeline

brueffer updated this revision to Diff 2797.Dec 20 2014, 7:33 PM
brueffer retitled this revision from to Capsicum support for jot(1).
brueffer updated this object.
brueffer edited the test plan for this revision. (Show Details)
brueffer added reviewers: pjd, rwatson, jonathan, eadler, trasz.
rwatson edited edge metadata.Jan 5 2015, 9:16 PM

For some reason, phabricator is not willing to show me full context on this file -- whereas it did for your elfdump patch. I wonder why?

brueffer updated this revision to Diff 3011.Jan 6 2015, 2:51 PM
brueffer edited edge metadata.

No changes, trying to get diff context working.

rwatson added a comment.Jan 6 2015, 6:15 PM

Overall I like these changes a lot, but had a few questions about implied need for ambient authority relating to random number generation and localisation.

usr.bin/jot/jot.c
120

It makes me slightly nervous that we need CAP_WRITE | CAP_FSTAT | CAP_IOCTL for stdout, but only CAP_WRITE for stderr.

288

I guess srandom() doesn't access /dev/random (unlike srandomdev()), but it would be good to convince ourselves that it is able to properly initialise in capability mode. (I seem to recall we do allow access to the arc4random sysctl from capability mode for this reason .. but maybe srandom() doesn't use that?)

397

Is there any risk that various locale-related bits might need ambient authority to initialise?

emaste added a subscriber: emaste.Jan 6 2015, 8:52 PM
eadler resigned from this revision.Mar 26 2015, 3:41 AM
eadler removed a reviewer: eadler.
eadler added inline comments.
usr.bin/jot/jot.c
397

why would they? Its just a table lookup?

jonathan added inline comments.May 22 2015, 12:15 PM
usr.bin/jot/jot.c
288

srandom() calls good_rand() (pure computation - lib/libc/stdlib/random.c:219), and random() (which only calls good_rand()). So, it's entirely numerical computation based on the provided seed.

On a side note, srandomdev() also seems to use sysctl() rather than opening /dev/random as a file (hooray!).

brueffer added a comment.May 22 2015, 12:57 PM

kib was vehemently opposed, so I left it for now. Maybe something to discuss at BSDCan.

jonathan edited edge metadata.Jul 10 2015, 6:48 PM

So... any progress on @rwatson's CAP_IOCTL comment?

jonathan requested changes to this revision.Jul 23 2015, 2:19 PM
jonathan edited edge metadata.

I think we need an answer to the CAP_IOCTL question before this change proceeds further.

usr.bin/jot/jot.c
288

(so I believe that we can call this comment "done")

397

@brueffer: any response?

This revision now requires changes to proceed.Jul 23 2015, 2:19 PM
emaste added reviewers: cem, allanjude.Sep 19 2016, 7:14 PM
cem commandeered this revision.Sep 19 2016, 9:54 PM
cem edited reviewers, added: brueffer; removed: cem.
Herald added a subscriber: imp. · View Herald TranscriptSep 19 2016, 9:54 PM
cem updated this revision to Diff 20507.Sep 19 2016, 10:34 PM
cem edited edge metadata.
cem marked 2 inline comments as done.
  • Restrict stderr identically to stdout.
  • Preopen NLS database(s) for err() usage after cap_enter().
usr.bin/jot/jot.c
318

Note arc4random -> arc4_stir attempts to fall back to /dev/random if the kern.arandom sysctl fails. (I don't know why it would fail; maybe it's a fallback that can be removed.)

I don't think this is a problem (the sysctl is used first).

397

Yes. err and friends need the localization database(s) opened.

jonathan accepted this revision.Oct 3 2016, 12:52 AM
jonathan edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 3 2016, 12:52 AM
rwatson added inline comments.Oct 3 2016, 8:33 AM
usr.bin/jot/jot.c
318

We should ensure that srandomdev() failing leads to an application fail stop. Right now, srandomdev() does not have a return value -- which means we must ensure that libc triggers application exit if both the sysctl and /dev/random fail. This is especially important in Capsicum, where the latter is guaranteed to fail!

emaste added inline comments.Oct 3 2016, 12:05 PM
usr.bin/jot/jot.c
318

See D8077

emaste added inline comments.Oct 3 2016, 1:44 PM
usr.bin/jot/jot.c
318

And now D8133 for srandomdev.

emaste added inline comments.Oct 3 2016, 1:50 PM
usr.bin/jot/jot.c
113

Code often makes assumptions about fd 0/1/2. We may want to limit stdin to no rights rather than closing it (cap_rights_limit(STDIN_FILENO, cap_rights_init(&rights));) as a pattern to follow, even if it doesn't matter for jot.

See for example rS306056 in elfdump.

cem added inline comments.Oct 3 2016, 3:47 PM
usr.bin/jot/jot.c
113

Sure.

emaste accepted this revision.Oct 3 2016, 3:53 PM
emaste added a reviewer: emaste.

LGTM either way

cem updated this revision to Diff 21084.Oct 5 2016, 7:41 PM
cem edited edge metadata.
  • Update to use capsicum_helpers header.
  • Restrict stdin rather than closing it.
This revision now requires review to proceed.Oct 5 2016, 7:41 PM
cem marked an inline comment as done.Oct 5 2016, 7:41 PM
emaste accepted this revision.Oct 19 2016, 9:44 PM
emaste edited edge metadata.
This revision is now accepted and ready to land.Oct 19 2016, 9:44 PM
Closed by commit rS307660: Capsicum support for jot(1) (authored by cem). · Explain WhyOct 19 2016, 9:51 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
usr.bin/
jot/
17 lines

Diff 3011

View Options

usr.bin/jot/jot.c

Loading...