Details

Reviewers

kib
kaktus

Summary

arg1 may point to a variable string which is not null-terminated. When micro-optimizing this string as a readonly one, strlen() may report a length exceeding the max length, hence it is possible to leak a portion of kernel memory to the userland.

Fixes: 210176ad76ee sysctl(9): add CTLFLAG_NEEDGIANT flag
MFC after: 1 week

Test Plan

Test with a hand crafted kernel module.

Makefile,

PACKAGE=examples
SRCS	= sysctl.c
KMOD	= sysctl

.include <bsd.kmod.mk>

sysctl.c

#include <sys/types.h>
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/module.h>
#include <sys/sysctl.h>
#include <sys/kernel.h>

char buf1[9] = {'d', 'a', 't', 'a', 'l', 'e', 'a', 'k', '\0'};

char leak_pre[8] = "preleak";
char buf2[8] = {'d', 'a', 't', 'a', 'd', 'a', 't', 'a'};
char leak_pos[8] = "posleak";


SYSCTL_STRING(_sysctl, OID_AUTO, leak1, CTLFLAG_RD, buf1, (sizeof buf1) / 2,
    "Read-only string buf 1");

SYSCTL_STRING(_sysctl, OID_AUTO, leak2, CTLFLAG_RD, buf2, sizeof buf2,
    "Read-only string buf 2");


static int
module_load(module_t mod, int cmd, void *arg)
{
	int error;

	error = 0;
	switch (cmd) {
	case MOD_LOAD:
		break;
	case MOD_UNLOAD:
		break;
	default:
		error = EOPNOTSUPP;
		break;
	}
	return (error);
}

static moduledata_t mod_data = {
	"sysctl",
	module_load,
	0
};

DECLARE_MODULE(sysctl, mod_data, SI_SUB_EXEC, SI_ORDER_ANY);

Build and load kernel module sysctl.ko.

Prior to the fix,

# sysctl -n sysctl.leak1
dataleak
# sysctl -n sysctl.leak2
datadataposleak

After the fix,

# sysctl -n sysctl.leak1
data
# sysctl -n sysctl.leak2
datadata

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

zlei created this revision.Jan 18 2025, 3:15 PM

Herald added subscribers: olce, imp. · View Herald TranscriptJan 18 2025, 3:15 PM

zlei requested review of this revision.Jan 18 2025, 3:15 PM

@markj
You may be interested with this. Spotted this while hacking 284073 , in case the buf overflows ...

I think this change is right, but it took me a while to understand it: the point is to harden sysctl_handle_string() against unterminated strings, so I'd suggest explaining a bit further in the commit log message.

sys/kern/kern_sysctl.c
1918	These lines should be wrapped.

kib added inline comments.Jan 19 2025, 3:38 AM

sys/kern/kern_sysctl.c
1918	In fact, nested assignment inside the ?: operator whines for the logic to be split. And since the same expression is repeated below, it might be reasonable to spent an effort to add a helper function there.

Simple if else block is much easier to read.

@kib Does this look good to you ?

kib added inline comments.Tue, Feb 4, 10:11 PM

sys/kern/kern_sysctl.c
1917	So why arg2 - 1? IMO this might get the last byte of non-nul terminated string get lost.
1928	Casts are not needed.

zlei marked 3 inline comments as done.Wed, Feb 5, 2:58 AM

zlei added inline comments.

sys/kern/kern_sysctl.c
1917	So why arg2 - 1? IMO this might get the last byte of non-nul terminated string get lost. I copy-pasted the code prior to 210176ad76ee. Think of char buff[10] = {...}; sysctl_handle_string(..., &buff, sizeof(buff), ...); If buff[9] != '\0', then (outlen = strnlen(buff, 10 - 1) + 1) == 10, the last byte will be copied out.
1917	Before I looked into the implementation of `strnlen(const char *s, size_t maxlen)`, I worried ( was misled ) that `strnlen()` may read (OOB) a range [0, maxlen] of the string, but I was wrong. Actually `strnlen()` reads only a range [0, maxlen - 1].

