Skip to content

Commit

Permalink
sysctl: Harden sysctl_handle_string() against unterminated string
Browse files Browse the repository at this point in the history
In case a variable string which is not null-terminated is passed in,
strlen() may report a length exceeding the max length, hence it is
possible to leak a portion of kernel memory to the userland.

Harden that by using strnlen() to limit the length to the max length.
While here, refactor the code a little to improve readability.

Note that, when calculating the out length, the null terminator '\0' of
the string is taken into account if available. This is not really
necessary but userland applications may have already relied on this
behavior.

Reviewed by:	avg, kib, olce
Fixes:		210176a sysctl(9): add CTLFLAG_NEEDGIANT flag
MFC after:	4 days
Differential Revision:	https://reviews.freebsd.org/D48881
  • Loading branch information
gmshake committed Feb 9, 2025
1 parent 7d4c0fa commit 1951235
Showing 1 changed file with 20 additions and 22 deletions.
42 changes: 20 additions & 22 deletions sys/kern/kern_sysctl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1885,8 +1885,7 @@ int
sysctl_handle_string(SYSCTL_HANDLER_ARGS)
{
char *tmparg;
size_t outlen;
int error = 0, ro_string = 0;
int error = 0;

/*
* If the sysctl isn't writable and isn't a preallocated tunable that
Expand All @@ -1898,33 +1897,32 @@ sysctl_handle_string(SYSCTL_HANDLER_ARGS)
*/
if ((oidp->oid_kind & (CTLFLAG_WR | CTLFLAG_TUN)) == 0 ||
arg2 == 0 || kdb_active) {
arg2 = strlen((char *)arg1) + 1;
ro_string = 1;
}
size_t outlen;

if (req->oldptr != NULL) {
if (ro_string) {
tmparg = arg1;
outlen = strlen(tmparg) + 1;
} else {
if (arg2 == 0)
outlen = arg2 = strlen(arg1) + 1;
else
outlen = strnlen(arg1, arg2 - 1) + 1;

tmparg = req->oldptr != NULL ? arg1 : NULL;
error = SYSCTL_OUT(req, tmparg, outlen);
} else {
size_t outlen;

if (req->oldptr != NULL) {
tmparg = malloc(arg2, M_SYSCTLTMP, M_WAITOK);
sx_slock(&sysctlstringlock);
memcpy(tmparg, arg1, arg2);
sx_sunlock(&sysctlstringlock);
outlen = strlen(tmparg) + 1;
}

error = SYSCTL_OUT(req, tmparg, outlen);

if (!ro_string)
free(tmparg, M_SYSCTLTMP);
} else {
if (!ro_string)
outlen = strnlen(tmparg, arg2 - 1) + 1;
} else {
tmparg = NULL;
sx_slock(&sysctlstringlock);
outlen = strlen((char *)arg1) + 1;
if (!ro_string)
outlen = strnlen(arg1, arg2 - 1) + 1;
sx_sunlock(&sysctlstringlock);
error = SYSCTL_OUT(req, NULL, outlen);
}
error = SYSCTL_OUT(req, tmparg, outlen);
free(tmparg, M_SYSCTLTMP);
}
if (error || !req->newptr)
return (error);
Expand Down

0 comments on commit 1951235

Please sign in to comment.