On Wed, Aug 28, 2013 at 09:43:24PM +0200, Maxime Villard wrote: > On 08/28/13 20:57, Matthew Dempsky wrote: > > On Wed, Aug 28, 2013 at 5:54 AM, Maxime Villard <m...@m00nbsd.net> wrote: > >> + /* Ensure interp is a valid, NUL-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; > >> } > > > > A more concise test would be: > > > > if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) { > > error = ENOEXEC; > > goto bad; > > } > > > > Hum you're right, it's better that way > > > 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 21:14:22 -0000 > @@ -552,11 +552,16 @@ 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 < 2 || pp->p_filesz >= MAXPATHLEN)
Still think you're depriving yourself of one character out by using ">=" instead of ">". .... Ken > 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 NUL-terminated and of the expected > length */ > + if (strnlen(interp, pp->p_filesz) != pp->p_filesz - 1) { > + error = ENOEXEC; > goto bad; > } > } else if (pp->p_type == PT_LOAD) { > > > It no longer looks like my patch... >