zlei marked an inline comment as done.Wed, Feb 5, 5:13 AM

zlei added inline comments.

sys/kern/kern_sysctl.c
1917	Before I looked into the implementation of `strnlen(const char *s, size_t maxlen)`, I worried ( was misled ) that `strnlen()` may read (OOB) a range [0, maxlen] of the string, but I was wrong. Actually `strnlen()` reads only a range [0, maxlen - 1]. Emm, please ignore the change, --- outlen = strnlen(tmparg, arg2 - 1) + 1; +++ outlen = strnlen(tmparg, arg2); That is absolutely wrong. In case `arg2 == 10`, `arg1` say "foo" has length == 3, that will end up missing the null character copied out.

zlei mentioned this in D23378: sysctl(9): add CTLFLAG_NEEDGIANT flag.Wed, Feb 5, 9:34 AM

The change looks good to me.
Thank you.

Remove unneeded casts to char * .

zlei edited the summary of this revision. (Show Details)Wed, Feb 5, 10:03 AM

The second case

out-of-bounds write to kernel memory.

is currently not possible, as sysctl_root() has already enforced the flag bit CTLFLAG_WR.

A simple call graph,

sysctl_root()
    kernel_sysctl()
    userland_sysctl()
    db_sysctl()

I think this version increases slightly the already too big amount of spaghetti code in this function.

Could you please consider keeping the modifications to a minimum and decreasing slightly the existing complexity by:

Keeping the setting of arg2 in the original if block, but guarded by an additional if (arg2 != 0).
Keeping the assignments of outlen where they were in the if (req->oldptr != NULL), just replacing outlen = strlen(tmparg) + 1 under the if (ro_string) with the optimized outlen = arg2.
Cutting some spaghetti in the else branch of the if (req->oldptr != NULL) by separating the ro_string case with the rest, as they in reality share nothing.
Limiting the scope of outlen.

Please see the proposed inline diffs to that effect. Thanks.

