mac_do(4): Implemented new sysctl knob and jail param for allowed executable paths #2

thesynthax · 2025-07-03T12:52:05Z

Features:

Added a new global sysctl knob for allowed executable paths (security.mac.do.exec_paths)
Added a new jail parameter for allowed executable paths (mac.do.exec_paths)
Added a new struct (conf) which will be a container for jail rules and the executable paths
Utilised the inheritance logic from rules for conf
Made setting of rules and exec_paths more robust

Signed-off-by: Kushagra Srivastava <[email protected]>

OlCe2

As you said via Signal, there is indeed currently a leak because you're allocating twice the fields rules and exec_paths of struct conf. First alloc happens via alloc_conf(), second one occurs when you call parse_rules()/clone_rules() and parse_exec_paths()/clone_exec_paths(), where you are basically crushing the existing pointers (which point to the leaked memory).

Additionally, clone_rules() only makes a shallow copy, whereas a deep copy is required (or else, you would have to have a ref count on struct rules and struct exec_paths also, but I suggest to give up on that for the time being).

More generally, I think you should just inline struct rules and struct exec_paths directly in struct conf, as said in inline comments. This will simplify lifecycle problems for the time being, and help push this forward.

There are a lot more inline comments with suggested changes. Goal here is going further in ensuring that memory lifecycle only happens for struct conf, and to avoid lots of allocations and data copying in various places (which in particular will remove practically all risks of bugs, such as the one explained above).

If you don't think you can handle all that in a reasonable timeframe (a few days), what I would propose is for you to fix the two outstanding issues (first two paragraphs above), and then I can handle the rest myself.

We could have some phone call on monday or tuesday about all that as necessary.

(Note for myself: There is also a delicate concurrency issue when copying the settings of the applicable configuration, to be handled once these changes are in.)

sys/security/mac_do/mac_do.c

OlCe2 · 2025-07-04T14:30:03Z

sys/security/mac_do/mac_do.c

+	struct rules *rules;
+	struct exec_paths *exec_paths;


It would be simpler to just include the structs inline here (i.e., just removing the *).

sys/security/mac_do/mac_do.c

OlCe2 · 2025-07-04T15:05:31Z

sys/security/mac_do/mac_do.c

+	char *rules_string, *exec_paths_string;
+	int error, jsys, rules_len = 0, execs_len = 0;

+	/* Read mac.do = -1 if unset */


What I think you're meaning is that -1 is a sentinel value indicating an unspecified mode. This is somewhat redundant with the comment before the _Static_assert() above, but I'm fine with having one here. I would instead just add /* Mark unfilled. */ after the jsys = -1; statement below, as is done in mac_do_jail_set(). Actually, /* Mark unspecified. */ here and there seems even better.

Also, /* Read mac.do = -1 if unset */ is hard to understand without context. Maybe just remove this comment once you've added the one suggested above on the jsys = -1; line, or if you think this needs more explanation, replace with something like /* If no mode is explicitly specified, 'jsys' is initialized to -1 and will be overridden with a valid value based on other parameters. */ (And general style: Always finish sentences with dots in comments, even for small comments.)

OlCe2 · 2025-07-04T17:19:40Z

sys/security/mac_do/mac_do.c

