ipfilter: Ensure that interface names are not OOB

[cy]

Summary

The indicies received from userland are userland controlled structures
that are used to address variable length fields within the fr_names
buffer. This is not a problem when ipf(8) creates passes these fields
through to the kernel. However, any jailed root user may be able to
with an application that calls the ioctl itself cause an OOB read.
We mitigate this by validating the indicies received from userland.

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

Test Plan

Will test on my prod ipfilter system.

[cy]

Use strnlen() instead of strlen.

[markj]

sys/netpfil/ipfilter/netinet/fil.c

3927

This doesn't make sense, fr->fr_ifnames[i] is an integer, not a string.

It looks like fr->fr_names is a buffer containing multiple strings, each one should be nul-terminated. Each fr_ifnames[i] needs to be validated.

[cy]

sys/netpfil/ipfilter/netinet/fil.c

3927

<sigh> I noticed that last night and started reworking this and other patches that surround FR_NAME. Not only is this wrong but I need to rethink the entire approach.

I will submit preparatory patches first and reworking these specific patches with the prerequisite patches in mind. I'm talking about two different things here, yes I agree this is wrong and secondly rework this and the others. There are a number of changes to these patches I am applying so there will be a period of inactivity followed by new patches. With this in mind should I put these patches on hold and rework them after the preparatory patches or would it be less confusing for people to abandon these and start afresh? It doesn't matter to me. I want to make this as simple for everyone to review as possible.

Working at this an hour or two in the evenings will mean there will be some wait time. What I'm saying is I submitted these patches for review before reviewing them myself and before testing them. Too hasty to fix possible CVEs. I'm good either way. Whatever works for you and Ed.

[markj]

sys/netpfil/ipfilter/netinet/fil.c

3927

Whatever works better for you is fine. I'm fine with abandoning this patch if that's easier. As long as each patch is relatively small and self-contained, it'll be straightforward to review.

[cy]

Use new ipf_frname_vfy() function.

[markj]

Shouldn't the validation be done at the ioctl layer, i.e., in frrequest() when we're copying in the frentry object?

[cy]

In D53276#1219432, @markj wrote:

Shouldn't the validation be done at the ioctl layer, i.e., in frrequest() when we're copying in the frentry object?

That would seem logical. A cursory look tells me it should be some place around were interface names are checked and before they're looked up. Could probably do the same for ip_nat too.

[cy]

I've taken Mark's suggestion to move the validation code from
ipf_synclist() to frrequest().

This also negates the need for ipf_frname_vfy() in another review
which will now be closed.

[cy]

Add missing ipferror.c updates.

[cy]

Validating fr_sifpidx in a separate commit is cumbersome. Both depend
on fr_namelen not to exceed the fr_names boundary.

[cy]

Again, update interror.c. Need to save it too before updating the commit.

[cy]

Fix a typo.

[markj]

sbin/ipf/libipf/interror.c

183

The alignment here looks inconsistent.

sys/netpfil/ipfilter/netinet/fil.c

4725

What about fp->fr_namelen < 0? Why do you check fr_namelen only here, and not for the other rule types?

4730

4819

How do we know that fd->fr_namelen is valid here?

4845

Are you sure this case is an error? Other code handles -1 as meaning "ignore this index".

4849

What happens if fp->fr_ifnames[i] is a large, positive integer? We will read past the end of the object. Calling strnlen() is not sufficient.

[cy]

The assumption that fr_names only contains interface names is incorrect and elements are not limited to LIFNAMSIZ. I need to step back to try an understand this.

Where len below is LIFNAMSIZ.

fr_namelen is 19, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is ue0, strlen of fr_name is 3
fr_namelen is 19, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is em0, strlen of fr_name is 3
fr_namelen is 21, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is , strlen of fr_name is 0
fr_namelen is 21, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is , strlen of fr_name is 0
fr_namelen is 21, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is , strlen of fr_name is 0
fr_namelen is 19, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is ue0, strlen of fr_name is 3
fr_namelen is 19, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is em0, strlen of fr_name is 3
fr_namelen is 21, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is , strlen of fr_name is 0
fr_namelen is 21, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is , strlen of fr_name is 0
fr_namelen is 21, len is 16
i is 0, fr_name is , strlen of fr_name is 0
i is 1, fr_name is , strlen of fr_name is 0
i is 2, fr_name is , strlen of fr_name is 0
i is 3, fr_name is , strlen of fr_name is 0

I only see this with rules like this:

pass in quick on ue0 from pool/trusted to pool/slippy

I cannot base my patches on what I think I might know.

sys/netpfil/ipfilter/netinet/fil.c

4725

This test is incorrect fr_name appears to be used for pool names as well.