MAC/do: add exec whitelist/blacklist support for mac_do consumers

[polyduekes_proton.me]

Summary

this adds support for an optional exec paths list to the
security.mac.do.rules sysctl which allows for a whitelist
/blacklist for paths allowed to be exec'ed by userland
supervisors such as mdo

Signed-off-by: polyduekes <[e-mail]>

[olce]

I like the idea presented here, it goes further than some initial thoughts I had. It looks like it could be made to work. However, the implications of this new feature appear not to have been explored thoroughly, and more work remains for the change here to be consistent.

In particular, the following points must be taken into account:

1. It has been specified that any rule that matches (both the current and wanted credentials) allows a submitted transition to succeed, and consequently that the order of rules does not matter. In the implementation, so far we have stopped at the first matching rule because there was no point in examining another one (once a transition is allowed by one rule, we don't care if other rules also allow it). That is not true anymore with this new feature. The currently proposed implementation has the bug that the new exec paths feature's specification of all but the first rule allowing a given transition will just be ignored. It is not too hard to fix that. But I'll let you think about it on your own first.
2. What should happen when the module is unloaded or deactivated after a successful setcred() but before a call to exec()?
3. Grafting the additional data to struct mac_do_setcred_data, or any per-thread structure deriving from struct mac_do_data_header (there are currently no other examples, but there will be in the future) and pointed to by the current thread slot, is conceptually wrong on multiple levels. The current per-thread structure is meant to be transient, and actually the change as it is at the moment relies on an implementation detail (the optimization that clear_data() does not deallocate the slot and the pointed object) and thus works by accident. That scheme will definitely break when evolving mac_do(4) to handle traditional system calls, each of which needing its own structure. The cleanest way out seem to be to have a second per-thread slot for longer term data.

Please also see the inline comments.

Finally, you're internally using the same name for fields in support of this feature as for the existing "exec paths". I'm aware you're proposing to rename the existing "exec paths" feature in the next revision (D58013). Technically, we could still change the name, as that feature has not been merged into a release yet (and I've refrained to merge it into the upcoming 14.5). That said, I'm wary of changing the name now: That work was announced under that name for a while now, it has already been merged into 15 (end of june), and, although I was initially seduced by the idea, it's still unclear to me if "supported_bins" is any better. I wonder if we instead should use "exec list" for this feature, as there's the ability to do a blacklist instead of a whitelist. Ideas?

Speaking of which, I don't think a blacklist actually has any value. Any executable that is readable with the initial credentials could simply be copied elsewhere. Any executable that is readable by the target credentials could be copied by application of cp (if not ruled out; and anyway that part could be cirmcumvented as described in the previous sentence). Any thoughts on this topic?

sys/security/mac_do/mac_do.c

958

963

I don't like that how blanks are defined in a mac_do(4)'s rules definition, which is uniform, leads to scattered identical calls. Please factor out that line into some separate internal function, which would also be called by strsep_noblanks().

971

Having ! after the = sign is inconsistent with what we are doing with the "to" part of rules. I'd impose it before exec instead (as we do for gid flags), even if here there's a priori no technical reason to do so.

973–978

This case will be caught below (if (nb == 0)).

986

We actually could, by first making a copy of exec_str, but I agree it does not seem worth the trouble. I'd amend the comment though.

997–999

In this make_parse_error() call and those below, please keep track of the position we are in from the start of the optional block. Typically, the exec_str pointer should not change, and another one passed to strsep*() calls.

1004–1005

Minor, but reallocf() is not necessary, as M_WAITOK allocations cannot fail.

1017–1018

These paths are never freed on success. The corresponding code was added in D58014, but should appear in this change instead (commits should be safe-contained as much as possible).

1023–1025

I'd make this code unnecessary by always filling the exec_paths and exec_paths_nb fields on rule, ultimately letting parse_single_rule() do the deallocations.

1049–1050

Even if it currently supports only the exec option, it's actually a block with options, so let's call it accordingly.

1099–1100

1101–1102

This line should be unconditional (not in a if) and follow the assignment to to_list. The rest of this block should be moved below, see some below inline comments.

1101–1109

You're more or less reimplementing strsep() in this block, but with drawbacks. Instead, use again strsep_noblanks(), and make sure blanks are tolerated after the options block (possibly by using strsep_noblanks() a second time).

1101–1117

Almost all this code is too early in this function. While you indeed need to delineate the to_list before parsing it, parsing the options before to_list is not consistent with the rest of rules parsing and unnatural, as it will lead to reporting options error to users first, even if the to_list itself isn't parseable either (errors there should be reported first).

Code treating the optional block should appear just before the STAILQ_INSERT_TAIL(&rules->head, new, r_entries); line near the end of the function.

1134–1136

Looks like the to_p and to_list are logically inverted. Additionally, I don't think the _p suffix has much value? So please rename: to_list => to and to_p => to_list and change the existing to_list occurrences in the loop accordingly. That makes for more changes but I tend to think they are worth it for clarity. The original to_list name was a kind of misnomer.

2240–2249

Extending struct mac_do_setcred_data for the purpose of this new feature is unfortunately an abuse of the structure which has a very different lifecycle than what is needed here. Please see the herald comment for more details.

2240–2250

Gratuitous move. Please move the structure back to its original place.

2248

The exec_blacklist name gives the impression it is a list, whereas it is a boolean. Could you rename it, to something like exec_is_blacklist, or simply is_blacklist? (That's something to do only when you've fixed the lifecycle problem mentioned elsewhere, which most probably entails moving the new fields to a different structure.)

2624–2644

This new block shouldn't execute if error != 0. But break; should still be issued on error != EPERM.

2728

This alone does not compile. The missing changes were sent to D58014 by mistake. Could you please put them back into this revision, so that the corresponding commit is self-contained?

2804–2833

vn_fullpath_jail() must be used unconditionally, because:

1. Paths returned by the exec() machinery are relative to the current chroot, which may not be equal to the jail's root.
2. There doesn't seem to be any namecache issues to worry about here in the common cases, as the vnode will normally have been obtained through path resolution earlier.

Why is "vn_fullpath_jail() can return a cached path from a prior lookup of the same inode by another process." a problem?

2848

Not as simple. Currently, exec() can be called twice, and the second same call will succeed, which is obviously bad.