I'm a little torn on the idea itself, but here's some review

Using this /etc/jail.conf:

.include "/usr/local/etc/jail[.]conf";
.include "/etc/jail.d/*.conf";
.include "/usr/local/etc/jail.d/*.conf";

Place this script into /usr/local/etc/jail.d/dirs.conf:

#!/bin/sh
set -Ceu
cd "${1%/*}"
for dir in *.d; do
        [ -d "$dir" ] || continue
        name="${dir%.d}"
        printf '%s {\n\t.include "%s/*.conf";\n}\n' "$name" "$dir"
done

To write a block like this for each ${dir}.d jail directory, deriving the jail name from the directory name (stripping the .d suffix):

jail_name { # variable references aren't allowed in jail names
    .include "$name.d/*.conf";
}

It would make sense for the exec.clean parameter to apply to the config execution. Bit of a chicken and egg problem there, but there's still the "-l" flag.

But I'm still just not convinced this is a good idea. It's pretty far outside the norm of how configuration files are expected to work.

Replace fexecve(2) with execve(2) since fexecve(2) doesn't work with (shell) scripts unless /dev/fd is mounted with the non-standard "nodup" option.

See PR #294780 for more details why this change is required.

Switch to single line comments inside the open_file() function.

Reformatted the code to 80 columns.

As discussed putting this feature behind a paranoia flag to not cause issues for anyone that just happens to have an accidentally executable jail.conf(5) makes it effectively useless.

For some reason Phabricator doesn't register that I edited two additional files in the latest patch I uploaded and the raw diff doesn't include them either?!?

Since Phabricator ignores my attempts to update this review to modify multiple files with the diff I created a GitHub pull request at https://github.com/freebsd/freebsd-src/pull/2164 .

I had hoped to attend the jail user call today to be able to discuss this, but the Discord event has it an hour off and I didn't discover the authoritative source @ callfortesting.org until I was wondering why nobody was showing up. *deep sigh*

I don't think we're comfortable, as a project, to enable this for all users of jail(8) by default without an additional flag. I appreciate that you want to do stuff like this with existing jail scripts but this is a huge POLA violation (even assuming proper communication across a major branch update) with security implications, and I don't think enabling maybe-executable config scripts is a pattern that we really want propagating.

There was some discussion out-of-band after a concerned user reached out about this, and it was pointed out to me that automountd does the same thing, so I've pitched a review to try and neuter that a bit because that's terrifying: D56680.

In D46284#1298773, @kevans wrote:

I had hoped to attend the jail user call today to be able to discuss this, but the Discord event has it an hour off and I didn't discover the authoritative source @ callfortesting.org until I was wondering why nobody was showing up. *deep sigh*

I don't think we're comfortable, as a project, to enable this for all users of jail(8) by default without an additional flag. I appreciate that you want to do stuff like this with existing jail scripts but this is a huge POLA violation (even assuming proper communication across a major branch update) with security implications, and I don't think enabling maybe-executable config scripts is a pattern that we really want propagating.

There was some discussion out-of-band after a concerned user reached out about this, and it was pointed out to me that automountd does the same thing, so I've pitched a review to try and neuter that a bit because that's terrifying: D56680.

The jail.conf(5) format already defines hooks that by design execute as root on the host (exec.prepare/created/prestart/poststart/prestop/poststop/release). Having any untrusted jail.conf(5) on the system is a game over scenario similar to having a malicous /etc/crontab or rc.d script installed. Moving the attack surface a few milliseconds forward from the exec.prepare (or exec.prestop when removing a jail) stage to the config parsing stage doesn't increase the attack surface in a meaningful way.

Having the dynamic jail.conf process drop privs is good defense in depth when possible, but since it's sole function is to emit a jail.conf(5) snippet to standard output the security gains are minimal. Jail(8) will parse and run the jail.conf(5) including any shell hooks as root no matter which user the dynamic configuration ran as. Jail(8) also doesn't stop your when you put something stupid like .include "/tmp/dumb_idea.*.conf"; in your /etc/jail.conf.

