Page MenuHomeFreeBSD
Differential D53805

ipf: Validate string offsets in ipnat structures
Needs ReviewPublic
Actions

Authored by markj on Tue, Nov 18, 4:16 PM.
Tags
None
Referenced Files
F136671144: D53805.id166680.diff
Tue, Nov 18, 8:18 PM
F136664888: D53805.id.diff
Tue, Nov 18, 7:36 PM
F136660886: D53805.diff
Tue, Nov 18, 7:11 PM
F136659815: D53805.diff
Tue, Nov 18, 7:04 PM
Subscribers
cy
glebius
imp
melifaro
vegeta_tuxpowered.net
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None

Diff Detail

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

Event Timeline

markj created this revision.Tue, Nov 18, 4:16 PM
Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro, imp. · View Herald TranscriptTue, Nov 18, 4:16 PM
markj requested review of this revision.Tue, Nov 18, 4:16 PM
Harbormaster completed remote builds in B68711: Diff 166680.Tue, Nov 18, 4:16 PM
markj mentioned this in D53752: ipfilter: Verify ipnat on entry into kernel.Tue, Nov 18, 4:24 PM
cy added a subscriber: cy.Tue, Nov 18, 6:41 PM

This would need to be duplicated to replace D53475.

I will abandon D53475 and D53752 to use this (with corresponding changes to fil.c) instead. Is that ok?

sys/netpfil/ipfilter/netinet/ip_nat.c
964

Let's rename this to check_names_string() and use it also in D53475.

972

This needs to be defined in interror.c.

976

This needs to be defined in interror.c.

982

This needs to be defined in interror.c.

cy added inline comments.Tue, Nov 18, 6:53 PM
sys/netpfil/ipfilter/netinet/ip_nat.c
964

Let's rename this to check_names_string() and use it also in D53475.

I think we should add check_names_string() in one commit. Then add two commits to check fr_names in fil.c (for ipfilter) and in ip_nat.c (for ipnat).

cy added a comment.Tue, Nov 18, 7:00 PM

Rather than set IPFERROR within the function we return a return code and let the caller set the IPFERROR. Do you mind if I take this and flesh it out a bit?

markj added a comment.Tue, Nov 18, 7:06 PM
In D53805#1228989, @cy wrote:

Rather than set IPFERROR within the function we return a return code and let the caller set the IPFERROR. Do you mind if I take this and flesh it out a bit?

Please go ahead!

cy added a comment.Tue, Nov 18, 7:29 PM
In D53805#1228994, @markj wrote:
In D53805#1228989, @cy wrote:

Rather than set IPFERROR within the function we return a return code and let the caller set the IPFERROR. Do you mind if I take this and flesh it out a bit?

Please go ahead!

Would you mind if the review will include the three commits (squashed) required to implement the ipfilter and ipnat verifications? Context matters.

markj added a comment.Tue, Nov 18, 7:34 PM
In D53805#1229199, @cy wrote:
In D53805#1228994, @markj wrote:
In D53805#1228989, @cy wrote:

Rather than set IPFERROR within the function we return a return code and let the caller set the IPFERROR. Do you mind if I take this and flesh it out a bit?

Please go ahead!

Would you mind if the review will include the three commits (squashed) required to implement the ipfilter and ipnat verifications? Context matters.

Yes, doing it all in one patch is ok with me.

cy added inline comments.Tue, Nov 18, 8:21 PM
sys/netpfil/ipfilter/netinet/ip_nat.c
1083

nat->in_ifnames[0] can be == nat->in_ifnames[1]. If it is we don't need to check it twice.

markj added inline comments.Tue, Nov 18, 8:23 PM
sys/netpfil/ipfilter/netinet/ip_nat.c
1083

Sure, but it's hardly an expensive check, so I'd rather keep things a bit simpler.

Revision Contents
Changeset List

PathSize
sys/
netpfil/
ipfilter/
netinet/
47 lines

Diff 166680

View Options

sys/netpfil/ipfilter/netinet/ip_nat.c

Loading...