On Wed, Aug 28, 2013 at 08:44:26PM +0200, Maxime Villard wrote:
> On 08/28/13 16:30, Kenneth R Westerback wrote:
> > 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.
>
>
> By 'valid string' I actually meant 'without trailing garbage'... but ok
> if it's not clear
> /* Ensure interp is NUL-terminated and of the expected length */
That would be clear to me.
.... Ken
>
>
> >
> > .... 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?
> >>>
> >>>
> >>
> >
>
