Page MenuHomeFreeBSD

libsysdecode: Fix decoding of Capsicum rights
ClosedPublic

Authored by markj on Apr 10 2022, 5:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 22 2024, 9:13 PM
Unknown Object (File)
Mar 22 2024, 9:13 PM
Unknown Object (File)
Mar 22 2024, 9:13 PM
Unknown Object (File)
Mar 12 2024, 9:14 PM
Unknown Object (File)
Jan 4 2024, 6:07 PM
Unknown Object (File)
Jan 4 2024, 6:07 PM
Unknown Object (File)
Jan 4 2024, 6:07 PM
Unknown Object (File)
Dec 19 2023, 10:21 PM

Details

Summary

Capsicum rights are a bit tricky since some of them are subsets of
others, and one can have rights R1 and R2 such that R1 is a subset of
R2, but there is no collection of named rights whose union is R2. So,
they don't behave like normal flag sets.

The commit tries to address several problems:

  • not all named rights are included in the caprights table due to the restrictive regexp used to collect them
  • if the passed rights are not a subset of the whole table, nothing is printed
  • if we print the name of every matching right, there is some redundancy in the output, e.g., a CAP_MMAP_R right would be printed as "CAP_MMAP_R,CAP_MMAP,CAP_SEEK,CAP_READ,CAP_SEEK_TELL".

Try to fix all of these problems:

  • use a constructor to sort the caprights table such that "larger" rights appear first and thus are matched first
  • don't print rights that are a subset of rights already printed, so as to minimize the length of the output
  • print a trailing message if some of the specific rights are not matched by the table

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Apr 10 2022, 5:33 PM

don't print rights that are a subset of rights already printed, so as to minimize the length of the output

I think this is the sensible thing to do, but we should make sure this is clearly noted in documentation. I could see it being confusing if a user specified a number of rights and we print only the composite equivalent (e.g. CAP_SEEK | CAP_READ turning into CAP_PREAD)

don't print rights that are a subset of rights already printed, so as to minimize the length of the output

I think this is the sensible thing to do, but we should make sure this is clearly noted in documentation. I could see it being confusing if a user specified a number of rights and we print only the composite equivalent (e.g. CAP_SEEK | CAP_READ turning into CAP_PREAD)

Good point, fixed.

Thanks, I think the man page change captures it well. I picked the CAP_PREAD example since it seemed like the most simple plausible case someone would encounter.

lib/libsysdecode/mktables
162

how do CAP_SOCK_CLIENT and CAP_SOCK_SERVER interact with this? I guess gen_table only looks at #define lines so they'll be skipped entirely. Probably OK.

#define CAP_SOCK_CLIENT \
        (CAP_CONNECT | CAP_GETPEERNAME | CAP_GETSOCKNAME | CAP_GETSOCKOPT | \
         CAP_PEELOFF | CAP_RECV | CAP_SEND | CAP_SETSOCKOPT | CAP_SHUTDOWN)
#define CAP_SOCK_SERVER \
        (CAP_ACCEPT | CAP_BIND | CAP_GETPEERNAME | CAP_GETSOCKNAME | \
         CAP_GETSOCKOPT | CAP_LISTEN | CAP_PEELOFF | CAP_RECV | CAP_SEND | \
         CAP_SETSOCKOPT | CAP_SHUTDOWN)
lib/libsysdecode/mktables
162

Yeah I think that's ok. Those helpers aren't event documented in rights(4).

pauamma_gundo.com added inline comments.
lib/libsysdecode/sysdecode_cap_rights.3
50

Since the example mentions the superset first, I'd match that in the narrative.

markj marked an inline comment as done.

Be a bit more consistent.

emaste added inline comments.
lib/libsysdecode/sysdecode_cap_rights.3
57

I read "thus" as "therefore" or "consequently" and thus it doesn't feel quite right; we've made the intentional decision to omit printing the subset rights, it's not a natural consequence of being a superset/subset. Other meanings of "thus" probably work, but just omitting the word seems fine.

This revision is now accepted and ready to land.Apr 12 2022, 4:30 PM
markj marked an inline comment as done.

Fix wording

This revision now requires review to proceed.Apr 12 2022, 9:43 PM
jhb added inline comments.
lib/libsysdecode/flags.c
1217

Hmmm, I guess it's fine to do this as a constructor vs only paying the cost on first-use. Trying to do it on first-use is a bit of a PITA in a library since our pthread_once() is broken in the single-threaded case (hence libc having its own special libc_once hack) (we really should fix our pthread_once() some day.. I think libstdc++ is the one thing that would be suboptimal as a result perhaps)

This revision is now accepted and ready to land.Apr 12 2022, 10:34 PM
lib/libsysdecode/flags.c
1217

Yeah, it seemed simpler to just do it here. It appears to be cheap enough in any case.

This revision was automatically updated to reflect the committed changes.