ministat(1): Capsicumify

[cem]

Summary

Separate dataset opening from reading/parsing. The number of input
files is already capped to a small number, so just open all input files
before sandboxing.

Test Plan

• ministat < iguana
• ministat iguana chameleon iguana
• 'make test'

[allanjude]

usr.bin/ministat/ministat.c

619 ↗

(On Diff #20423)

Do you need to allow TIOCGETA for STDIN and friends?

[cem]

usr.bin/ministat/ministat.c

619 ↗

(On Diff #20423)

Stdin — I don't think so. (fread or whatever invokes isatty() doesn't seem to behave wrongly when that call fails.)

Stdout/stderr — I also don't think so. It seems to work fine without that privilege. I believe isatty() on std output streams controls line-buffering, but I'm not sure that matters for ministat. Ministat output, unlike e.g. logs, doesn't really make sense one line at a time.

I'm not especially confident of either of those statements. I've just observed it seems to work fine as-is. In the absence of *any other factor*, I prefer fewer lines of code. :-)

[jhb]

usr.bin/ministat/ministat.c

619 ↗

(On Diff #20423)

I wonder if it makes sense to have some kind of wrapper for the "normal" stdio cases i.e. something like:

cap_permit_stdio_read(fd);
cap_permit_stdio_write(fd);

And then another small wrapper:

cap_permit_stdio_std()
{
    cap_permit_stdio_read(STDIN_FILENO);
    cap_permit_stdio_write(STDOUT_FILENO);
    cap_permit_stdio_write(STDERR_FILENO);
}

Only in the sense of if having these would reduce the boilerplate required in N utilities.

[cem]

usr.bin/ministat/ministat.c

619 ↗

(On Diff #20423)

Yep, I think something like that would make sense.

[emaste]

I have an alternative suggestion in D7982 for these sorts of programs - just don't bother restricting stdio rights, at least initially, and we can introduce the wrappers jhb mentions later on.

[cem]

In D7925#165071, @emaste wrote:

I have an alternative suggestion in D7982 for these sorts of programs - just don't bother restricting stdio rights, at least initially, and we can introduce the wrappers jhb mentions later on.

Sure, we could remove the stdio restrictions. It wouldn't reduce the diff very much, though. (Most of the change is around preopening input files, of which there are a limited number (up to ~8).)

We can always change the code as it is in this patch when such wrappers are introduced, too.

[emaste]

In D7925#165141, @cem wrote:

Sure, we could remove the stdio restrictions.

I think we need to either remove them or add fstat and TIOCGETA.

[cem]

In D7925#165616, @emaste wrote:

In D7925#165141, @cem wrote:

Sure, we could remove the stdio restrictions.

I think we need to either remove them or add fstat and TIOCGETA.

Why? They appear to have zero impact on this program.

[emaste]

Why? They appear to have zero impact on this program.

As a matter of principle I think we should not attempt to perform operations for which we do not hold a sufficient capability. As of rS306081 as a debugging aid you can make software trap when attempting to perform an operation not permitted in capability mode or not permitted by the rights held on the fd.

[emaste]

This comment was removed by emaste.

[cem]

Update to use helpers.

[cem]

Ping — any objection to this patch? I believe it's ready to commit.