Page MenuHomeFreeBSD

mac_do: add a new MAC/do policy and mdo(1) utility
ClosedPublic

Authored by bapt on May 9 2024, 10:08 PM.
Tags
None
Referenced Files
F86512770: D45145.id138902.diff
Fri, Jun 21, 11:08 AM
Unknown Object (File)
Thu, Jun 13, 6:29 PM
Unknown Object (File)
May 22 2024, 4:34 AM
Unknown Object (File)
May 20 2024, 12:09 AM
Unknown Object (File)
May 19 2024, 5:59 PM
Unknown Object (File)
May 17 2024, 3:36 AM
Unknown Object (File)
May 16 2024, 8:21 PM
Unknown Object (File)
May 16 2024, 8:19 PM

Details

Summary

This policy enables a user to become another user without having to be
root (hence no setuid binary). it is configured via rules using sysctl
security.mac.do.rules

For example:
security.mac.do.rules=uid=1001:80,gid=0:any

The above rule means the user identifier by the uid 1001 is able to
become user 80
Any user of the group 0 are allowed to become any user on the system.

The mdo(1) utility expects the MAC/do policy to be installed and its
rules defined.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 57722
Build 54610: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Make sure only /usr/bin/mdo is allowed to use setuid and setgroups

How does this work for jailed processes/users? I may have missed out on something (a lot) since last I prodded the MAC framework, but IIRC the support for jail-specific rules is limited, at best?

The idea is good, we would quickly become users of such a feature, but only if it can be leveraged in a jailed environment..

delphij requested changes to this revision.May 14 2024, 5:41 PM
delphij added a subscriber: delphij.

The idea is interesting (I have never thought about a MAC policy to accomplish this goal) and I like its simplicity, please see comment inline for some minor nits.

I'm a little bit concerned about how this would interact with jails because it seems that the policy would also work for jails, but there is no way to specify different policies for them. It seems that it's not trivial to implement per-jail policy with the current MAC framework, but perhaps this could be implemented as blocking the request if inside jail by default with a knob to enable it?

share/man/man4/mac_do.4
12
sys/security/mac_do/mac_do.c
153–160

This doesn't seem right to me. In line 153, an unprotected read of rule_string would be performed, based on the context I believe what you should have done is something like:

	new_string = malloc(MAC_RULE_STRING_LEN, M_DO,
	    M_WAITOK|M_ZERO);
	mtx_lock(&rule_mtx);
	strcpy(new_string, rule_string);
	mtx_unlock(&rule_mtx);

        error = sysctl_handle_string(oidp, new_string, MAC_RULE_STRING_LEN, req);
	if (req->newptr == NULL)
		return (error));

and the error = sysctl_handle_string below should be gone.

usr.bin/mdo/mdo.1
4
usr.bin/mdo/mdo.c
5
This revision now requires changes to proceed.May 14 2024, 5:41 PM

Add jail support with jail parameter mdo.rules
Fix manpage

bapt marked 3 inline comments as done.

fix copyright

bapt marked 2 inline comments as done.

copyright again

per jail support has been added

sys/security/mac_do/mac_do.c
4
bapt marked an inline comment as done.May 16 2024, 6:46 AM
des requested changes to this revision.May 20 2024, 7:59 PM
des added a subscriber: des.
des added inline comments.
share/man/man4/mac_do.4
2
4

We normally place the copyright first and the tag below, cf. https://docs.freebsd.org/en/articles/license-guide/

20

(no paragraph break needed before a new section)

24
25
32
47
48
51
54
55
60
61

Please add an EXAMPLES section.

sys/security/mac_do/mac_do.c
2
5

We normally place the copyright first and the tag below, cf. https://docs.freebsd.org/en/articles/license-guide/

23

needs sorting

194
209
243
254

Here you unlock ppr while pr is still locked. Technically there's no rule against that but it seems odd.

259

In every case where you call this with non-null lrp, you unlock pr immediately on return, so why not just unlock it here?

307
354
403

Isn't this redundant? It's global, it will be initialized to all-zeroes by the loader.

536
usr.bin/mdo/mdo.1
2

See above.

usr.bin/mdo/mdo.c
2

See above.

58
62
64
67
This revision now requires changes to proceed.May 20 2024, 7:59 PM
bapt marked 31 inline comments as done.

Address all of des@ points

I think I have addressed all the @des points

share/man/man4/mac_do.4
24

You dropped “other” before the second “users”, was that intentional?

61

missing “and”

69
78
79
80
81
sys/security/mac_do/mac_do.c
235

I find these names confusing. Do rules and nrules actually point to several rules, or just one (each)? And I understand that nrules means “new rules” but I keep reading it as “number of rules” so perhaps it would be better not to abbreviate “new”.

265
265
343
391

According to style(9), pr should go below methods, but I won't insist if you prefer it this way around.

392

This could be static. I'm not sure it matters. (I would also suggest adding const to the osd_register() prototype, but that's not your fault.)

bapt marked 13 inline comments as done.May 22 2024, 11:44 AM
bapt added inline comments.
sys/security/mac_do/mac_do.c
235

rules points to several rules 0-N rules in a tailq

bapt marked an inline comment as done.

Address the new set of comments from @des

This revision was not accepted when it landed; it landed in state Needs Review.May 22 2024, 12:02 PM
This revision was automatically updated to reflect the committed changes.