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); }
Here's the commit which introduced the bug: http://lists.freebsd.org/pipermail/svn-src-head/2010-July/018600.html
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.
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.
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
Re-open for pending (?) MFC request. Set to + when committed, or - if denied, with comment
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.