Page MenuHomeFreeBSD

bsdgrep(1): Sprinkle some capsicum(4) on top
Needs ReviewPublic

Authored by kevans on Mar 23 2017, 6:44 PM.

Details

Reviewers
emaste
oshogbo
Summary

Sprinke some capsicum(4) on top of bsdgrep(1).

Unfortunately, we can't effectively enter capabilities mode as early as we should (or at all) in 2/3 cases because we lack a casper file service of any kind, so go the md5(1) route and enter capabilities mode immediately after opening the last file argument.

We do enter capability mode immediately if we're only reading stdin through no arguments, so that's at least one positive. Ideally it would all get refactored to enter capabilities mode immediately before the first instance of cap_enter().

This change does conflict with D10108, but I'll sort out the merge conflicts if/when the time arises.

Test Plan

Run kyua tests to ensure no regressions

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9036
Build 9442: arc lint + arc unit

Event Timeline

kevans created this revision.Mar 23 2017, 6:44 PM
kevans edited the summary of this revision. (Show Details)Mar 23 2017, 6:44 PM
kevans updated this revision to Diff 26616.Mar 23 2017, 6:45 PM
  • Fix bogus indentation

Also, I did some header fiddling based on style(9) recommendations in the files I was working with.

cem added a subscriber: cem.Apr 5 2017, 12:30 AM
emaste edited edge metadata.Apr 5 2017, 6:23 PM

This does not apply on top of the other bsdgrep changes right now, so I'm going to suggest that we postpone this for a short while and then rebase and review it after the other changes are in.

kevans updated this revision to Diff 27952.May 2 2017, 11:57 PM
  • Rebase on top of other changes
  • Fix some style(9) nits in various places

I believe we're otherwise done introducing changes that would conflict with this.

emaste added inline comments.May 3 2017, 1:40 AM
usr.bin/grep/file.c
271–275

We should probably wrap this with something like

#ifndef WITHOUT_CAPSICUM
...
#endif

to keep this portable for other users (other BSDs, OS X, etc.), as was done in bhyve.

usr.bin/grep/grep.c
411

It's preferable I think to declare the capsicum rights in the two places cap_rights_limit is called, perhaps using the idiom from the man page, something like:

cap_rights_t rights;

if (cap_rights_limit(fd, cap_rights_init(&rights, CAP_READ, CAP_WRITE)) < 0)
        err(1, "Unable to limit capability rights");

(with appropriate rights of course)

kevans updated this revision to Diff 27957.May 3 2017, 2:18 AM
  • Wrap capsicum bits in WITHOUT_CAPSICUM blocks for portability
  • Also wrap last_included_file/do_cap_enter logic in WITHOUT_CAPSICUM to avoid that overhead when the result won't be used
  • Use cap_rights_t variables on the stack at the two invocations, taking cue from cap_rights_init(3) for initialization
cem added a comment.May 3 2017, 7:35 PM

I think I'd prefer a less complicated patch where we just opened "/" readonly and used relative opens for all file arguments. I'm not sure there's much value in trying to protect single argument grep. (Preopening all input files would require more code modification for little benefit.)

usr.bin/grep/file.c
271

Can we drop the stdin check? Theoretically any file can be opened as fd 0.

kevans added a comment.May 5 2017, 6:06 PM
In D10121#219577, @cem wrote:

I think I'd prefer a less complicated patch where we just opened "/" readonly and used relative opens for all file arguments. I'm not sure there's much value in trying to protect single argument grep. (Preopening all input files would require more code modification for little benefit.)

In that case, I may abandon this outright (rather than postpone and maintain) and poke @oshogbo about the Casper file service a bit more. I believe that would allow for a much simpler introduction of capsicum(4) to bsdgrep. oshogbo@ indicated as of our previous discussion that a file service was to be introduced in the short term.

usr.bin/grep/file.c
271

Uhhh, I suppose there's no harm in removing it. I originally added it since caph_limit_stdio() is invoked almost immediately in main(), but it doesn't seem to do any damage to limit it after that.

cem added a comment.May 5 2017, 6:12 PM
In D10121#220284, @kevans91_ksu.edu wrote:

