Page MenuHomeFreeBSD

pf: Fix parsing of long table names
ClosedPublic

Authored by kp on Apr 24 2021, 2:00 PM.

Details

Summary

When parsing the nvlist for a struct pf_addr_wrap we unconditionally
tried to parse "ifname". This broke for PF_ADDR_TABLE when the table
name was longer than IFNAMSIZ. PF_TABLE_NAME_SIZE is longer than
IFNAMSIZ, so this is a valid configuration.

Only parse (or return) ifname or tblname for the corresponding
pf_addr_wrap type.

This manifested as a failure to set rules such as these, where the pfctl
optimiser generated an automatic table:

pass in proto tcp to 192.168.0.1 port ssh
pass in proto tcp to 192.168.0.2 port ssh
pass in proto tcp to 192.168.0.3 port ssh
pass in proto tcp to 192.168.0.4 port ssh
pass in proto tcp to 192.168.0.5 port ssh
pass in proto tcp to 192.168.0.6 port ssh
pass in proto tcp to 192.168.0.7 port ssh

Reported by: Florian Smeets
X-MFC-With: 5c11c5a3655842a176124ef2334fcdf830422c8a
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kp requested review of this revision.Apr 24 2021, 2:00 PM

The patch works for me. Thanks.

donner added inline comments.
lib/libpfctl/libpfctl.c
166–172

Could we have a "switch" here to catch the other (missing) cases?

So those addr->ifname might be unset in contrast to the previous version. Can this cause any harm? What's the default?

lib/libpfctl/libpfctl.c
166–172

I think I'd prefer to leave it as is. The fields under addr->v are part of a union, so they used to just always be copied in. We now do a bit more validation, which is what caused the bug this fixes: we checked the length of the ifname string, but that overlapped (because union) with the tblname string, which can be longer.

In fact, it turns out that for type == PF_ADDR_DYNIFTL we still use the mask value, so the resulting conditional would end up being complex (and thus error-prone).

This revision is now accepted and ready to land.Apr 26 2021, 1:13 PM
This revision was automatically updated to reflect the committed changes.