| Message ID | 878qybet6t.fsf_-_@oldenburg.str.redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | binfmt_elf: Fail execution of shared objects with ELIBEXEC (was: Re: [RFC PATCH v19 1/5] exec: Add a new AT_CHECK flag to execveat(2)) | expand |
Florian Weimer <fweimer@redhat.com> writes: > * Mickaël Salaün: > >> On Sat, Jul 06, 2024 at 05:32:12PM +0200, Florian Weimer wrote: >>> * Mickaël Salaün: >>> >>> > On Fri, Jul 05, 2024 at 08:03:14PM +0200, Florian Weimer wrote: >>> >> * Mickaël Salaün: >>> >> >>> >> > Add a new AT_CHECK flag to execveat(2) to check if a file would be >>> >> > allowed for execution. The main use case is for script interpreters and >>> >> > dynamic linkers to check execution permission according to the kernel's >>> >> > security policy. Another use case is to add context to access logs e.g., >>> >> > which script (instead of interpreter) accessed a file. As any >>> >> > executable code, scripts could also use this check [1]. >>> >> >>> >> Some distributions no longer set executable bits on most shared objects, >>> >> which I assume would interfere with AT_CHECK probing for shared objects. >>> > >>> > A file without the execute permission is not considered as executable by >>> > the kernel. The AT_CHECK flag doesn't change this semantic. Please >>> > note that this is just a check, not a restriction. See the next patch >>> > for the optional policy enforcement. >>> > >>> > Anyway, we need to define the policy, and for Linux this is done with >>> > the file permission bits. So for systems willing to have a consistent >>> > execution policy, we need to rely on the same bits. >>> >>> Yes, that makes complete sense. I just wanted to point out the odd >>> interaction with the old binutils bug and the (sadly still current) >>> kernel bug. >>> >>> >> Removing the executable bit is attractive because of a combination of >>> >> two bugs: a binutils wart which until recently always set the entry >>> >> point address in the ELF header to zero, and the kernel not checking for >>> >> a zero entry point (maybe in combination with an absent program >>> >> interpreter) and failing the execve with ELIBEXEC, instead of doing the >>> >> execve and then faulting at virtual address zero. Removing the >>> >> executable bit is currently the only way to avoid these confusing >>> >> crashes, so I understand the temptation. >>> > >>> > Interesting. Can you please point to the bug report and the fix? I >>> > don't see any ELIBEXEC in the kernel. >>> >>> The kernel hasn't been fixed yet. I do think this should be fixed, so >>> that distributions can bring back the executable bit. >> >> Can you please point to the mailing list discussion or the bug report? > > I'm not sure if this was ever reported upstream as an RFE to fail with > ELIBEXEC. We have downstream bug report: > > Prevent executed .so files with e_entry == 0 from attempting to become > a process. > <https://bugzilla.redhat.com/show_bug.cgi?id=2004942> > > I've put together a patch which seems to work, see below. > > I don't think there's any impact on AT_CHECK with execveat because that > mode will never get to this point. > > Thanks, > Florian > > ---8<----------------------------------------------------------------- > Subject: binfmt_elf: Fail execution of shared objects with ELIBEXEC > > Historically, binutils has used the start of the text segment as the > entry point if _start was not defined. Executing such files results > in crashes with random effects, depending on what code resides there. > However, starting with binutils 2.38, BFD ld uses a zero entry point, > due to commit 5226a6a892f922ea672e5775c61776830aaf27b7 ("Change the > linker's heuristic for computing the entry point for binaries so that > shared libraries default to an entry point of 0."). This means > that shared objects with zero entry points are becoming more common, > and it makes sense for the kernel to recognize them and refuse > to execute them. > > For backwards compatibility, if a load segment does not map the ELF > header at file offset zero, the kernel still proceeds as before, in > case the file is very non-standard and can actually start executing > at virtual offset zero. As written I find the logic of the patch confusing, and slightly wrong. The program header value e_entry is a virtual address, possibly adjusted by load_bias. Which makes testing it against the file offset of a PT_LOAD segment wrong. It needs to test against elf_ppnt->p_vaddr. I think performing an early sanity check to avoid very confusing crashes seems sensible (as long as it is inexpensive). This appears inexpensive enough that we don't care. This code is also before begin_new_exec so it is early enough to be meaningful. I think the check should simply test if e_entry is mapped. So a range check please to see if e_entry falls in a PT_LOAD segment. Having code start at virtual address 0 is a perfectly fine semantically and might happen in embedded scenarios. The program header is not required to be mapped or be first, (AKA p_offset and p_vaddr can have a somewhat arbitrary relationship) so any mention of the program header in your logic seems confusing to me. I think your basic structure will work. Just the first check needs to check if e_entry is lands inside the virtual address of a PT_LOAD segment. The second check should just be checking a variable to see if e_entry was inside any PT_LOAD segment, and there is no interpreter. Does that make sense? Eric > > Signed-off-by: Florian Weimer <fweimer@redhat.com> > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index a43897b03ce9..ebd7052eb616 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -830,6 +830,7 @@ static int load_elf_binary(struct linux_binprm *bprm) > unsigned long e_entry; > unsigned long interp_load_addr = 0; > unsigned long start_code, end_code, start_data, end_data; > + bool elf_header_mapped = false; > unsigned long reloc_func_desc __maybe_unused = 0; > int executable_stack = EXSTACK_DEFAULT; > struct elfhdr *elf_ex = (struct elfhdr *)bprm->buf; > @@ -865,6 +866,9 @@ static int load_elf_binary(struct linux_binprm *bprm) > continue; > } > > + if (elf_ppnt->p_type == PT_LOAD && !elf_ppnt->p_offset) > + elf_header_mapped = true; > + > if (elf_ppnt->p_type != PT_INTERP) > continue; > > @@ -921,6 +925,20 @@ static int load_elf_binary(struct linux_binprm *bprm) > goto out_free_ph; > } > > + /* > + * A zero value for e_entry means that the ELF file has no > + * entry point. If the ELF header is mapped, this is > + * guaranteed to crash (often even on the first instruction), > + * so fail the execve system call instead. (This is most > + * likely to happen for a shared object.) If the object has a > + * program interpreter, dealing with the situation is its > + * responsibility. > + */ > + if (elf_header_mapped && !elf_ex->e_entry && !interpreter) { > + retval = -ELIBEXEC; > + goto out_free_dentry; > + } > + > elf_ppnt = elf_phdata; > for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) > switch (elf_ppnt->p_type) {
* Eric W. Biederman: > As written I find the logic of the patch confusing, and slightly wrong. > > The program header value e_entry is a virtual address, possibly adjusted > by load_bias. Which makes testing it against the file offset of a > PT_LOAD segment wrong. It needs to test against elf_ppnt->p_vaddr. I think we need to test both against zero, or maybe invert the logic: if something is mapped at virtual address zero that doesn't come from a zero file offset, we disable the ELIBEXEC check. > I think performing an early sanity check to avoid very confusing crashes > seems sensible (as long as it is inexpensive). This appears inexpensive > enough that we don't care. This code is also before begin_new_exec > so it is early enough to be meaningful. Yeah, it was quite confusing when it was after begin_new_exec because the ELIBEXEC error is visible under strace, and then the SIGSEGV comes … > I think the check should simply test if e_entry is mapped. So a range > check please to see if e_entry falls in a PT_LOAD segment. It's usually mapped even with e_entry ==0 because the ELF header is loaded at virtual address zero for ET_DYN using the default linker flags (and this is the case we care about). With -z noseparate-code, it is even mapped executable. > Having code start at virtual address 0 is a perfectly fine semantically > and might happen in embedded scenarios. To keep supporting this case, we need to check that the ELF header is at address zero, because we make a leap of faith and assume it's not really executable even if it is mapped as such because due to its role in the file format, it does not contain executable instructions. That's why the patch is focused on the ELF header. I could remove all these checks and just return ELIBEXEC for a zero entry point. I think this is valid based on the ELF specification, but it may have a backwards compatibility impact. > The program header is not required to be mapped or be first, (AKA > p_offset and p_vaddr can have a somewhat arbitrary relationship) so any > mention of the program header in your logic seems confusing to me. It's the ELF header. > I think your basic structure will work. Just the first check needs to > check if e_entry is lands inside the virtual address of a PT_LOAD > segment. The second check should just be checking a variable to see if > e_entry was inside any PT_LOAD segment, and there is no interpreter. I think the range check doesn't help here. Just checking p_vaddr for zero in addition to p_offset should be sufficient. If you agree, can test and send an updated patch. Thanks, Florian
On Mon, Jul 08, 2024 at 06:37:14PM +0200, Florian Weimer wrote: > * Mickaël Salaün: > > > On Sat, Jul 06, 2024 at 05:32:12PM +0200, Florian Weimer wrote: > >> * Mickaël Salaün: > >> > >> > On Fri, Jul 05, 2024 at 08:03:14PM +0200, Florian Weimer wrote: > >> >> * Mickaël Salaün: > >> >> > >> >> > Add a new AT_CHECK flag to execveat(2) to check if a file would be > >> >> > allowed for execution. The main use case is for script interpreters and > >> >> > dynamic linkers to check execution permission according to the kernel's > >> >> > security policy. Another use case is to add context to access logs e.g., > >> >> > which script (instead of interpreter) accessed a file. As any > >> >> > executable code, scripts could also use this check [1]. > >> >> > >> >> Some distributions no longer set executable bits on most shared objects, > >> >> which I assume would interfere with AT_CHECK probing for shared objects. > >> > > >> > A file without the execute permission is not considered as executable by > >> > the kernel. The AT_CHECK flag doesn't change this semantic. Please > >> > note that this is just a check, not a restriction. See the next patch > >> > for the optional policy enforcement. > >> > > >> > Anyway, we need to define the policy, and for Linux this is done with > >> > the file permission bits. So for systems willing to have a consistent > >> > execution policy, we need to rely on the same bits. > >> > >> Yes, that makes complete sense. I just wanted to point out the odd > >> interaction with the old binutils bug and the (sadly still current) > >> kernel bug. > >> > >> >> Removing the executable bit is attractive because of a combination of > >> >> two bugs: a binutils wart which until recently always set the entry > >> >> point address in the ELF header to zero, and the kernel not checking for > >> >> a zero entry point (maybe in combination with an absent program > >> >> interpreter) and failing the execve with ELIBEXEC, instead of doing the > >> >> execve and then faulting at virtual address zero. Removing the > >> >> executable bit is currently the only way to avoid these confusing > >> >> crashes, so I understand the temptation. > >> > > >> > Interesting. Can you please point to the bug report and the fix? I > >> > don't see any ELIBEXEC in the kernel. > >> > >> The kernel hasn't been fixed yet. I do think this should be fixed, so > >> that distributions can bring back the executable bit. > > > > Can you please point to the mailing list discussion or the bug report? > > I'm not sure if this was ever reported upstream as an RFE to fail with > ELIBEXEC. We have downstream bug report: > > Prevent executed .so files with e_entry == 0 from attempting to become > a process. > <https://bugzilla.redhat.com/show_bug.cgi?id=2004942> Thanks for the info. > > I've put together a patch which seems to work, see below. > > I don't think there's any impact on AT_CHECK with execveat because that > mode will never get to this point. Correct, that is not an issue for AT_CHECK use cases.
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index a43897b03ce9..ebd7052eb616 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -830,6 +830,7 @@ static int load_elf_binary(struct linux_binprm *bprm) unsigned long e_entry; unsigned long interp_load_addr = 0; unsigned long start_code, end_code, start_data, end_data; + bool elf_header_mapped = false; unsigned long reloc_func_desc __maybe_unused = 0; int executable_stack = EXSTACK_DEFAULT; struct elfhdr *elf_ex = (struct elfhdr *)bprm->buf; @@ -865,6 +866,9 @@ static int load_elf_binary(struct linux_binprm *bprm) continue; } + if (elf_ppnt->p_type == PT_LOAD && !elf_ppnt->p_offset) + elf_header_mapped = true; + if (elf_ppnt->p_type != PT_INTERP) continue; @@ -921,6 +925,20 @@ static int load_elf_binary(struct linux_binprm *bprm) goto out_free_ph; } + /* + * A zero value for e_entry means that the ELF file has no + * entry point. If the ELF header is mapped, this is + * guaranteed to crash (often even on the first instruction), + * so fail the execve system call instead. (This is most + * likely to happen for a shared object.) If the object has a + * program interpreter, dealing with the situation is its + * responsibility. + */ + if (elf_header_mapped && !elf_ex->e_entry && !interpreter) { + retval = -ELIBEXEC; + goto out_free_dentry; + } + elf_ppnt = elf_phdata; for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) switch (elf_ppnt->p_type) {