Add options for enforcing W^X

[jtl]

Summary

It seems useful to have several options for enforcing W^X. Silently blocking the allocations may lead to hidden cases of mis-behavior or changed behavior in applications.

This patch adds options to log violations (but allow the allocation to proceed anyway), block the violations, or even kill the processes (to allow a developer to pinpoint the place the W^X violation is occurring). These options can aid in evaluating the possibility of enabling W^X and in debugging W^X violations.

I chose not to maintain an option to silently block allocations. We could certainly add that, if that seems like a good idea. However, it seems like a system administrator who enables W^X protections probably would expect no regular ongoing violations would be occurring. Therefore, it seemed like it was useful to log the violations. But, I am open to hearing differing opinions.

Because this patch adds an option to choose to enforce or not enforce W^X globally, it also changes the default for ELF to enforcing W^X. The patch also checks for W^X violations in the ELF sections.

Test Plan

I ran a program with W^X violations in all 4 modes and saw the expected behavior occur.

[imp]

A couple of nits, but otherwise looks quite reasonable.

FreeBSD/sys/kern/imgact_elf.c

223 ↗

(On Diff #110618)

This likely warrants both a 'Relnote: yes' and a blurb in UPDATING.
Please also update security.7 maybe (though it is currently silent on what the default is, so maybe not)

752 ↗

(On Diff #110618)

I'd be tempted to add 'and bad protection mask' or something to the end.

FreeBSD/sys/vm/vm_map.c

1607 ↗

(On Diff #110618)

This should be added to security.7 man page

[jtl]

Changes suggested in the review:

• Added entry to UPDATING.
• Updated security(7) man page.

[kib]

FreeBSD/sys/kern/imgact_elf.c

223 ↗

(On Diff #110618)

This is premature IMO

751 ↗

(On Diff #110618)

Look how other places (in this file) handle activator' failures.

FreeBSD/sys/vm/vm_map.c

1627 ↗

(On Diff #110618)

curproc is curthread->td_proc, so it is better to cache td = curthread and then use td.
P_SYSTEM, PSR_NEW, and perhaps init should be excluded instead of kernel processes.

1629 ↗

(On Diff #110618)

kern_psignal requires process lock, but I think that the signal should be sent to the offending thread and not to the process.

In fact, sending a raw signal is not informative. Look at siginfo_t and reporting of the causes. Also look how SIGTRAP is generated for ENOTCAPABLE, perhaps there should be used something like that instead of overloading SIGSEGV.

Also, I do not like the verbosity by default, kernel should stay silent unless explicitly asked to talk.

[jtl]

Made changes based on review:

• Switch from syslog to uprintf() for reporting ELF W^X failure.
• Separate W^X logging and enforcement.
• Change test for which processes to signal. (Now, system processes, processes which are still starting, and kernel threads are exempt.)
• Switch from using SIGSEGV to SIGTRAP with a descriptive code.

[jtl]

FreeBSD/sys/kern/imgact_elf.c

223 ↗

(On Diff #110618)

Do you mind explaining your reasoning? I switched this due to concerns about user experience: needing to turn on two controls by default seemed like it could be too confusing. And, my personal experience with this feature makes me think that a system administrator wanting to adopt W^X protection may want to use the more fine-grained logging/enforcement controls in the VM system. That is why I thought it would be better to turn this one on by default, but leave the enforcement in the VM system disabled. However, I'd like to understand your reasoning, since I could very easily be mistaken in my thinking.

751 ↗

(On Diff #110618)

Thanks! I think I've made the change you requested. If not, let me know.

FreeBSD/sys/vm/vm_map.c

1627 ↗

(On Diff #110618)

I've made this change. I verified that init is marked with P_SYSTEM, so it should be caught.

I ended up using the test != PRS_NEW; however, I almost made it == PRS_NORMAL. Let me know if you prefer the second test.

1629 ↗

(On Diff #110618)

I believe I've made the changes you suggested. Let me know if I've missed anything.

[kib]

IMO there is too much controls now for wxorx. There is proccontrol -m wxmap, and essentially adding yet another knob breaks it.
Also I am not sure that we want/need logging for mmap/mprotect violating the policy. It is a developer's option, not an administrator.

What I do like from your patch is the SIGTRAP/TRAP_WXORX, which clearly needs its own knob. But I suggest to remove logging.

WRT my comment of changing the default to be premature, I remember that an exp run was taken by Ed (?, added emaste to reviewers), and the result was that most high-profile jit using programs failed.

[jtl]

In D36595#831026, @kib wrote:

IMO there is too much controls now for wxorx. There is proccontrol -m wxmap, and essentially adding yet another knob breaks it.

I'll look into this a little more.

Also I am not sure that we want/need logging for mmap/mprotect violating the policy. It is a developer's option, not an administrator.

My reason for adding this was based on my own experience trying to deploy W^X protection. The first thing I wanted to know was which programs I run this might break. Having a logging option allowed me to see this (as an administrator). Having the signal allowed me to more easily find where the call was buried (as a developer).

WRT my comment of changing the default to be premature, I remember that an exp run was taken by Ed (?, added emaste to reviewers), and the result was that most high-profile jit using programs failed.

I don't doubt this. Problems like this are why my patch still leaves the protection disabled by default. I just moved the place it was disabled from the ELF level to the VM level. The only thing that used to work which will now break is an ELF file which has a section with RWX protection and does not have the special control flag to allow WX.

[jtl]

In D36595#831110, @jtl wrote:

In D36595#831026, @kib wrote:

IMO there is too much controls now for wxorx. There is proccontrol -m wxmap, and essentially adding yet another knob breaks it.

I'll look into this a little more.

What we have currently is one set of controls which selects the processes for which we are going to enforce W^X:

• the ELF allow_wx sysctls
• procctl -m wxmap
• (any others?)

What I am proposing is that we have two sets of controls:

• The existing set which selects processes for which we are going to consider enforcing W^X
• A new set of controls which selects the enforcement mechanism we will apply (which could include not enforcing it)

I believe the above separation is useful for system administrators who are trying to transition to a W^X configuration. We could probably collapse this to a single set of controls which applies on a per-process basis. However, I'm not sure whether that would be easier or harder to understand (either as a kernel developer or as a user).

[kib]

In D36595#831136, @jtl wrote:

In D36595#831110, @jtl wrote:

In D36595#831026, @kib wrote:

IMO there is too much controls now for wxorx. There is proccontrol -m wxmap, and essentially adding yet another knob breaks it.

I'll look into this a little more.

What we have currently is one set of controls which selects the processes for which we are going to enforce W^X:

• the ELF allow_wx sysctls
• procctl -m wxmap
• (any others?)

What I am proposing is that we have two sets of controls:

• The existing set which selects processes for which we are going to consider enforcing W^X
• A new set of controls which selects the enforcement mechanism we will apply (which could include not enforcing it)

I believe the above separation is useful for system administrators who are trying to transition to a W^X configuration. We could probably collapse this to a single set of controls which applies on a per-process basis. However, I'm not sure whether that would be easier or harder to understand (either as a kernel developer or as a user).

I do not object against this. Even more, I believe that SIGTRAP reporting unifies two enforcement mechanisms, for capsicum and wxorx, and this is why I like it.

What I think excessive is the kernel verbosity, i.e. logging of the violations, instead of returning error/sending signal. In fact, even image activator logging is arguably excessive because wx mapping cannot be created if wxorx policy is enforced. But it is somewhat useful due to inability of the process to catch signals after its vmspace is destroyed and not yet recreated.

[jtl]

Added a section to the security(7) man page in an attempt to clarify the controls which will be available if this change is merged.

[imp]

I'm still thinking about the documentation... I think the code is OK, modulo a couple of nits.

sys/vm/vm_map.c

1607

Does '2' deny mapping and send a SIGTRAP? Or just the SIGTRAP? The code looks like 'both' below.
As such I'd suggest 'deny and send trap to process'.

1643

nit: I think this is an extra blank line.
/* FALLTHROUGH */ seems more common as well, since it originated with LINT, but I think either gcc or clang can be made to issue a warning when it isn't present and there's no break;.

1650

extra newline

[markj]

I like the change overall. I don't have strong feelings about supporting logging from the kernel. We don't do it for, e.g., Capsicum, but perhaps that'd be useful.

UPDATING

35 ↗

(On Diff #110807)

This diff also changes a default in that executables with WX ELF segments won't be loaded. I suspect this should be documented as well, and I'd suggest changing the default in a separate commit.

share/man/man7/security.7

950

Perhaps expand that here to "Write XOR eXecute" to make it clearer where the name comes from.

[jtl]

Addressed review feedback.
Restored defaults.

[jtl]

UPDATING

35 ↗

(On Diff #110807)

Both of these comments make sense. I've reverted the part of the diff which changed the default behavior. It might be helpful to seek broader input on a mailing list before changing the defaults the way I suggested.

share/man/man7/security.7

950

I made the suggested change.

sys/vm/vm_map.c

1607

It tries to send a SIGTRAP, but will also deny the mapping. I've made the suggested change.

1643

Fixed.

1650

Fixed.

[jtl]

Fix the man page description of the vm.enforce_wxorx sysctl/tunable to indicate that a value of 2 both denies mappings and sends a signal.

[kib]

I still do not like the extra verbosity from kernel. If insisting on adding more kernel messages, you could make logging for SIGTRAP with interesting si_codes, covering both wxorx and capsicum in one shot.

[jtl]

In D36595#832812, @kib wrote:

I still do not like the extra verbosity from kernel. If insisting on adding more kernel messages, you could make logging for SIGTRAP with interesting si_codes, covering both wxorx and capsicum in one shot.

If I understand your suggested change correctly, it will not produce the same behavior as I have proposed. I only intended to send a signal to the process for debugging purposes. In normal use, I expect a system administrator would do what I did when I wanted to enable W^X:

1. Log violations, but do not actually enforce W^X. This gives a system administrator a chance to assess how many programs may work differently if W^X enforcement is enabled.
2. Optionally, use signals to debug where the actual violations are in the code so they can either change the code or understand the implications of denying the mapping.
3. Enable enforcement (with or without logging, according to their preference), which would simply deny such a mapping (without signaling the process).

As described above, I don't propose to always send a SIGTRAP to the user process. But, if I've misunderstood your proposal, please feel free to correct me.

[kib]

In D36595#833573, @jtl wrote:

In D36595#832812, @kib wrote:

I still do not like the extra verbosity from kernel. If insisting on adding more kernel messages, you could make logging for SIGTRAP with interesting si_codes, covering both wxorx and capsicum in one shot.

If I understand your suggested change correctly, it will not produce the same behavior as I have proposed. I only intended to send a signal to the process for debugging purposes. In normal use, I expect a system administrator would do what I did when I wanted to enable W^X:

1. Log violations, but do not actually enforce W^X. This gives a system administrator a chance to assess how many programs may work differently if W^X enforcement is enabled.
2. Optionally, use signals to debug where the actual violations are in the code so they can either change the code or understand the implications of denying the mapping.
3. Enable enforcement (with or without logging, according to their preference), which would simply deny such a mapping (without signaling the process).

As described above, I don't propose to always send a SIGTRAP to the user process. But, if I've misunderstood your proposal, please feel free to correct me.

With my proposal, eventually you should get the same behavior, after the logging part is implemented, in the more generic context.

For now, I suggest to only add an optional ability to send SIGTRAP on wxorx violations. Next step would be add more generic optional logging for SIGTRAPs sending _places_ generated by syscall actions (i.e. capsicum + wxorx violations). It should get the knobs to configure same set of kernel actions (SIGTRAP yes/no, log yes/no) as in your patch.

[kib]

Probably my proposal would be clearer if formulated more operational than functional:

• either in trapsignal() add special handling for SIGTRAP with known si_codes (TRAP_CAP/WXORX): pass or ignore, log or stay silent based on the set of options
• or add some syscalltrapped() wrapper around trapsignal() which do the some checks, and optionally calls trapsignal/log. This seems to be nicer because trapsignal() does not need to grow specific knowledge, and because ksi can be filled specifically in syscalltrapped() instead of the same code spread over the kernel.

This way it is more generic at least, and more useful, and is easier to extend for more similar situations by avoiding copy/pasting).