On Wed, 2024-09-04 at 19:04 +0200, Jann Horn wrote:
> [necrothreading...]
> [+IMA folks]
>
> On Fri, May 31, 2024 at 3:01 PM Christian Brauner <[email protected]> wrote:
> > Back in 2021 we already discussed removing deny_write_access() for
> > executables. Back then I was hesistant because I thought that this might
> > cause issues in userspace. But even back then I had started taking some
> > notes on what could potentially depend on this and I didn't come up with
> > a lot so I've changed my mind and I would like to try this.
> [snip]
> > Yes, someone in userspace could potentially be relying on this. It's not
> > completely out of the realm of possibility but let's find out if that's
> > actually the case and not guess.
>
> FYI, ima_bprm_check() still has a comment that claims that executables
> use deny_write_access():
>
> /**
> * ima_bprm_check - based on policy, collect/store measurement.
> * @bprm: contains the linux_binprm structure
> *
> * The OS protects against an executable file, already open for write,
> * from being executed in deny_write_access() and an executable file,
> * already open for execute, from being modified in get_write_access().
> * So we can be certain that what we verify and measure here is actually
> * what is being executed.
> *
> * On success return 0. On integrity appraisal error, assuming the file
> * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
> */
>
> But what actually happens in there is not so different from what
> happens in ima_file_mmap(), so I think probably the only change
> required here is to fix up the comment...
We need to do the violation check for the BPRM_CHECK IMA hook too:
violation_check = ((func == FILE_CHECK || func == MMAP_CHECK
||
func == MMAP_CHECK_REQPROT) &&
(ima_policy_flag & IMA_MEASURE));
Roberto
