Bug 206648 - Fix double strlen in ktrstruct
Summary: Fix double strlen in ktrstruct
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Mateusz Guzik
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-26 17:29 UTC by CTurt
Modified: 2016-01-31 05:51 UTC (History)
4 users (show)

See Also:
mjg: mfc-stable10-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-26 17:29:06 UTC
The `ktrstruct` function from `sys/kern/kern_ktrace.c` was calling `strlen` on `name` twice, which could have lead to a race attack (contents of `name` changed between these two calls). I've verified that this wasn't previously optimised into a single call to `strlen` in a compiled kernel.

There is no way to exploit this bug since it is only used by macros which pass strings with static contents:

#define ktrsockaddr(s) \
 ktrstruct("sockaddr", (s), ((struct sockaddr *)(s))->sa_len)
#define ktrstat(s) \
 ktrstruct("stat", (s), sizeof(struct stat))

However, this situation is fragile, and should be patched to prevent the possibility of being exploited in the future if ever `ktrstruct` is passed a non-static name. At the very least, this patch will also be a very minor optimisation.

Current:

void
ktrstruct(name, data, datalen)
	const char *name;
	void *data;
	size_t datalen;
{
	struct ktr_request *req;
	char *buf = NULL;
	size_t buflen;

	if (!data)
		datalen = 0;
	buflen = strlen(name) + 1 + datalen;
	buf = malloc(buflen, M_KTRACE, M_WAITOK);
	strcpy(buf, name);
	bcopy(data, buf + strlen(name) + 1, datalen);
	if ((req = ktr_getrequest(KTR_STRUCT)) == NULL) {
		free(buf, M_KTRACE);
		return;
	}
	req->ktr_buffer = buf;
	req->ktr_header.ktr_len = buflen;
	ktr_submitrequest(curthread, req);
}

Patched:

void
ktrstruct(name, data, datalen)
	const char *name;
	void *data;
	size_t datalen;
{
	struct ktr_request *req;
	char *buf = NULL;
	size_t namelen;
	size_t buflen;

	if (!data)
		datalen = 0;

	namelen = strlen(name);

	buflen = namelen + 1 + datalen;
	buf = malloc(buflen, M_KTRACE, M_WAITOK);
	strcpy(buf, name);
	bcopy(data, buf + namelen + 1, datalen);
	if ((req = ktr_getrequest(KTR_STRUCT)) == NULL) {
		free(buf, M_KTRACE);
		return;
	}
	req->ktr_buffer = buf;
	req->ktr_header.ktr_len = buflen;
	ktr_submitrequest(curthread, req);
}
Comment 1 CTurt 2016-01-26 17:32:15 UTC
Here's the commit which introduced the bug:

http://lists.freebsd.org/pipermail/svn-src-head/2010-July/018600.html
Comment 2 Mateusz Guzik freebsd_committer 2016-01-26 17:45:35 UTC
The code is fine correctness-wise, although indeed could be improved by getting rid of double strlen, I'll maybe commit that later.

If you make changes you would like to submit, please make actual patches (diff -u, svn diff, git diff, whatever).

Now to the point:
buflen = strlen(name) + 1 datalen;                   
buf = malloc(buflen, M_KTRACE, M_WAITOK);
strcpy(buf, name);
bcopy(data, buf + strlen(name) + 1, datalen);

The claim is that calling strlen twice would be problematic if the string could change.

First of all, if that was really the case, we would run into trouble with strcpy. Further, if it could change, maybe it also could be freed?

Clearly, the kernel code needs to ensure the stability of the string. If it is assumed the string is not going to change, the code is perfectly fine, although somewhat inefficient.

If the string is in kernel memory and can change, appropriate locks need to be held across the call. If it is in user memory, it has to be brought in with things like copyin().

Regardless, I see no bug nor an insecure practice here. If one was to not trust anything, it would be impossible to implement stuff.

That said thanks for bringing this up, that strlen would really use getting rid of, but there is nothing to fix correctness-wise or even in the spirit of defensive programming. One could consider adding an assertion the buffer belongs to the kernel, but then why would this function be special.
Comment 3 Mateusz Guzik freebsd_committer 2016-01-26 18:10:38 UTC
Maybe it should be noted that even with all callers behaving as they should, there indeed could be a problem here. If there was a bug elsewhere in the kernel allowing someone to modify the passed string they could indeed try to trick the kernel into overflowing the buffer by moving the null terminator before strcpy is called.

However, I consider trying to fight these kind of problems in this way to be a non-starter.

That said, the code is somewhat weaker than it could be, but changing this place while there are zilions other places with similar kind of issues is not the way to go. Same thing applies to kernels from other projects.
Comment 4 commit-hook freebsd_committer 2016-01-27 19:55:48 UTC
A commit references this bug:

Author: mjg
Date: Wed Jan 27 19:55:03 UTC 2016
New revision: 294934
URL: https://svnweb.freebsd.org/changeset/base/294934

Log:
  ktrace: tidy up ktrstruct

  - minor style fixes
  - avoid doing strlen twice [1]

  PR:		206648
  Submitted by:	C Turt <ecturt gmail.com> (original version) [1]

Changes:
  head/sys/kern/kern_ktrace.c
Comment 5 Kubilay Kocak freebsd_committer 2016-01-31 05:47:44 UTC
Re-open for pending (?) MFC request.

Set to + when committed, or - if denied, with comment
Comment 6 Mateusz Guzik freebsd_committer 2016-01-31 05:51:36 UTC
The change is cosmetic and not worth merging. Should there be more changes in the code, this can be merged no problems to reduce the diff.