sys/kern/kern_sysctl.c
1902
1906–1908
1912–1914
1917	That is absolutely wrong. In case arg2 == 10, arg1 say "foo" has length == 3, that will end up missing the null character copied out. `SYSCTL_OUT()` has never cared about the string being null terminated, and doesn't need it. Callers (e.g.,`sysctl(8)`) are written to cope with this. Thus, `strnlen(tmparg, arg2)` is correct here. I certainly don't oppose changing the contract (boy, I'd be glad if someone not me does it), but then please do that as a separate change, not here.
1924–1933

Whoops... The first inline comment was wrong, I proposed another version.

sys/kern/kern_sysctl.c
1902	Replaces my previous inline comment.

olce added inline comments.Wed, Feb 5, 9:24 PM

sys/kern/kern_sysctl.c
1888	Reduce scope of `outlen`, see inline comments below.

Seems Phab did not save my inline comments which consisted of only line removals and no written text. Or I messed up something, but don't know how. The diffs should be complete now.

sys/kern/kern_sysctl.c
1913–1917	Becomes unnecessary with the other changes.

kib added inline comments.Thu, Feb 6, 12:53 AM

sys/kern/kern_sysctl.c
1917	I still do not understand this -1/+1. Just clip at exactly arg2 if the outlen > arg2.

avg added inline comments.Thu, Feb 6, 8:50 AM

sys/kern/kern_sysctl.c
1917	"-1/+1" is needed to ensure that terminating '\0', if it's found, is accounted in `outlen`. If it's not found then `outlen` would be equal to `arg`. The same could be achieved with an explicit condition, but "-1/+1" works too, although maybe not very obvious.

zlei added inline comments.Thu, Feb 6, 9:36 AM

sys/kern/kern_sysctl.c
1917	"-1/+1" is needed to ensure that terminating '\0', if it's found, is accounted in `outlen`. If it's not found then `outlen` would be equal to `arg`. The same could be achieved with an explicit condition, but "-1/+1" works too, although maybe not very obvious. Exactly ! Assume `len = strlen(tmparg)`, then `strnlen(tmparg, arg2)` is equivalent to `MIN(len, arg2)` , while the "-1/+1" is equivalent to `MIN(len+1, arg2-1+1)` .

@olce Thanks for you suggestion! I think this revision is much simpler.

zlei added inline comments.Thu, Feb 6, 9:49 AM

sys/kern/kern_sysctl.c
1956	It appears that reducing the scope of `tmparg` does not serve much value...

Restore scope of tmparg to reduce diff.

zlei added inline comments.Thu, Feb 6, 10:09 AM

sys/kern/kern_sysctl.c
1902	I think a comment /* Pre-calculate out length of a read-only string. */ can help reading the code.

zlei edited the test plan for this revision. (Show Details)Thu, Feb 6, 11:48 AM

Please see the new inline comments. Thanks!

(Sorry, but it seems it's not possible in Phabricator to delete older, now obsolete inline comments. If someone knows how to do it, please tell me.)

sys/kern/kern_sysctl.c
1890	Please rename this variable, as its current name is really misleading.
1902	I think a comment (...) can help reading the code. I concur!
1904	In fact, I prefer this version even to my initial suggestion of limiting the scope of `outlen`, and I had considered sending another comment to that effect this morning (but there are too many comments now, and I can't seem to find a way to delete old, now irrelevant ones). Indeed, it's the output length we want to compute, so the result should go into `outlen`. In addition to that, I don't think we should mess with `arg2`. This variable is used after the "output" code in order to limit what is read in. This function should be hardened against the case of a programming error where the caller of `sysctl_handle_string()` would not have thrown an error on a non-null 'req->newptr' (indicating the user wants to set the value) in the case of a constant (e.g., someone uses `SYSCTL_STRING()` on a "constant" but with `CTLFLAG_WR`). And, actually, it is already, if we leave `arg2` to 0 (see the later `if (req->newlen - req->newidx >= arg2 \|\| req->newlen - req->newidx < 0) {` test). Adding a comment to that effect would also be great.
1906	I'd add a comment about the reason for the -1/+1, something like "make sure that the terminating 0 is output, if there is any within `arg2` bytes". But, as I said in an earlier comment, this is not really a necessity, as callers already cope with that behavior, which I agree is in violation with what this function's herald comment states. If you really want to fix that (would be great!), I'd suggest: Doing this as a separate commit. Making sure a terminating 0 is output including in the case of `arg1` being of (string) size greater than `arg2`, which is not the case even with the -1/+1 trick.

I thought the complex logic of this string sysctl handler for a whole night. I can conclude that the root cause is the different requirement of req->oldfunc and req->newfunc . There're generally three cases,

Kernel boot time / module dynamically load time, set loader tunables via sysctl_load_tunable_by_oid_locked(). No locks or memory allocation / snapshoting required.
Other kernel consumers that want to manipulate sysctl knobs, sysctlstringlock may be acquired if not readonly, but no snapshoting required.
Userland sysctl syscall. sysctlstringlock may be acquired if not readonly, snapshoting required if not readonly.

I'd propose to refactor sysctl_handle_string throughly , but at this time I want to have the fix in to catch up 13.5-RELEASE. I'll update a different revision that omits the confusing ro_string.

In D48511#1114777, @zlei wrote:

...
I'll update a different revision that omits the confusing ro_string.

@olce Posted to D48881. Let's start over :)

Superseded by D48881 .

sysctl: Harden sysctl_handle_string() against unterminated strings
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 150524

sys/kern/kern_sysctl.c

sysctl: Harden sysctl_handle_string() against unterminated stringsAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 150524

sys/kern/kern_sysctl.c

sysctl: Harden sysctl_handle_string() against unterminated strings
AbandonedPublic
Actions

Revision Contents
Changeset List