Fix a bug that incorrectly reports GNU_STACK missing when its not.

[devx00]

When determining whether or not NX is set for amd64 binaries, checksec was
incorrectly reporting that GNU_STACK was missing.

During my investigation of that problem I became aware that the logic to
determine whether or not NX was enabled was also incorrect.

I believe the author of #2186 incorrectly understood the logic that determines
whether or not NX is enabled on the stack. I have compiled the evidence that led
me to that belief below. It is quite possible I am the one missing some context
but I hopefully included enough information to make it easy to validate my
claims.

In #2186 the author uses the code changed in this commit
as the basis for their reasoning, however they seem to imply that code directly
changes the possible states of NX, and makes it permanently enabled on amd64.

For amd64, although read_implies_exec is always false for kernel version >=
5.8, that does not mean nx is always enabled for kernel version >= 5.8. The
kernel still honors GNU_STACK=+x as seen below:

---

In fs/binfmt_elf.c line 925, in the function load_elf_binary, the kernel
checks the GNU_STACK segment to determine if the executable bit is set for the
segment. If it is the kernel sets executable_stack to EXSTACK_ENABLE_X.

reference

// fs/binfmt_elf.c:925

// static int load_elf_binary(struct linux_binprm *bprm)
// ...

for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++)
	switch (elf_ppnt->p_type) {
	case PT_GNU_STACK:
		if (elf_ppnt->p_flags & PF_X)
			executable_stack = EXSTACK_ENABLE_X;
		else
			executable_stack = EXSTACK_DISABLE_X;
		break;

	case PT_LOPROC ... PT_HIPROC:
		retval = arch_elf_pt_proc(elf_ex, elf_ppnt,
					  bprm->file, false,
					  &arch_state);
		if (retval)
			goto out_free_dentry;
		break;
	}

Then on line 1000 of fs/binfmt_elf.c, still in load_elf_binary, the kernel
uses the read_implies_exec macro to determine whether or not to set the
READ_IMPLIES_EXEC flag to the personality. It does not effect the
executable_stack variable. We see on line 1010 that the executable_stack
variable is passed to setup_arg_pages which is used to set the stack
permissions.

reference

// fs/binfmt_elf.c:1000

/* Do this immediately, since STACK_TOP as used in setup_arg_pages
   may depend on the personality.  */
SET_PERSONALITY2(*elf_ex, &arch_state);
if (elf_read_implies_exec(*elf_ex, executable_stack))
	current->personality |= READ_IMPLIES_EXEC;

const int snapshot_randomize_va_space = READ_ONCE(randomize_va_space);
if (!(current->personality & ADDR_NO_RANDOMIZE) && snapshot_randomize_va_space)
	current->flags |= PF_RANDOMIZE;

setup_new_exec(bprm);

/* Do this so that we can load the interpreter, if need be.  We will
   change some of these later */
retval = setup_arg_pages(bprm, randomize_stack_top(STACK_TOP),
			 executable_stack);
if (retval < 0)
	goto out_free_dentry;

Looking at the setup_arg_pages function in fs/exec.c, we see that the
executable_stack variable is used to set the VM_EXEC flag of vm_flags, and
then, finally, the vm_flags get passed to mprotect_fixup on line 790. It
will cause a warning to fire but all that will happen is a warning written to
the system log.

reference

// fs/exec.c:775
/*
 * Adjust stack execute permissions; explicitly enable for
 * EXSTACK_ENABLE_X, disable for EXSTACK_DISABLE_X and leave alone
 * (arch default) otherwise.
 */
if (unlikely(executable_stack == EXSTACK_ENABLE_X))
	vm_flags |= VM_EXEC;
else if (executable_stack == EXSTACK_DISABLE_X)
	vm_flags &= ~VM_EXEC;
vm_flags |= mm->def_flags;
vm_flags |= VM_STACK_INCOMPLETE_SETUP;

vma_iter_init(&vmi, mm, vma->vm_start);

tlb_gather_mmu(&tlb, mm);
ret = mprotect_fixup(&vmi, &tlb, vma, &prev, vma->vm_start, vma->vm_end,
		vm_flags);