+promote_inherited_conf(struct prison *pr, bool with_rules, bool with_execs)
+{
+	struct prison *ppr;
+	struct conf *parent = find_conf(pr, &ppr);


parent misleading. Rename to, e.g., applicable_conf.

sys/security/mac_do/mac_do.c

OlCe2 · 2025-07-04T17:39:50Z

sys/security/mac_do/mac_do.c

+	error = parse_and_set_exec_paths(td_pr, buf, &parse_error);
+	if (error != 0) {
+		if (print_parse_error)
+			printf("MAC/do: Parse error at index %zu: %s\n",
+			    parse_error->pos, parse_error->msg);
+		free_parse_error(parse_error);
+	}


Same as above, you would use parse_and_set_conf() here instead.

OlCe2 · 2025-07-04T17:47:37Z

sys/security/mac_do/mac_do.c

+	vfs_getopts(opts, "mac.do.exec_paths", &execs_err);
+
+	if (rules_err == ENOENT && execs_err == ENOENT)
+		set_default_conf(pr);


Testing for the presence of "mac.do.rules" and "mac.do.exec_paths" is not a bad idea per se, but I would prefer to avoid it, as:

Setting the default configuration (which disables mac_do(4)) at creation makes us immune to any changes in the jail machinery that would allow the jail being created to be observed before the parameters are set (not possible today, and very unlikely in the future).

This forces duplicating detection code. Typically here, you would also have to test whether strings are empty, consistently with what is done in other mac_do_jail_* functions.
Jails are not created often, and there is no real constraint performance-wise here.
So here I prefer less code (just call set_default_conf() unconditionally) rather than duplicate one.

sys/security/mac_do/mac_do.c

Signed-off-by: Kushagra Srivastava <[email protected]>

Multiple issues existed within the powerpc FP/VSX save/restore functionality, leading to register corruption and loss of register contents in specific scenarios involving high signal load and use of both floating point and VSX instructions. Issue #1 On little endian systems the PCB used the wrong location for the shadowed FP register within the larger VSX register. This appears to have been an attempt to correct issue #2 without understanding how the vector load/store instructions actually operate. Issue #2 On little endian systems, the VSX state save/restore routines swapped 32-bit words within the 64-bit aliased double word for the associated floating point register. This was due to the use of a word-oriented load/store vs. doubleword oriented load/store. Issue #3 The FPU was turned off in the PCB but not in hardware, leading to a potential race condition if the same thread was scheduled immediately after sigreturn. The triggering codebase for this is Go, which makes heavy use of signals and and generates an unusual mix of floating point and VSX assembler. As a result, when combined with th powerpc lazy FPU restore, a condition was repeatedly hit whereby the thread was interrupted in FP+VSX mode, then restored in FP only mode, thus reliably triggering the issues above. Also clean up the associated asm() style issue flagged by GitHub Actions. Signed-off-by: Timothy Pearson <[email protected]> MFC after: 1 week Pull Request: freebsd#1756

OlCe2

There is still a lot to tackle.

GitHub's reviews are not ideal, e.g., I don't think you can see the old comments on a new version of the diff, even if they are still applicable (i.e., have not been resolved). You can cycle through all comments by clicking on the comments icon (has two comics-like bubbles in it, with a number on the side), when on the "Files changed" tab.

Please be careful about reading all existing comments and fully understanding them.

sys/security/mac_do/mac_do.c

OlCe2 · 2025-08-04T10:30:11Z

sys/security/mac_do/mac_do.c

+	char *rules_string, *exec_paths_string;
+	int error, jsys, rules_len = 0, execs_len = 0;

+	/* Read mac.do = -1 if unset */


Also, /* Read mac.do = -1 if unset */ is hard to understand without context. Maybe just remove this comment once you've added the one suggested above on the jsys = -1; line, or if you think this needs more explanation, replace with something like /* If no mode is explicitly specified, 'jsys' is initialized to -1 and will be overridden with a valid value based on other parameters. */ (And general style: Always finish sentences with dots in comments, even for small comments.)

OlCe2 · 2025-08-04T10:43:09Z

sys/security/mac_do/mac_do.c

+		if (!has_rules && !has_execs) {
+			vfs_opterror(opts, "mac.do set to 'new' but neither rules nor exec_paths specified");
+			return (EINVAL);
+		}


I think we should indeed allow this case as described in my previous comment.

OlCe2 · 2025-08-04T12:10:09Z

sys/security/mac_do/mac_do.c

 	case JAIL_SYS_DISABLE:
+		remove_conf(pr);
+		return (0);
+
+	struct prison *p;


You've just removed remove_conf(), which could be enough but provided mac_do_jail_create() is changed as requested (see old comment for it).

OlCe2 · 2025-08-04T12:21:37Z

sys/security/mac_do/mac_do.c

-			break;
+	/* Infer jsys if needed */
+	if (jsys == -1) {
+		if (has_rules)


I'd suggest changing with:

Suggested change

if (has_rules)

if (has_rules || has_exec_paths)

i.e., if only exec paths are specified, we just copy the rules part. I think this is more consistent with the rest, even if it could be slightly more dangerous.

If you have a strong case that we should not, then at least please add a comment saying that not putting has_exec_paths is deliberate and why.

OlCe2 · 2025-08-04T12:51:45Z

sys/security/mac_do/mac_do.c

+	if (parent == NULL)
+		return (NULL);
+
+	struct conf *new_conf = alloc_conf();


Style: Declaration must be at top of file.

OlCe2 · 2025-08-04T12:56:17Z

sys/security/mac_do/mac_do.c

+{
+	struct prison *ppr;
+	struct conf *parent = find_conf(pr, &ppr);
+	prison_unlock(ppr);


You must obtain a reference on conf before releasing the prison lock here, else there is a risk that it is freed concurrently (e.g., if an administrator changes the settings of the upper jail) while we are reading from it. And you must release that reference when finished with conf.

Style: Statements must be separated from the declarations by an empty line.

OlCe2 · 2025-08-04T13:03:51Z

sys/security/mac_do/mac_do.c

-			    parse_error->pos, parse_error->msg);
-			free_parse_error(parse_error);
+		parent_conf = find_conf(curproc->p_ucred->cr_prison, &p);
+		prison_unlock(p);


Same problem here as explained in a comment I left in parse_and_set_conf(), you must obtain a reference on conf before releasing the prison lock.

OlCe2 · 2025-08-04T13:05:03Z

sys/security/mac_do/mac_do.c

+			}
+		}
+
+		prison_unlock(pr);


