ipfilter: Avoid OOB read when ingesting interface names in ip_nat

[cy]

Summary

Rogue callers (i.e. rogue jail) to ip_nat may input interface names
longer than the passed string length. Make sure the interface string
is not longer than specified.

This is not a problem when using the command line interface. Only when
calling ipfilter directly from user code that tries to emulate ipfilter
userland.

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

[emaste]

sys/netpfil/ipfilter/netinet/ip_nat.c

1546

Do we have some existing check that we are guaranteed not to walk off the end of a malicious in_names?

[cy]

I will review my use of strlen throughout. I'll get back to you tomorrow.

sys/netpfil/ipfilter/netinet/ip_nat.c

1546

I suppose I could use an arbitrary number. in_names are interface names.

[cy]

Use strnlen().

[cy]

Use LIFNAMSIZ.

[markj]

Looking at the code, I think this structure is copied in in ipf_nat_ioctl(). There, we copy in a struct ipnat without the variable-length interface name after it. Then, we check the in_size field of the copied-in structure (which presumably includes the length of the interface name) and copy the whole thing in.

So, I guess we should handle validation there, and we should check that the string is in the bounds defined by the in_size field instead.

[cy]

In D53278#1219408, @markj wrote:

Looking at the code, I think this structure is copied in in ipf_nat_ioctl(). There, we copy in a struct ipnat without the variable-length interface name after it. Then, we check the in_size field of the copied-in structure (which presumably includes the length of the interface name) and copy the whole thing in.

So, I guess we should handle validation there, and we should check that the string is in the bounds defined by the in_size field instead.

ip_nat doesn't use frrequest(). It's parsed from a different file. The ioctl handler passes ip_nat requests to ipf_nat*. Yeah I should do the same there. Would probably make more sense than playing whack-a-mole.

[cy]

Hmm. I may want to abandon the reviews and take a broader approach to input verification. Some input verification is already there but needs improvement.

Regarding reloading rules from ipfs, I'm not happy with ipfs. It should be exported as a set of text rules that can be parsed on re-input rather than data structures that can be abused. It should be removed as it has some panic issues and nobody uses it.

[cy]

This is based on my incorrect understanding that fr_names contains only interface names.