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 */
>
> .... 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?
>>>
>>>
>>
>
