Page MenuHomeFreeBSD

libsysdecode: Fix decoding of Capsicum rights
ClosedPublic

Authored by markj on Apr 10 2022, 5:33 PM.
Tags
None
Referenced Files
F86219568: D34874.diff
Mon, Jun 17, 4:07 AM
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

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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45135
Build 42023: arc lint + arc unit

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.