tlb_finish_mmu(&tlb);

if (ret)
	goto out_unlock;
BUG_ON(prev != vma);

if (unlikely(vm_flags & VM_EXEC)) {
	pr_warn_once("process '%pD4' started with executable stack\n",
		     bprm->file);
}

pr_warn_once is just a kernel log statement. Nothing else.

reference

// include/linux/printk.h:647

#define pr_warn_once(fmt, ...)					\
	printk_once(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)

---

As far as I can tell, nothing forces NX when it is explicitly disabled on amd64.
I read through the issue thread and the PR thread where these changes were
explained but I don't see an explanation for why we were returning None when
GNU_STACK was present and it contained the X bit.

In addition, I am changing the return value when GNU_STACK is missing to
return True. We can assume that NX is set if GNU_STACK is not present even
though some older systems don't support NX, because those systems will just
ignore the NX bit anyways.

As this kernel commit message,
which was linked in the original PR thread, states:

With modern x86 64-bit environments, there should never be a need for
automatic READ_IMPLIES_EXEC, as the architecture is intended to always be
execute-bit aware (as in, the default memory protection should be NX unless a
region explicitly requests to be executable).

There were very old x86_64 systems that lacked the NX bit, but for those, the
NX bit is, obviously, unenforceable, so these changes should have no impact on
them.

I think it should be up to the user to determine whether or not NX will be
honored or not on the system they are attacking, and we should not let the rare
chance of a user targeting an old system, which doesn't support NX, make the
output of checksec less clear for the majority.

[devx00]

I am unsure how to correctly amend the CHANGELOG. Does this go under a new version number or the current released version?

Nvmd I believe I figured it out.

[peace-maker]

I am unsure how to correctly amend the CHANGELOG. Does this go under a new version number or the current released version?

Nvmd I believe I figured it out.

We should start a new 4.13.2 changelog! The change sounds reasonable, but @Arusekk has a better overview of this.

[Arusekk]

NX is a protection against data execution. Linux has had its quirks, but by default (GNU_STACK missing) NX is disabled on 'old' architectures (for compat: before NX was a thing, executables did not mark anything 'executable', not even text; they would crash if NX was on), but enabled on 'new' arches. If there is any GNU_STACK (even 'executable stack'), the kernel always enables NX. amd64 used to be an 'old' arch, but this changed to 'new' recently, hence the None value.

By NX in the ELF class we mean enabling any NX at all (even for the heap).

[devx00]

If there is any GNU_STACK (even 'executable stack'), the kernel always enables NX. amd64 used to be an 'old' arch, but this changed to 'new' recently, hence the None value.

Ok I see the problem. It is still mis-reporting GNU_STACK as missing as a result of returning None but I understand the change now. I'll amend my PR.

I think reporting Unknown for the nx state here is unhelpful. Since we know the exact conditions that would determine whether it's enabled or not why not report those conditions to the user so they can determine the state in their situation?

I will update my PR to fix the misreporting of GNU_STACK presence and if I have time, and can find a clean way to do so, I will add the additional context if that is something you will approve.

[devx00]

Why is non_exec in that function only referring to the GNU_STACK segments? Shouldn't it just check all segments and be True if any are marked non exec?

That's the part that made me assume NX here was referring specifically to the stack.

[Arusekk]

TBH I think you should ask Linux devs on the rationale behind READ_IMPLIES_EXEC handling. We just mimic the kernel logic here.

[devx00]

I am unsure how to correctly amend the CHANGELOG. Does this go under a new version number or the current released version?
Nvmd I believe I figured it out.

We should start a new 4.13.2 changelog! The change sounds reasonable, but @Arusekk has a better overview of this.

I added it. Not sure if you wanted me to move the stable tag to the new one or if you move that when its released so I can change it if necessary.

I reverted all the logic changes, and just fixed the issue I initially set out to fix lol.

Thanks for the insight @Arusekk I learned a lot and I also should have realized at least some of that myself before I went through all that.

I am confident this final change is correct though because the only architecture that even returns None when GNU_STACK is missing is amd64 and even then its not the only reason None may be returned.

[devx00]

And just for good measure here is an example of it misreporting.