Page MenuHomeFreeBSD

Improve parameters handling in veriexec
AcceptedPublic

Authored by hum_semihalf.com on Dec 3 2021, 10:05 AM.

Details

Summary

Provide more robust parameter parsing in veriexec. Do a little cleanup as well.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

wma requested review of this revision.Dec 3 2021, 10:05 AM
sbin/veriexec/veriexec.c
47

Can you please use the @brief format

93

This would break all our usage of veriexec for past 15+ years.
There is no need to pass more arg than is sufficient to be unambiguous.
So 'a' is sufficient. 'e' is sufficient etc.

198

again the requirement to fully spell out debug vs 'd' is a step backwards.

stephane.rochoy_stormshield.eu added inline comments.
sbin/veriexec/veriexec.c
93

Well, veriexec(8) do not document it as the expected usage:

The possible states are:

loaded   set automatically when first manifest has been loaded.
active   mac_veriexec(4) will begin checking files.  This state can only be entered from the loaded state.
enforce  mac_veriexec(4) will fail attempts to exec(2) or open(2) files with O_VERIFY unless verified.
locked   prevent loading of any more manifests.

And, to be honest, this behavior is quite surprising (for example, mtree(8) wants keywords, not keyword-abbreviations) that's why we felt it would need to be adjusted. Could we agree on something in-between like strcmp(arg_text, "a") == 0 || strcmp(arg_text, "active") == 0 (and adjust the man accordingly)?

sbin/veriexec/veriexec.c
93

I'm happy to update the man page to explain that a non-ambiguous prefix match is sufficient.

Note strcmp would never be a suitable method of matching, if more than a single character is needed, then strncmp would be useful eg.

if (strncmp("active", arg_text, strlen(arg_text) == 0)
sbin/veriexec/veriexec.c
93

I believe this parameter parsing should be improved:

  • veriexec -i a && veriexec -i active && veriexec -i alphonse all resolve to the same status. "alphonse" is not even a prefix of "activate".
  • in today's code, "lock" resolves to "locked", but "loc" (which is an unambiguous prefix of "locked") resolved to "loaded".

Personally I find the unambiguous prefix matching a bit overkill for such a small program. I suggest the following: each status can be matched either by a long string ("activate", "locked") or a shortcut string ("a" for "activate", "lock" for "locked", etc...).

sbin/veriexec/veriexec.c
93

FWIW the 'locked' state is something we have never used, it is a hold over from the original NetBSD implementation which relied on manifests loaded during single user and then state locked - the only way to update was to reboot.
Using signed manifests allowed for much greater flexibility.

The use of strncmp as I described earlier is a simple way to allow better matching without breaking backwards compatability.

162

ie. current state Is?

hum_semihalf.com set the repository for this revision to rG FreeBSD src repository.

Match args with shortest prefix. Add help message and prevent from running with invalid path.

One style nit, but otherwise looks ok

sbin/veriexec/veriexec.c
247

space before (

This revision is now accepted and ready to land.May 6 2022, 4:12 PM
This revision now requires review to proceed.May 9 2022, 7:50 AM
This revision is now accepted and ready to land.May 13 2022, 6:00 PM
This revision now requires review to proceed.May 16 2022, 5:24 AM
sbin/veriexec/veriexec.c
100–103

"l" and "lo" resolve to "loaded" while it should yield an error.

Add a simple check for the argument whether it matches only one option. In case of ambiguity return an error.

This revision is now accepted and ready to land.May 17 2022, 7:34 AM