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...
> 

Reply via email to