In that case, I may abandon this outright (rather than postpone and maintain) and poke @oshogbo about the Casper file service a bit more. I believe that would allow for a much simpler introduction of capsicum(4) to bsdgrep. oshogbo@ indicated as of our previous discussion that a file service was to be introduced in the short term.

I wouldn't count on such a thing being reviewed and available any time soon. But maybe I am mistaken.

usr.bin/grep/file.c
271

Right — no harm in restricting a descriptor to same or fewer rights than it already has.

kevans added a comment.May 5 2017, 6:23 PM
In D10121#220286, @cem wrote:

I wouldn't count on such a thing being reviewed and available any time soon. But maybe I am mistaken.

Then revisiting your previous comment- that seems like it could also be fairly complicated, though maybe not. We'd have to make sure to resolve all arguments to absolute paths before entering capability mode (unless realpath(3) is available in capability mode? My understanding was that it is not). I guess that's not much of an issue, though, because we'd likely have to do this anyways with a file service since we still can't access global namespace.

Opening "/" readonly and proceeding that way makes me slightly uncomfortable, but finding a common path in all potential path arguments sounds like extra overhead that we don't need to introduce.

I'll take a poke at it within the next week or so.

cem added a comment.May 5 2017, 6:33 PM
In D10121#220295, @kevans91_ksu.edu wrote:

Then revisiting your previous comment- that seems like it could also be fairly complicated, though maybe not. We'd have to make sure to resolve all arguments to absolute paths before entering capability mode (unless realpath(3) is available in capability mode? My understanding was that it is not). I guess that's not much of an issue, though, because we'd likely have to do this anyways with a file service since we still can't access global namespace.

Right, realpath won't work in Capability mode (lstat(2) calls). We could open two directory fds — one for "/" for absolute paths, and one for "." for relative paths. Then we'd use one or the other for opening file arguments, depending on if they start with "/" or not. That wouldn't be especially invasive.

Opening "/" readonly and proceeding that way makes me slightly uncomfortable, but finding a common path in all potential path arguments sounds like extra overhead that we don't need to introduce.

Right, it's maybe not ideal. Preopening all input files (another option) has performance ramifications and requires more invasive code changes. The common path is potentially only "/", so it's maybe not too useful generally.

I'll take a poke at it within the next week or so.

Ok, great!

kevans added a comment.May 5 2017, 6:37 PM
In D10121#220307, @cem wrote:

Right, realpath won't work in Capability mode (lstat(2) calls). We could open two directory fds — one for "/" for absolute paths, and one for "." for relative paths. Then we'd use one or the other for opening file arguments, depending on if they start with "/" or not. That wouldn't be especially invasive.

Yeah, unfortunately '.' may not be helpful since openat(2) will not accept paths with a '..' component or symlinks with a '..' component. This caveat alone makes it not worth the headache to not-realpath(3) everything. =( Sounds good otherwise, though!

cem added a comment.May 5 2017, 7:20 PM
In D10121#220308, @kevans91_ksu.edu wrote:

Yeah, unfortunately '.' may not be helpful since openat(2) will not accept paths with a '..' component or symlinks with a '..' component. This caveat alone makes it not worth the headache to not-realpath(3) everything. =( Sounds good otherwise, though!

Ah, right, or at least, you cannot use ".." to escape above the cwd fd. ".." components are allowed now, so long as they do not escape the initial directory fd. So we must at least realpath all file inputs to use that method. At that point you may as well just preopen all of the files :-).

kevans added a comment.May 5 2017, 7:41 PM

Ah, cool, that's less bad than the man page indicated the last time I peaked at it (relatively recently)

cem added a comment.May 5 2017, 8:12 PM
In D10121#220325, @kevans91_ksu.edu wrote:

Ah, cool, that's less bad than the man page indicated the last time I peaked at it (relatively recently)

Yeah, it was added quite recently (added disabled-by-default in r308212 and enabled-by-default in r309887). Maybe the manual page wasn't updated.

emaste added a subscriber: bapt.May 25 2017, 5:37 PM

Casper fileargs service exists now, see rS340373