As I discussed with Jamie: putting the executable jail.conf behind a command line flag would make this feature unusable with any existing script invoking jail(8) even the base system jail rc.d script. Requiring users to first create an explicit opt-in file in a hardcoded location writable only by root would be possible.

@kevans: Can you think of a realistic situation where someone will have their jail configuration unintentionally executable? I don't think chmod -R 777 /etc is a supported configuration. Are we forced to support FAT32 as root file system on any strange platform or something like that?

In D46284#1299021, @crest_freebsd_rlwinm.de wrote:

@kevans: Can you think of a realistic situation where someone will have their jail configuration unintentionally executable? I don't think chmod -R 777 /etc is a supported configuration. Are we forced to support FAT32 as root file system on any strange platform or something like that?

That's the wrong question. For security related things, you have to default to 'fail safe' and this feature fails to meet that criteria.

In D46284#1299031, @imp wrote:

In D46284#1299021, @crest_freebsd_rlwinm.de wrote:

@kevans: Can you think of a realistic situation where someone will have their jail configuration unintentionally executable? I don't think chmod -R 777 /etc is a supported configuration. Are we forced to support FAT32 as root file system on any strange platform or something like that?

That's the wrong question. For security related things, you have to default to 'fail safe' and this feature fails to meet that criteria.

Accepted. How do you want to make the opt-in explicit without cluttering/breaking the CLI interface? Would requiring root to to create /var/db/optin-dynamic-jail.conf be an acceptable safety net?

In D46284#1299021, @crest_freebsd_rlwinm.de wrote:

In D46284#1298773, @kevans wrote:

I had hoped to attend the jail user call today to be able to discuss this, but the Discord event has it an hour off and I didn't discover the authoritative source @ callfortesting.org until I was wondering why nobody was showing up. *deep sigh*

I don't think we're comfortable, as a project, to enable this for all users of jail(8) by default without an additional flag. I appreciate that you want to do stuff like this with existing jail scripts but this is a huge POLA violation (even assuming proper communication across a major branch update) with security implications, and I don't think enabling maybe-executable config scripts is a pattern that we really want propagating.

There was some discussion out-of-band after a concerned user reached out about this, and it was pointed out to me that automountd does the same thing, so I've pitched a review to try and neuter that a bit because that's terrifying: D56680.

The jail.conf(5) format already defines hooks that by design execute as root on the host (exec.prepare/created/prestart/poststart/prestop/poststop/release). Having any untrusted jail.conf(5) on the system is a game over scenario similar to having a malicous /etc/crontab or rc.d script installed. Moving the attack surface a few milliseconds forward from the exec.prepare (or exec.prestop when removing a jail) stage to the config parsing stage doesn't increase the attack surface in a meaningful way.

I can audit static config in an automated manner for these things being set and bail out if they're unsafe, I can't easily do that if they're being produced by something we execute past the point of no return.

Having the dynamic jail.conf process drop privs is good defense in depth when possible, but since it's sole function is to emit a jail.conf(5) snippet to standard output the security gains are minimal. Jail(8) will parse and run the jail.conf(5) including any shell hooks as root no matter which user the dynamic configuration ran as. Jail(8) also doesn't stop your when you put something stupid like .include "/tmp/dumb_idea.*.conf"; in your /etc/jail.conf.

As I discussed with Jamie: putting the executable jail.conf behind a command line flag would make this feature unusable with any existing script invoking jail(8) even the base system jail rc.d script. Requiring users to first create an explicit opt-in file in a hardcoded location writable only by root would be possible.

Can you provide a little more detail here for the 'existing scripts' you're referencing? I'm not sure I see where the jail rc.d script is a problem, my recollection is that it honors jail_flags in rc.conf (and maybe even a few other variants of that name) and those can easily include a new flag to enable it.

@kevans: Can you think of a realistic situation where someone will have their jail configuration unintentionally executable? I don't think chmod -R 777 /etc is a supported configuration. Are we forced to support FAT32 as root file system on any strange platform or something like that?

You don't really have to use it as a rootfs, it's sufficient to have a flash drive with your config backed up that you tried to restore from without thinking of the consequences, because this is a really subtle behavior.