[prev in list] [next in list] [prev in thread] [next in thread]
List: openbsd-tech
Subject: Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
From: Kenneth R Westerback <kwesterback () rogers ! com>
Date: 2013-08-29 2:34:26
Message-ID: 20130829023426.GT24710 () mac ! westerback ! ca
[Download RAW message or body]
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 <max@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...
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic