Bug 204097 - witness_initialize() does not perform bound checking of witness_count
Summary: witness_initialize() does not perform bound checking of witness_count
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
Keywords: needs-patch, needs-qa, patch
Depends on:
Reported: 2015-10-28 18:15 UTC by CTurt
Modified: 2016-10-07 15:02 UTC (History)
4 users (show)

See Also:
koobs: mfc-stable10?
koobs: mfc-stable9?


Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2015-10-28 18:15:04 UTC
The witness_count sysctl node is of type CTLFLAG_RDTUN, which means it's a read-only variable, but can be set during boot by creating a "debug.witness.count" entry in /boot/loader.conf.

The witness_initialize() function of sys/kern/subr_witness.c does not perform bound checks on witness_count which could lead to integer overflows, and memory corruption.

The following line from witness_initialize() can cause an overflow, if witness_count is 2147483647 for example, since a signed comparison is used:

	for (i = 0; i < witness_count + 1; i++) {

This means that the w_rmatrix[i] buffers are never allocated, which would lead to kernel reads and writes from an uninitialized pointer.

A potential fix would be to add the following bound check at the beginning of the function:

	if (witness_count < 0 || witness_count >= 2147483647) {
		printf("Invalid witness_count value of %d, setting to 2147483646\n", witness_count);
		witness_count = 2147483646;
Comment 1 Benjamin Kaduk freebsd_committer 2015-11-10 21:29:49 UTC
Changing 'i' to be of type size_t would be a more architecturally pleasing change, if a change is needed here.

I see some argument for anti-foot-shooting here, but it is not a particularly strong one -- such a large value of witness_count seems unlikely to lead to a usable system anyway, so it is not reasonable for an administrator to want to set such a value.  There are many, many ways in which tunables can be (ab)used to create a broken system and panics, and it's not reasonable to put in anti-foot-shooting measures for all of them, particularly the ones where there is not a reasonable reason to want to set such values in the first place.
Comment 2 Shawn Webb 2016-03-01 20:56:16 UTC
Any movement on this?
Comment 3 Benjamin Kaduk freebsd_committer 2016-03-03 05:52:42 UTC
> Any movement on this?

Perhaps you should say what movement you are expecting?  My reading is that someone needs to supply a justification for why it is necessary to prevent root from crashing the machine in this case, as opposed to all the other ways in which root can crash the machine.
Comment 4 Shawn Webb 2016-03-03 14:22:03 UTC
(In reply to Benjamin Kaduk from comment #3)

Code correctness would be an excellent reason.
Comment 5 Ed Maste freebsd_committer 2016-03-31 18:52:31 UTC
i can't trivially be made a size_t as later on there's a
for (i = witness_count - 1; i >= 0; i--)
Comment 6 Gleb Smirnoff freebsd_committer 2016-03-31 18:57:33 UTC
Since WITNESS isn't a production feature, is not enabled in GENERIC kernels, and no sane person would put that into production, this problem can't be qualified as a security problem.

So, please feel free to commit the fix without any coordination with secteam. And I'm unsubscribing the secteam from the bug.
Comment 7 Ed Maste freebsd_committer 2016-10-07 15:02:05 UTC
Removing security keyword per glebius' comment