Capsicum helpers

[oshogbo]

Summary

Add header with some with predefined Capsicum helpers which reduce of duplicated lines between multiple programs.

[bapt]

I am a bit puzzled about the name vs the usage

capm_limit_stdin(STDIN_ERRNO) sounds like strange to me to repeat myself (twice stdin :)) but I have no proposal for better solution :)

sys/sys/capsicum_misc.h

65 ↗

(On Diff #20642)

Maybe add a flag to check for EBADF (in case one stdin can be closed?)

85 ↗

(On Diff #20642)

I find the name confusing. What about something like capm_cache_catpages (and above capm_cache_tzdata())

usr.bin/cmp/cmp.c

169 ↗

(On Diff #20642)

That is confusing :)

[oshogbo]

So my idea was to put names which tells you what you can do after calling that function.
For example capm_time calls all functions which prefetch data which you need to use time, if you want to use err you should call capm_err before, this was my idea but I open for suggestion :)

My idea with capm_limit_stdin/stdout was that this is standard option to limit descriptor for stdin/stdout so maybe we could call it for example capm_limit_fdin ?

I open for suggestion because naming is the hardest part ;)

[cem]

sys/sys/capsicum_misc.h

1 ↗

(On Diff #20642)

Bikeshed: I'd prefer capsicum_helpers.h :-).

42 ↗

(On Diff #20642)

Bikeshed: I'd prefer to return 0 or errno, kernel style. Or failing that, -1, POSIX style.

43 ↗

(On Diff #20642)

stdout might not be the best name. This is appropriate for all write-only stdio descriptors.

On the other hand, I wonder if we can just infer the direction of a stream automatically, and have one cap_limit_stdio(stream)? E.g.,

static inline int
capm_limit_stdio(FILE *stream)
{
cap_rights_t rights;
unsigned long cmds[] = { foo };
int error, flags;

cap_rights_init(&rights, CAP_FCNTL, CAP_FSTAT, CAP_IOCTL, CAP_READ, CAP_WRITE);
flags = fcntl(fileno(stream), F_GETFL);
if (flags < 0)
  return (-1);
if ((flags & O_ACCMODE) == O_WRONLY)
  cap_rights_clear(&rights, CAP_READ);
else if ((flags & O_ACCMODE) == O_RDONLY)
  cap_rights_clear(&rights, CAP_WRITE);

error = cap_rights_limit(fileno(stream), &rights);
if (error < 0 && errno == ENOSYS)
  return (0);

if (cap_ioctls_limit(fileno(stream), cmds, nitems(cmds)) < 0)
  return (-1);

if (cap_fcntls_limit(fileno(stream), CAP_FCNTL_GETFL) < 0)
  return (-1);

return (0);
}

85 ↗

(On Diff #20642)

Agreed.

[cem]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

Usage would be like so:

int
main(int argc, char **argv)
{
...

if (capm_limit_stdio(stdin) < 0 ||
  capm_limit_stdio(stdout) < 0 ||
  capm_limit_stdio(stderr) < 0)
    err(1, "restricting stdio");

FILE *foo = fopen("/bar", "w");
if (capm_limit_stdio(foo) < 0)
  err(1, "restricting foo");

(Yes, I agree we should have a common wrapper for doing the typical 3 stdin,out,err, but FILE* is used more widely than just those 3.)

[cem]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

I suppose we could call that capm_limit_file(FILE *f), and then use capm_limit_stdio(void) to perform it on stdin, stdout, stderr.

[allanjude]

sys/sys/capsicum_misc.h

42 ↗

(On Diff #20642)

agree

43 ↗

(On Diff #20642)

I like that syntax, with an additional wrapper to do the 3 default FDs at once.

65 ↗

(On Diff #20642)

probably a good idea

[emaste]

Adding David Drysdale for possible comment on this from the cross-platform perspective. This seems reasonable to me but I'd like to do it in a way that facilitates portability.

What about adding this to libcasper (either an implementation in the library, or header linlines)? It seems it does not belong in sys/ in any case.
Also for committing I think we should add the new functionality in one commit, then switch tools over to using it in a 2nd.

[cem]

In D8013#167065, @emaste wrote:

What about adding this to libcasper (either an implementation in the library, or header linlines)? It seems it does not belong in sys/ in any case.

Agreed, I think this should be in a library (not header).

Also for committing I think we should add the new functionality in one commit, then switch tools over to using it in a 2nd.

Agreed.

[ed]

sys/sys/capsicum_misc.h

42 ↗

(On Diff #20642)

Public header files should still use __inline. :-C

43 ↗

(On Diff #20642)

Would it make sense to just merge this into a single function that takes an fopen()-like mode string?

cap_limit_stdio(fd, "w+")

[cem]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

@ed , it seems the mode can be inferred from the file flags. Do you think there's any reason to explicitly pass (and parse) a string in that case?

[ed]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

Oh, sorry about that. I saw I forgot to send out my comment earlier, which is why I sent them out now, forgetting to remove the comment related to this issue. Yes, I agree that it can be inferred from the file flags. :-)

[ed]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

Wait. Now that I think of it, I'm not sure that using fcntl(F_GETFL) is the right way to go. The downside is that if you look at the stdin, stdout and stderr file descriptors, they all point to the same underlying descriptor pointing to the TTY:

#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>

int main() {
  printf("%d\n", fcntl(STDIN_FILENO, F_GETFL) & O_ACCMODE);
}

This prints 2, O_RDWR, even for stdin. We should somehow extract the original open mode passed to fopen(), fdopen(), etc. but I'm not sure there is a portable way to obtain this information.

[cem]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

Maybe that's fine for the TTY? I think stdin/stdout will usually have more appropriate modes if they point to opened pipes/files, no?

[ed]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

In my opinion the case of the TTY is the most important one. For files the rights are already fairly close to optimal, as they got opened with the right O_* value. Pipes only support a very small number of operations anyway. For TTYs you do actually want to limit these rights, so sandboxed apps cannot silently steal keystrokes, etc.

[cem]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

We're talking about the generic wrappers for FILE objects. The higher level cap_limit_stdio(void) routine could easily restrict stdout/err further (to not have CAP_READ, for example). We can always reduce capabilities in Capsicum :-).

(Also, the sandbox app has to be in the foreground to receive keystrokes, no? Typing into a console and expecting the foreground app not to receive keystrokes seems a little odd.)

[ed]

sys/sys/capsicum_misc.h

43 ↗

(On Diff #20642)

Well, that's the stupid thing about UNIX TTYs: whether or not you're at the foreground is determined by process group -- not by process.

[drysdale_google.com]

sys/sys/capsicum_misc.h

88 ↗

(On Diff #20642)

Does this leak a descriptor? (i.e. would there be problems if someone accidentally called this repeatedly/in a loop?)

[cem]

sys/sys/capsicum_misc.h

88 ↗

(On Diff #20642)

Nope, no problem there. catopen -> load_msgcat opens an fd, mmaps it, and then closes the fd. (Both open and mmap are readonly.) The category is stored in a global list and future lookups will prefer the cache rather than loading from file again.

lib/libc/nls/msgcat.c

[oshogbo]

Thank you all for discussion. With this change I'm trying too look into some middle ground off all problems.

First I rename capsicum_misc.h to capsicum_helpers.h which we all agree.

I have some issue with proposing the stream version of helpers are Capsicum function are doing work on descriptors so I would not change that behavior, in worse case you still can do fileno(3), in other direction is a bit harder to do. I also not a big fun of FILE structure, so please excuse me.

But idea of doing this base on mode I kinda like so I introduce caph_limit_fd().
I also addressed @bapt and @allanjude request about the EBADF.

Another and the biggest problem is the naming API, we all are a little bit divided about it. Unfortunately I don't have strong position about it. In one case I would like to name them as "after doing X you can do Y" but as @bapt point out to me some things can "unlocked" multiple different things like "catopen libc" is more useful then only doing err.

I will do as @emast suggested to do two different commits but I lave this review as once to show usage of all of those helpers.

I hope I didn't skip any request and I look forward for more discussion.

[bapt]

I like what I'm seeing now, thanks! I would like to see what others things about it
Do not forget the manpage :)

[cem]

Looks good, thanks. Some minor nits below.

sys/sys/capsicum_helpers.h

43–44 ↗

(On Diff #20911)

It seems to me like these routines should actually call into the general routine, perhaps with flags to indicate additional restrictions (CAPH_RDONLY, CAPH_WRONLY).

49 ↗

(On Diff #20911)

It's unclear to me why this is CAP_WRITE, not CAP_READ.

Should we also allow fcntl FGETFL? Seems harmless and some of stdio code uses it (e.g. fdopen).

67 ↗

(On Diff #20911)

Should this be CAP_WRITE?

And why not ioctl TIOCGETA for this one? Stdio uses it on stdout too, I believe.

90–92 ↗

(On Diff #20911)

This should handle O_RDWR as well.

I think this can be done generally in this routine, by using cap_rights_init(&rights, CAP_READ, CAP_WRITE) and then:

accmode = mode & O_ACCMODE;
if (accmode == O_RDONLY || CAPH_RDONLY)
  cap_rights_clear(&rights, CAP_WRITE);
else if (accmode == O_WRONLY || CAPH_WRONLY)
  cap_rights_clear(&rights, CAP_READ);

110 ↗

(On Diff #20911)

wronly?

usr.bin/cmp/cmp.c

166–168 ↗

(On Diff #20911)

Let's leave individual program changes for a separate review after the helpers are committed.

[oshogbo]

Update header as @cem suggested.

[cem]

sys/sys/capsicum_helpers.h

51 ↗

(On Diff #20941)

mode, not flags?

57 ↗

(On Diff #20941)

Maybe sort alphabetically.

60–62 ↗

(On Diff #20941)

So the one thing this is missing is a way to force lower capabilities than the file descriptor's current ACCMODE (Ed's tty suggestion). I'd suggest CAPH_RDONLY / CAPH_WRONLY flags for that.

74–75 ↗

(On Diff #20941)

Shouldn't we return success here?

[oshogbo]

Aghr to many stupid bugs...
What do you think about such approach?

[cem]

Looks good to me other than some minor style nits. Do other reviewers want to chime in? :-)

sys/sys/capsicum_helpers.h

42–43 ↗

(On Diff #20942)

Whitespace seems different between READ/WRITE

53 ↗

(On Diff #20942)

Usually spelled if ((flags & CAPH_READ) != 0) or even if (flags & CAPH_READ).

[allanjude]

[allanjude]

sys/sys/capsicum_helpers.h

77 ↗

(On Diff #20942)

Do we want '| CAPH_IGNORE_EBADF' here?

STDIN being closed is likely not an error, or we need to make caph_limit_stdio() accept the flag.

[oshogbo]

sys/sys/capsicum_helpers.h

77 ↗

(On Diff #20942)

Is the closed stdin a standard thing?
We could add flags to all of them but my goal was to add the standard path as simple as it can be.

[emaste]

What was the resolution on putting this into a library instead of inline functions in the header? E.g. libc's use of TIOCGETA is an implementation detail, and maybe a future libc won't use it, or will start using another ioctl?

[cem]

In D8013#168164, @emaste wrote:

What was the resolution on putting this into a library instead of inline functions in the header? E.g. libc's use of TIOCGETA is an implementation detail, and maybe a future libc won't use it, or will start using another ioctl?

I support making it a library. But I would also be fine with putting off libification until a follow-up commit.

[oshogbo]

That was my inital idea to recreate libcapsicum.

The problem that we wanted omit with libification is that we would need to add the library to every capsicumized util and use headers (so basically every). The count of the functions which we provides is now very limited. So there was some discussion if its worth doing it.

So I would leave it as it is and if we will add more functions to this header then I would libification it.

[drysdale_google.com]

In D8013#168195, @oshogbo wrote:

That was my inital idea to recreate libcapsicum.

On Linux the base Capsicum functions (cap_enter, cap_rights_limit etc.) will not be in libc, so it would be nice if "libcapsicum" was still available as a name/place for them to live in.

However, that doesn't necessarily make "libcapsicum" a bad name for where these helpers live -- it's probably fine to have both the low-level primitives (cap_enter, cap_rights_limit) plus the convenience helpers in the same library:

• Linux:
  • libcapsicum: low-level primitives, helper functions
• FreeBSD
  • libc: low-level primitives
  • libcapsicum: helper functions

(Aside: at the moment, the Linux userspace library is called libcaprights rather than libcapsicum, so as not to clash with FreeBSD libcapsicum as-was; when the latter went away, I intended to change the Linux name but haven't got that far yet.)

[oshogbo]

Lets try found middle ground again ;)

[ed]

lib/libcapsicum/capsicum_helpers.h

45 ↗

(On Diff #20989)

This should still use __inline ;-)

46 ↗

(On Diff #20989)

Maybe rename this to caph_limit_stream()? We're not just 'limiting a file descriptor', in this case we're interested in limiting rights for file descriptors, so that they can only be used as streams. FYI: stream is also the term POSIX/C uses for stdio objects.

[cem]

I'm fine with this. Ed's suggestions LGTM.