Fix Coverty Scan CID 1498397 - array overrun in kern_sysctl.c

[se]

Summary

CID 1498397 reports that an out-of-bounds access happens in case of an OID that has more than 24 elements.
Such OIDs do not exist, but could be created (e.g. by a malicious driver trying to exploit this bug by creating a deep sysctl sub-tree).
Since the fetched OID name fits into an array of length CTL_MAXNAME, there is no reason to allocate more than that number of elements for name2.
In that case, name1 will be large enough to hold the two initial elements and the elements from name2.

Test Plan

Apply patch and verify that sysctl -a works with sysctl trees of the supported maximum depth.

[cem]

sys/kern/kern_sysctl.c

2875–2877

This would never emit an l2 bigger than CTL_MAXNAME, I think (even before this change).

[se]

sys/kern/kern_sysctl.c

2875–2877

Well, I do think it would!

If you got a 24 element OID that contains another level (since you could construct a sysctl tree of any depth), then the next element will have 25 elements.

The value of l2 passed in is 26*4 (CTL_MAXNAME+2 ints), which matches the length of the name2 array.
But if there really was a 25 deep element, then the returned actual length would be 100 (25*4), l2 would become 25 in line 2885.

Does sysctl_find_oid() return an error for l2 > 24?

That would actually protect the memcpy() in line 2904.
But I do not see that it limits the depth, and there is no other validation of the depth ...

But independently of this detail (whether sysctl_find_oid() returns an error for too long OIDs), the suggested patch will make kernel_sysctl() in line 2876 fail for OIDs with more than 24 elements and will protect the rest of the code. This would also guarantee that the memcpy() does not write ou-of-bounds, since the offset of 2 elements is compensated by a 2 element larger dest array compared to the size of the source array.

I do not see any reason to keep the +2 elements in name2. The extra elements will never contain a value in a well-formed sysctl tree (with depth <= CTL_MAXNAME). And limiting the length will make the code clearer and more robust, IMHO.

[cem]

Nevermind

sys/kern/kern_sysctl.c

2875–2877

We simply don't have sysctl trees deeper than 24 elements.

[se]

sys/kern/kern_sysctl.c

2875–2877

Yes, that is correct, we do not have sysctl trees deeper than 24 elements.

But how about my malicious driver module provides as a package that contains a sysctl tree deeper than 24 elements?

It is trivial to construct such a tree of any depth, since each level just refers to its parent level - there is no mechanism that limits the depth.

I do not expect such a kernel module to exist, but if you insist on none being possible then I'll create one ;-)

And, BTW: While I do not know why anybody might change the value of CTL_NAMEMAX, I might build a system with that value reduced to, e.g., 4.

With the patch suggested in this review, the behavior of this function would still be correct in such a case (error return instead of buffer overflow).