ipfilter: Verify frentry on entry into kernel

[cy]

Summary

The frentry struct is built by ipf(8), specifically ipf_y.y when parsing
the ipfilter configuration file (typically ipf.conf). frentry contains
a variable length string field at the end of the struct. This data field,
called fr_names, may contain various text strings such as NIC names,
destination list (dstlist) names, and filter rule comments. There is no
upper bound limit to the length of strings as long as the fr_namelen
length field specifies the length of fr_names within the frentry
structure and fr_size specifies the size of the frentry structure
itself.

Reported by: Ilja Van Sprundel <[e-mail]>
MFC after: 1 week

[cy]

I added jrm@ as a subscriber as Ed, Joseph and I had discussed this patch after the wireless meeting.

[markj]

sys/netpfil/ipfilter/netinet/fil.c

4541

You still need to check that _a->_b <= rem_namelen and that the string isn't longer than rem_namelen - _a->_b, no?

4552

Isn't fp->fr_namelen also user-controlled? From reading the code at the beginning of the function, it looks like fp->fr_size is the canonical object size since that's the allocation size.

Also, do we need to do this validation if makecopy == 0? In that case, the request is coming from the kernel, not userspace.

[cy]

Would it be better if we got together in a Teams call to discuss this?

sys/netpfil/ipfilter/netinet/fil.c

4541

_b is an offset into fr_names.

name is the pointer to the offset. It's in fact is (_a)->fr_names + (_a)->_b. To call it _a->_b is confusing.

rem_namelen starts out as fp->fr_namelen. We subtract the sizeof(the string) from rem_namelen. There is no need to compare _a->_b <= rem_namelen because we test for rem_namelen being negative. If rem_namelen is negative we've exceeded the end point of fr_names.

4552

Isn't fp->fr_namelen also user-controlled? From reading the code at the beginning of the function, it looks like fp->fr_size is the canonical object size since that's the allocation size.

fp->fr_namelen and fp->fr_size are both user controlled. This is why we start out with v_fr_size = sizeof(*fp)

...and for each iteration,

v_fr_size += element_size;
and if v_fr_size > fp->fr_size we've exceeded fr_entry_t + fr_names.

Similarly we start out with rem_namelen = fp->fr_namelen

... and for each iteration

rem_namelen -= element_size;
Test for rem_namelen < 0, suggesting that we've exceeded the end of fr_names.

element_size is the strlen of the string.

Or am I missing something? Reviewing my logic I don't think I am but maybe I have.

Also, do we need to do this validation if makecopy == 0? In that case, the request is coming from the kernel, not userspace.

Correct. I will look at moving the block of code to within the "makecopy" block.

[markj]

In D53475#1221471, @cy wrote:

Would it be better if we got together in a Teams call to discuss this?

I'm happy to have a call. Teams tends to have trouble recognizing my webcam and mic on FreeBSD, whereas other platforms do not, so if possible I'd rather use a different platform (meet.freebsd.org, zoom, google meet are all fine), but I can try.

sys/netpfil/ipfilter/netinet/fil.c

4541

_b is an offset into fr_names.

Right. Suppose _a->_b is 1000000. What happens? We will call strnlen() on a pointer that's completely outside of the object. We need to make sure that name points within the bounds of the object we copied in.

4552

The problem is that we're calling strnlen(name, rem_namelen) before we check that name and rem_namelen have sane values. Such a call can trigger a panic if name points outside the object or if name + rem_namelen points outside the object.

The fact that we blindly allocate fr->fr_size bytes without bounding it to some limit (1MB?) is itself a bug.

[cy]

I do have something ready to upload to this review. I want to exercise it a bit more. There will be another review for a similar patch for ipnat, also being tested here.

sys/netpfil/ipfilter/netinet/fil.c

4541

I was thinking about that on Sunday, That's been moved up before the KMALLOCS(). And made other checks including a max_namelen check.

4552

The problem is that we're calling strnlen(name, rem_namelen) before we check that name and rem_namelen have sane values. Such a call can trigger a panic if name points outside the object or if name + rem_namelen points outside the object.

Thinking about that on Sunday (while cleaning up the yard). That too has been addressed.

The fact that we blindly allocate fr->fr_size bytes without bounding it to some limit (1MB?) is itself a bug.

I've added a sysctl/kenv tunable/ipf tunable that clamps namelen to max_namelen which defaults to 128 bytes. Typically one would use much less but since there is a free form comment field (comment argument) it could grow larger, though I doubt anyone would use more than 80 bytes for comments alone. It can be increased/decreased using a kenv tunable, sysctl, and ipfilter tunable (ipf -T). This will also be used by the same code (implemented here) for ipnat.

[cy]

1. Make sure frentry_t passed from userland >= sizeof(frentry_t).

2. Moved fr_size == sizeof(frentry_t) + fr_namelen test is before KMALLOCS().

3. Limit fr_namelen received from userland is not greater than tunable ipf_max_namelen (defaults to 128).

4. When performing strnlen() limit search for '\0' to last byte of fr_names.