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)
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 */
+ 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?
>
>
