Page MenuHomeFreeBSD
Differential D53276

ipfilter: Ensure that interface names are not OOB
AbandonedPublic
Actions

Authored by cy on Wed, Oct 22, 11:30 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 18, 9:14 PM
Unknown Object (File)
Tue, Nov 18, 7:50 AM
Unknown Object (File)
Mon, Nov 17, 10:51 PM
Unknown Object (File)
Sat, Nov 15, 2:00 PM
Unknown Object (File)
Sat, Nov 15, 9:11 AM
Unknown Object (File)
Thu, Nov 13, 10:08 PM
Unknown Object (File)
Tue, Nov 11, 4:46 AM
Unknown Object (File)
Tue, Nov 4, 5:27 PM
View All 40 Files
Subscribers
glebius
imp
melifaro
vegeta_tuxpowered.net

Details

Reviewers
emaste
markj
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 <ivansprundel@ioactive.com>
MFC after: 1 week

Test Plan

Will test on my prod ipfilter system.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 68016
Build 64899: arc lint + arc unit

Event Timeline

cy created this revision.Wed, Oct 22, 11:30 PM
Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro, imp. · View Herald TranscriptWed, Oct 22, 11:30 PM
Harbormaster completed remote builds in B68016: Diff 164814.Wed, Oct 22, 11:30 PM
cy requested review of this revision.Wed, Oct 22, 11:30 PM
cy updated this revision to Diff 164848.Thu, Oct 23, 5:25 AM

Use strnlen() instead of strlen.

Harbormaster completed remote builds in B68034: Diff 164848.Thu, Oct 23, 5:25 AM
markj added inline comments.Fri, Oct 24, 2:56 PM
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 added inline comments.Fri, Oct 24, 10:51 PM
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 added inline comments.Sat, Oct 25, 1:35 PM
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 updated this revision to Diff 165178.Tue, Oct 28, 2:40 AM

Use new ipf_frname_vfy() function.

Harbormaster completed remote builds in B68154: Diff 165178.Tue, Oct 28, 2:40 AM
markj added a comment.Tue, Oct 28, 2:00 PM

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

cy added a comment.Tue, Oct 28, 4:34 PM
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 retitled this revision from ipfilter: Avoid out of bounds read in ip_nat to ipfilter: Ensure that interface names are not OOB.Tue, Oct 28, 5:53 PM
cy edited the summary of this revision. (Show Details)
cy edited the test plan for this revision. (Show Details)
cy updated this revision to Diff 165230.Tue, Oct 28, 5:55 PM
cy edited the summary of this revision. (Show Details)

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.

Harbormaster completed remote builds in B68181: Diff 165230.Tue, Oct 28, 5:55 PM
cy updated this revision to Diff 165231.Tue, Oct 28, 6:05 PM

Add missing ipferror.c updates.

Harbormaster completed remote builds in B68182: Diff 165231.Tue, Oct 28, 6:05 PM
cy mentioned this in D53277: ipfilter: Make sure fr_sifpidx does not point outside of fr_names.Tue, Oct 28, 7:11 PM
cy updated this revision to Diff 165237.Tue, Oct 28, 7:17 PM

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

Harbormaster completed remote builds in B68185: Diff 165237.Tue, Oct 28, 7:17 PM
cy updated this revision to Diff 165238.Tue, Oct 28, 7:18 PM

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

Harbormaster completed remote builds in B68186: Diff 165238.Tue, Oct 28, 7:19 PM
cy updated this revision to Diff 165248.Tue, Oct 28, 7:32 PM

Fix a typo.

Harbormaster completed remote builds in B68187: Diff 165248.Tue, Oct 28, 7:32 PM
markj added inline comments.Tue, Oct 28, 9:57 PM
sbin/ipf/libipf/interror.c
183 ↗(On Diff #165248)

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
4808

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

4834

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

4838

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 abandoned this revision.Tue, Oct 28, 11:13 PM

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.

Revision Contents
Changeset List

PathSize
sys/
netpfil/
ipfilter/
netinet/
4 lines

Diff 164814

View Options

sys/netpfil/ipfilter/netinet/fil.c

Loading...