Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated

[Maxime Villard]

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