This prison_unlock() call is wrongly placed, as it won't be executed if exec_path_count is 0, and it has to be regardless of its value.

OlCe2 · 2025-08-04T13:20:43Z

sys/security/mac_do/mac_do.c

+
+	if (exec_paths->exec_path_count > 0) {
+		for (int i = 0; i < exec_paths->exec_path_count; i++) {
+			if (strcmp(exec_paths->exec_paths[i], path) == 0) {


Note (mostly for me): This does not work inside jails in general, as vn_fullpath() above returns the full path from the machine's root, not the one from the current jail. This is a pre-existing bug, which I didn't catch in testing as I only tested with child jails having the same root. You don't have to fix it yourself, but if you want to, then advance in path after the jail's root prefix (obtained through cr_prison->pr_path) before the loop with the strcmp() calls.

It's working in jails for me. My jail is stored in /root/jails/test, and I supplied "/usr/bin/mdo:/home/thesynthax/mdo" for mac.do.exec_paths, and still worked.

Signed-off-by: Kushagra Srivastava <[email protected]>

OlCe2

See inline comments for the leaks and breaking the "installed configuration (ref count non 0) should never be modified" invariant, which must be restored.

OlCe2 · 2025-08-11T13:14:02Z

sys/security/mac_do/mac_do.c

-toast_rules(struct rules *const rules)
+toast_rules(struct rules const rules)


Keep passing a pointer (else the structure is copied without reason; this could even lead to bugs if, e.g., modifying the structure like zero-ing it (this is not the case currently)).

OlCe2 · 2025-08-11T13:36:33Z

sys/security/mac_do/mac_do.c

+	struct rules rules;

 	_Static_assert(MAC_RULE_STRING_LEN > 0, "MAC_RULE_STRING_LEN <= 0!");
-	rules->string[0] = 0;
-	STAILQ_INIT(&rules->head);
-	rules->use_count = 0;
+	bzero(&rules, sizeof(rules));
+	rules.string[0] = '\0';
+	STAILQ_INIT(&rules.head);
+
 	return (rules);


Although it's a common idiom in lots of other languages, in C it's rare that a function takes a structure as an argument or returns a structure as these are just copied, which can lead to surprising behaviors (you modify a copy, not the original object; not the case here) and always to worse performance (sometimes it doesn't really matter, but in systems programming, it's better to avoid it always).

So, the idiom here is that you instead pass as an argument the pointer to the structure to modify, and modify the object through it (and you don't return anything (return type is void), or possibly the same pointer you received, although here there's no point in doing that).

OlCe2 · 2025-08-11T13:45:15Z

sys/security/mac_do/mac_do.c

 }

+static struct exec_paths
+init_exec_paths(void)


Same as for init_rules() (signature + remove redundant code).

OlCe2 · 2025-08-11T14:02:30Z

sys/security/mac_do/mac_do.c

 		if (error != 0) {
 			(*parse_error)->pos += rule - copy;
-			toast_rules(rules);
+			toast_rules(*rules);


As explained above:

Suggested change

toast_rules(*rules);

toast_rules(rules);

OlCe2 · 2025-08-11T14:11:14Z

sys/security/mac_do/mac_do.c

-	if (refcount_release(&rules->use_count))
-		toast_rules(rules);
+	if (refcount_release(&conf->use_count)) {
+		toast_rules(conf->rules);


Suggested change

toast_rules(conf->rules);

toast_rules(&conf->rules);

OlCe2 · 2025-08-11T16:15:11Z

sys/security/mac_do/mac_do.c

+	bzero(&rules, sizeof(rules));
+	rules.string[0] = '\0';


Just drop these two lines: The second is redundant with the first, and the first is redundant when assuming that the provided storage has been zeroed.

Suggested change

bzero(&rules, sizeof(rules));

rules.string[0] = '\0';

Add an herald comment before init_rules() saying it assumes the storage has been zeroed already.

OlCe2 · 2025-08-11T16:23:47Z

sys/security/mac_do/mac_do.c

+	prison_unlock(ppr);
+
+	if (ppr == pr)
+		conf = applicable_conf;


With this line and what happens below, you're breaking the invariant that, once allocated, configurations should never be modified. This is very important for correctness when another thread is concurrently executing a credentials-changing function. So you first have to duplicate applicable_conf in this case.

OlCe2 · 2025-08-11T16:24:22Z

sys/security/mac_do/mac_do.c

+	 	conf = alloc_conf();
+
+	if (rules_string != NULL && rules_string[0] != '\0') {
+		error = parse_rules(rules_string, &conf->rules, parse_error);


Your leak is here (and below for exec paths, and in the similar duplicated code in mac_do_jail_set() that should disappear) when conf is applicable_conf, see my comment above on how to fix.

OlCe2 · 2025-08-11T16:24:40Z

sys/security/mac_do/mac_do.c

+		conf->rules = clone_rules(&applicable_conf->rules);
+
+	if (exec_paths_string != NULL && exec_paths_string[0] != '\0') {
+		error = parse_exec_paths(exec_paths_string, &conf->exec_paths, parse_error);


Leak here, see comment above about how to fix.

OlCe2 · 2025-08-11T16:25:58Z

sys/security/mac_do/mac_do.c

+}
+
+static int 
+parse_and_set_conf(struct prison *pr, const char *rules_string, 


Add here the optimization of not retrieving the applicable conf when at least one of the rules and exec paths strings are not NULL.

Signed-off-by: Kushagra Srivastava <[email protected]>

The current incarnation of execvPe() is a bit messy, and it can be rather difficult to reason about whether we're actually doing the right thing with our errors. We have two cases in which we may enter the loop: 1.) We have a name that has no slashes in it, and we enter the loop normally through our strsep() logic to process $PATH 2.) We have a name with at least one slash, in which case we jump into the middle of the loop then bail after precisely the one iteration if we failed Both paths will exit the loop if we failed, either via jumping to the `done` label to preserve an errno or into the path that clobbers errno. Clobbering errno for case #2 above would seem to be wrong, as we did not actually search -- this would seem to be what POSIX expects, as well, based on expectations of the conformance test suite. Simplify reasoning about the two paths by splitting out an execvPe_prog that does the execve(2) call specifically, and returns based on whether the error would be fatal in a PATH search or not. For the relative/absolute case, we can just ignore the return value and keep errno intact. The search case gets simplified to return early if we hit a fatal error, or continue until the end and clobber errno if we did not find a suitable candidate. Another posix_spawnp() test is added to confirm that we didn't break our EACCES behavior in the process. Reviewed by: des, markj Sponsored by: Klara, Inc. Differential Revision: https://reviews.freebsd.org/D51629

github-actions · 2025-08-21T12:34:30Z

Thank you for taking the time to contribute to FreeBSD!
All issues resolved.

thesynthax added 3 commits July 2, 2025 13:15

mac_do(4): Complete refactor of allowed executable paths feature

33edbca

Signed-off-by: Kushagra Srivastava <[email protected]>

mac_do(4): Fixed changing security.mac.do.* knobs in inheritance mode

c1114b4

Signed-off-by: Kushagra Srivastava <[email protected]>

mac_do(4): Debugging rules and exec_paths leak on destroy

d4e4cb4

Signed-off-by: Kushagra Srivastava <[email protected]>

OlCe2 reviewed Jul 4, 2025

View reviewed changes

thesynthax added 2 commits July 8, 2025 08:34

mac_do(4): Deep copy rules

21fe1cb

Signed-off-by: Kushagra Srivastava <[email protected]>

mac_do(4): Fixed leak

ac8902f

Signed-off-by: Kushagra Srivastava <[email protected]>

OlCe2 requested changes Aug 4, 2025

View reviewed changes

mac_do(4): fixed various bugs, structs inlined, leaks remain

d0264ff

Signed-off-by: Kushagra Srivastava <[email protected]>

OlCe2 requested changes Aug 11, 2025

View reviewed changes

thesynthax added 3 commits August 13, 2025 13:11

mac_do(4): MAC/do working in jail, leaks decreased

a5bcc68

Signed-off-by: Kushagra Srivastava <[email protected]>

mac_do(4): MAC/do fixed, works in host and jails, leaks removed

b1a7963

Signed-off-by: Kushagra Srivastava <[email protected]>

mac_do(4): style

14fdc49

Signed-off-by: Kushagra Srivastava <[email protected]>

thesynthax force-pushed the task/exec-paths-refactor branch from 9c25634 to 14fdc49 Compare August 23, 2025 11:50

		toast_rules(struct rules *const rules)
		toast_rules(struct rules const rules)

mac_do(4): Implemented new sysctl knob and jail param for allowed executable paths #2

Are you sure you want to change the base?

mac_do(4): Implemented new sysctl knob and jail param for allowed executable paths #2

Uh oh!

Conversation

thesynthax commented Jul 3, 2025

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OlCe2 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlCe2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

OlCe2 left a comment •

edited

Loading

github-actions bot commented Aug 21, 2025 •

edited

Loading