On Wed, Aug 28, 2013 at 02:54:11PM +0200, Maxime Villard wrote:
> Updated diff, with small tweaks from Andres Perera,
> * int -> size_t, signedness issue, even if it can't be >INT_MAX
> * NULL -> NUL
>
>
> Index: exec_elf.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/exec_elf.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 exec_elf.c
> --- exec_elf.c 4 Jul 2013 17:37:05 -0000 1.93
> +++ exec_elf.c 28 Aug 2013 14:36:58 -0000
> @@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> int error, i;
> char *interp = NULL;
> u_long pos = 0, phsize;
> - size_t randomizequota = ELF_RANDOMIZE_LIMIT;
> + size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
>
> if (epp->ep_hdrvalid < sizeof(Elf_Ehdr))
> return (ENOEXEC);
> @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
>
> for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> if (pp->p_type == PT_INTERP && !interp) {
> - if (pp->p_filesz >= MAXPATHLEN)
> + if (pp->p_filesz >= MAXPATHLEN ||
> + pp->p_filesz < 2)
Since p_filesz includes the NUL and MAXPATHLEN includes the NUL as
far as I know, could the comparison not be simply '> MAXPATHLEN'?
In passing I note that 37 out of 749 declarations using MAXPATHLEN in the
tree use 'char blah[MAXPATHLEN + 1]'. No idea if it is worth looking at
making them do without the '+ 1'. :-)
I also note in passing several "#define PATH_MAX (MAXPATHLEN + 1)'
declarations, which strike me as odd since MAXPATHLEN is defined
in sys/param.h by '#define MAXPATHEN PATH_MAX'.
Let the POSIX lawyers apply any necessary cluebats please.
> goto bad;
> interp = pool_get(&namei_pool, PR_WAITOK);
> if ((error = ELFNAME(read_from)(p, epp->ep_vp,
> pp->p_offset, interp, pp->p_filesz)) != 0) {
> + goto bad;
> + }
> + /* Ensure interp is a valid, NUL-terminated string */
The condition is not that the string is NUL terminated ("aaa\0aaa\0" is after
all NUL terminated and valid as far as any function reading strings would be
concerned. It just has trailing garbage.) Perhaps the comment should refer
to making sure the string is spec compliant by being of the expected length.
.... Ken
> + for (n = 0; n < pp->p_filesz; n++) {
> + if (interp[n] == '\0')
> + break;
> + }
> + if (n != pp->p_filesz - 1) {
> + error = ENOEXEC;
> goto bad;
> }
> } else if (pp->p_type == PT_LOAD) {
>
>
>
> On 08/28/13 08:44, Maxime Villard wrote:
> > Hi,
> > in the ELF format, the PT_INTERP segment contains the path of the
> > interpreter which must be loaded. For this segment, the kernel looks
> > at these values in the program header:
> >
> > p_offset: offset of the path string
> > p_filesz: size of the path string, including the \0
> >
> > The path string must be a valid string, or the binary won't be loaded.
> >
> > The kernel actually doesn't check the string, it just reads p_filesz
> > bytes from p_offset, and later it checks if it can be loaded. Malformed
> > binaries which have paths like:
> >
> > "aaaaaaaaaaaaaaaaaaaaa"
> > or
> > "aaa\0aaa\0aaa\0aaa\0"
> >
> > are parsed, loaded, and then the kernel figures out that the path is
> > not valid, and aborts.
> >
> > When the kernel reads the path from the file, it should check directly
> > the string to ensure it is at least a valid string.
> >
> > Here is a patch for this. For pp->p_filesz, I could have put
> > if (pp->p_filesz == 0)
> > but values of 1 also don't make sense; "\0" can't be a valid path.
> >
> >
> >
> > Index: exec_elf.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/exec_elf.c,v
> > retrieving revision 1.93
> > diff -u -p -r1.93 exec_elf.c
> > --- exec_elf.c 4 Jul 2013 17:37:05 -0000 1.93
> > +++ exec_elf.c 27 Aug 2013 22:59:21 -0000
> > @@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> > Elf_Ehdr *eh = epp->ep_hdr;
> > Elf_Phdr *ph, *pp, *base_ph = NULL;
> > Elf_Addr phdr = 0, exe_base = 0;
> > - int error, i;
> > + int error, i, n;
> > char *interp = NULL;
> > u_long pos = 0, phsize;
> > size_t randomizequota = ELF_RANDOMIZE_LIMIT;
> > @@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
> >
> > for (i = 0, pp = ph; i < eh->e_phnum; i++, pp++) {
> > if (pp->p_type == PT_INTERP && !interp) {
> > - if (pp->p_filesz >= MAXPATHLEN)
> > + if (pp->p_filesz >= MAXPATHLEN ||
> > + pp->p_filesz < 2)
> > goto bad;
> > interp = pool_get(&namei_pool, PR_WAITOK);
> > if ((error = ELFNAME(read_from)(p, epp->ep_vp,
> > pp->p_offset, interp, pp->p_filesz)) != 0) {
> > + goto bad;
> > + }
> > + /* Ensure interp is a valid, NULL-terminated string */
> > + for (n = 0; n < pp->p_filesz; n++) {
> > + if (interp[n] == '\0')
> > + break;
> > + }
> > + if (n != pp->p_filesz - 1) {
> > + error = ENOEXEC;
> > goto bad;
> > }
> > } else if (pp->p_type == PT_LOAD) {
> >
> >
> >
> > If you want to test
> > * Before patching
> > take whatever binary you have, open it with a text/hex editor, at the
> > beginning of the file there should be a string like
> > "/usr/libexec/ld.so",
> > just add a character to it, then run the binary with execv(): you will
> > get abort traps.
> > * After patching
> > execv() now returns -1, and errno is set to ENOEXEC.
> >
> > There should not be any regression; if there is, it means that the binary
> > is not spec compliant.
> >
> >
> > Ok/Comments?
> >
> >
>
