Re: fs/binfmt_elf: Integer Overflow vulnerability report

[Greg KH <greg@kroah.com>]

On Mon, Aug 16, 2021 at 07:50:39AM +0300, Itay Iellin wrote:
> Bcc: 
> Subject: fs/binfmt_elf: Integer Overflow vulnerability report
> Reply-To: 
> I'm sharing a report of an integer overflow vulnerability I found (in 
> fs/binfmt_elf.c). I sent and discussed this vulnerability report with members
> of security@kernel.org. I'm raising this for public discussion, with approval
> from Greg (greg@kroah.com).
> 
> On Sun, Aug 01, 2021 at 04:30:30PM +0300, Itay Iellin wrote:
> > In fs/binfmt_elf.c, line 1193, e_entry value can be overflowed. This
> > potentially allows to create a fake entry point field for an ELF file.
> > 
> > The local variable e_entry is set to elf_ex->e_entry + load_bias.
> > Given an ET_DYN ELF file, without a PT_INTERP program header, with an 
> > elf_ex->e_entry field in the ELF header, which equals to
> > 0xffffffffffffffff(in x86_64 for example), and a load_bias which is greater 
> > than 0, e_entry(the local variable) overflows. This bypasses the check of 
> > BAD_ADDR macro in line 1241.
> > 
> > It is possible to set a large enough NO-OP(NOP) sled, before the
> > actual code, modify the elf_ex->e_entry field so that elf_ex->e_entry+load_bias
> > will be in the range where the NO-OP sled is mapped(because the offset
> > of the PT_LOAD program header of the text segment can be controlled). 
> > This is practically a guess, because load_bias is randomized, the ELF file can
> > be loaded a large amount of times until elf_ex->e_entry + load_bias 
> > is in the range of the NO-OP sled.
> > To conclude, this bug potentially allows the creation of a "fake" entry point
> > field in the ELF file header. 
> > 
> > Suggested git diff:
> > 
> > Add a BAD_ADDR test to elf_ex->e_entry to prevent from using an
> > overflowed elf_entry value.
> > 
> > Signed-off-by: Itay Iellin <ieitayie@gmail.com>
> > ---
> >  fs/binfmt_elf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 439ed81e755a..b59dcd5857db 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1238,7 +1238,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
> >  		kfree(interp_elf_phdata);
> >  	} else {
> >  		elf_entry = e_entry;
> > -		if (BAD_ADDR(elf_entry)) {
> > +		if (BAD_ADDR(elf_entry) || BAD_ADDR(elf_ex->e_entry)) {
> >  			retval = -EINVAL;
> >  			goto out_free_dentry;
> >  		}
> > -- 
> > 2.32.0
> > 
> 
> I am not attaching the replies to my initial report from the discussion with
> members of security@kernel.org, only when or if I will be given permission
> from the repliers to do so.

The replies can be summarized as "are you sure this is an issue, and
if so, that this is really the correct fix at all?"

So I think you need to provide a bit more information here as to why
this really is a problem and how this would not harm valid elf programs.

thanks,

greg k-h