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

967

972

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().

980

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.

982–987

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

995

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.

1006–1008

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.

1013–1014

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

1026–1027

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).

1032–1034

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.

1063–1066

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

1113–1114

1115–1116

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.

1115–1123

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).

1115–1131

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.

1151–1154

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.

2276–2285

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.

2276–2286

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

2284

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.)

2705–2715

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

2847

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?

2920–2949

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?

2964

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

[polyduekes_proton.me]

use a separate osd slot lifecycle
fix ordering for the blacklist literal(!)
added cleanup code
use any-match and multi passes for the exec rule parsing
various other fixes

[polyduekes_proton.me]

In D58012#1339807, @olce wrote:

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.

I have fixed that using any-match and multi passes for both whitelist and blacklist but perhaps all-match for blacklist would be better,can you please provide your thoughts on this?

2. What should happen when the module is unloaded or deactivated after a successful setcred() but before a call to exec()?

if the module is unloaded before a exec(),it's hook is deregistered so mac_do_vnode_check_exec doesn't run and there's nothing to deny the exec, so exec would be allowed in that case

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.

I am extremely sorry for doing that, i will make changes asap and switch to using a second per-thread slot as you suggested

Please also see the inline comments.

Thanks i have responded to them appropriately,Please do check

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?

i think "supported_bins" indeed sounds better but as you say i think it's not a good decision to change it now after it has been merged into 15(i was not aware of that at that time) so i agree with going with "exec list" but it has high chances to confuse an user about the difference between "exec paths" and "exec list",perhaps the manpage can be amended to note the distinction for clarity?

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?

currently that is correct ,as the code currently is there is very little gain from a blacklist but the reason i added it is because i am planning to extend this feature in future to use mac_veriexec to check the hash alongside the path of whatever is to be exec'd but doing that in practice would require some things to change such as building and shipping the mac_veriexec module by default(it's currently disabled by default) as such i have kept the mac_veriexec additions on hold for now and instead using it the way it is currently as a temperory resort
thank you for taking the time to review my code,

sys/security/mac_do/mac_do.c

1006–1008

i have found that the anchor for this will be better suited at the start of the full rule,not the optional block as using the full rule anchor in parse_rule_exec makes the position directly comparable without an extra adjustment at the call site,is it ok to go ahead this way? please let me know if i am missing anything,thanks

2920–2949

initially i was using vn_fullpath_jail() unconditionally as well,but after some testing i found that some orders of failed exec and successful exec will lead to erronous states such as:

• when i tested mdo id,i found that after the case where a user runs mdo id and /usr/bin/id is not in whitelisted paths,subsequant uses of mdo id where /usr/bin/id is in whitelisted paths would also fail,after some probing with dtrace i found that the resolved path was not /usr/bin/id in such cases which obviously meant when it was compared with path members, the resolved path did not match any path member which resulted in failure of mdo id even when allowed by the rule.
• the testing method is:

1. to set something like sysctl security.mac.do.rules='uid=1001>uid=0,gid=*,+gid=*(exec=/usr/local/bin/a)
2. then run mdo id followed by mdo whoami
3. then set sysctl security.mac.do.rules='uid=1001>uid=0,gid=*,+gid=*(exec=/usr/local/bin/a:/usr/bin/id)
4. then run mdo id again and observe exec failure because the resolved path will remain /usr/bin/whoami
5. dtrace output for example :

PID 70197:

has_exec_constraint = 1
exec_blacklist      = 0
exec_paths_nb       = 2

PID 70197 vn_fullpath_jail args:

vp      = fffff800573bb898
&path   = fffffe00a98488f8
&to_free = fffffe00a98488f0
return value = 0
resolved path = /usr/bin/whoami
to_free ptr (from *to_free) = fffff80203841800
path[0] = /usr/local/bin/a

strcmp return value = 10

path[1] = /usr/bin/id

strcmp return value = -14

• for the convenience of testing i have attached links to github gist containing the dtrace script and code i initially used at gist

2964

i have fixed that with exemption of the interpreter

[olce]

Have received notifications that you made updates. I'm mostly AFK until end of August, so please expect slow responses or no response at all until that point. Thanks for your patience.