kern: imgact: fix imgp->interpreted

[kevans]

Summary

This is a mask, so the new value should have taken the next bit to avoid
breaking a shell script that's interpreted by a binmisc-activated
interpreter.

While we're here, compose a little better with the rest of the
activators by setting the bit rather than clobbering it to be sure that
we wouldn't disturb a case like:

<binary> -> imgact_binmisc -> native elf

We should probably add tests for these things.

Fixes: 389c124fecb0 ("imgact_elf.c indicate that interpreter [...]")

[kevans]

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

[sjg]

In D58063#1331516, @kevans wrote:

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

I don't think so. The original problem was that when exec'ing ld-elf.so as part of running an ELF binary, the 'interpreted' field was not set so if we set veriexec's indirect flag on ld-elf.so, mac_veriexec thinks it is a direct exec and blocks it - which is sub-optimal. Simply having imgp->interpreted != 0 is sufficient to address that.

[kevans]

In D58063#1331519, @sjg wrote:

In D58063#1331516, @kevans wrote:

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

I don't think so. The original problem was that when exec'ing ld-elf.so the 'interpreted' field was not set so if we set veriexec's indirect flag on ld-elf.so, mac_veriexec thinks it is a direct exec and blocks it - which is sub-optimal. Simply having imgp->interpreted != 0 is sufficient to address that.

Right, but as far as I can tell it won't work right in vanilla FreeBSD. We set imgp->interpreted = 0 here:

https://cgit.freebsd.org/src/tree/sys/kern/imgact_elf.c#n1461

After you would have set it non-zero up in load_interp -- I think clearing it needs to move above the interp != NULL branch, or the |= here changes back to = and we just clear it in the else block (interp == NULL).

[sjg]

In D58063#1331520, @kevans wrote:

In D58063#1331519, @sjg wrote:

In D58063#1331516, @kevans wrote:

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

I don't think so. The original problem was that when exec'ing ld-elf.so the 'interpreted' field was not set so if we set veriexec's indirect flag on ld-elf.so, mac_veriexec thinks it is a direct exec and blocks it - which is sub-optimal. Simply having imgp->interpreted != 0 is sufficient to address that.

Right, but as far as I can tell it won't work right in vanilla FreeBSD. We set imgp->interpreted = 0 here:

https://cgit.freebsd.org/src/tree/sys/kern/imgact_elf.c#n1461

After you would have set it non-zero up in load_interp -- I think clearing it needs to move above the interp != NULL branch, or the |= here changes back to = and we just clear it in the else block (interp == NULL).

There are some diffs b/w imgact_elf.c in main and our tree (we're a bit behind), but wrt above it looks the same, and it works fine.

[kevans]

In D58063#1331523, @sjg wrote:

In D58063#1331520, @kevans wrote:

In D58063#1331519, @sjg wrote:

In D58063#1331516, @kevans wrote:

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

I don't think so. The original problem was that when exec'ing ld-elf.so the 'interpreted' field was not set so if we set veriexec's indirect flag on ld-elf.so, mac_veriexec thinks it is a direct exec and blocks it - which is sub-optimal. Simply having imgp->interpreted != 0 is sufficient to address that.

Right, but as far as I can tell it won't work right in vanilla FreeBSD. We set imgp->interpreted = 0 here:

https://cgit.freebsd.org/src/tree/sys/kern/imgact_elf.c#n1461

After you would have set it non-zero up in load_interp -- I think clearing it needs to move above the interp != NULL branch, or the |= here changes back to = and we just clear it in the else block (interp == NULL).

There are some diffs b/w imgact_elf.c in main and our tree (we're a bit behind), but wrt above it looks the same, and it works fine.

Interesting, OK- I don't see how imgp->interpreted could possibly preserve the bit in question until it gets to a MAC check, but I think I'll punt on that and just leave this patch as-is for now. In the short-to-medium term, my plan is to rewrite the imgact_binmisc configuration interface to allow rules scoped to jails; at that point, I'll write some tests for how all of these things work together and maybe tweak stuff further.

[kevans]

In D58063#1331879, @kevans wrote:

In D58063#1331523, @sjg wrote:

There are some diffs b/w imgact_elf.c in main and our tree (we're a bit behind), but wrt above it looks the same, and it works fine.

Interesting, OK- I don't see how imgp->interpreted could possibly preserve the bit in question until it gets to a MAC check, but I think I'll punt on that and just leave this patch as-is for now. In the short-to-medium term, my plan is to rewrite the imgact_binmisc configuration interface to allow rules scoped to jails; at that point, I'll write some tests for how all of these things work together and maybe tweak stuff further.

I wanted to spend some time understanding this, and I feel better now. Basically, I didn't notice that the imgp we're setting IMGACT_INTERP_ELF on isn't at all related to the one back in kern_exec.c, it's solely for the transient tempdata. That part of the original patch appears to be strictly unnecessary (or could be an assertion), because we don't propagate tempdata->image_params back up to the exec image_params in any way. Veriexec works fine because you only care about __elfN(load_interp_file) -> exec_check_permissions -> mac_vnode_check_exec.

I'll remove the |= bit of this patch and related commentary in the commit message. I won't change the kern_exec.c part of the patch, but I do want to think about whether it would be useful for imgp->interpreted to reflect all of the interpreters involved in the chain -- it could be that it's useful to set IMGACT_INTERP_ELF in imgp->interpreted back where __elfN(load_interp) is first called in the elf activator. I'm not sure I see how at the moment, though:

• mac_execve_exit just frees the imgp->execlabel
• The PT_INTEPR can't itself be interpreted, so this is a pretty final state
• I don't really see anything in the area where it could be set that it might be of use

[sjg]

I'll remove the |= bit of this patch and related commentary in the commit message. I won't change the kern_exec.c part of the patch, but I do want to think about whether it would be useful for imgp->interpreted to reflect all of the interpreters involved in the chain -- it could be that it's useful to set IMGACT_INTERP_ELF in imgp->interpreted back where __elfN(load_interp) is first called in the elf activator. I'm not sure I see how at the moment, though:

My understanding may be incorrect, but if there is an interpreter chain, I would think kern_exec is only dealing with them one after another no
so not sure |= is helping.

[kevans]

In D58063#1332290, @sjg wrote:

I'll remove the |= bit of this patch and related commentary in the commit message. I won't change the kern_exec.c part of the patch, but I do want to think about whether it would be useful for imgp->interpreted to reflect all of the interpreters involved in the chain -- it could be that it's useful to set IMGACT_INTERP_ELF in imgp->interpreted back where __elfN(load_interp) is first called in the elf activator. I'm not sure I see how at the moment, though:

My understanding may be incorrect, but if there is an interpreter chain, I would think kern_exec is only dealing with them one after another no
so not sure |= is helping.

We loop back to the beginning and run through the activator list again with the interpreter, so the common thing here is we set IMGACT_SHELL, jump back to the interpret label, then end up in either the elf or binmisc activators. The binmisc activator will do one more jump back to interpret and land on an ELF (probably static), at which point imgp->interpreted gets zapped for the do_execve imgp.

A binmisc ->. binmisc loop is probably a valid concern and the activators are written to avoid it, so I could see why we maintain the bits most of the way through.