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

[kevans]

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

[kevans]

• Fix bogus indentation

[kevans]

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

[emaste]

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]

• 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]

usr.bin/grep/file.c

265–269

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

395

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]

• 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]

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

265

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

[kevans]

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

265

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]

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

265

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

[kevans]

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]

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]

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]

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]

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

[cem]

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]

Casper fileargs service exists now, see rS340373