I would like to end this thread. Here's a final patch:
Index: exec_elf.c
===================================================================
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 -0000 1.93
+++ exec_elf.c 26 Nov 2013 18:36:35 -0000
@@ -552,11 +552,16 @@
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) {
To sum up what it does, and why it does what it does:
PT_INTERP segments are parts of PIE binaries, and specify a path to
an interpreter. This path's len is registered in the p_filesz field
of this segment, which includes the last NUL byte. The goal of this patch
is to check the length of this path to ensure it is consistent with
what is specified in p_filesz.
We do this, because otherwise the kernel may use a non-NUL-terminated
interp. It currently doesn't harm, since the copystr()'s that are later
called will stop the execution as well. But it is error-prone.
Ok/Opinions?
