Page MenuHomeFreeBSD

Add a simple port filter to SIFTR
ClosedPublic

Authored by rscheff on Jan 18 2019, 7:32 PM.

Details

Summary

SIFTR does not allow any kind of filtering, but captures every packet processed by the TCP stack.
Often, only a specific session or service is of interest, and doing the filtering in post-processing of the log adds to the overhead of SIFTR.

This adds a new sysctl net.inet.siftr.port_filter. When set to zero, all packets get captured as previously. If set to any other value, only packets where either the source or the destination ports match, are captured in the log file.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22042
Build 21270: arc lint + arc unit

Event Timeline

sys/netinet/siftr.c
322

Does it make sense to use SYSCTL_U16 and a uint16_t here to match the port number?

919

I'd tend to use an explicit != 0 since this isn't a bool.

  • let us not forget on IPv6
  • have sysctl check the port number range directly
rscheff marked 2 inline comments as done.
  • line wrap
sys/netinet/siftr.c
322

I assume that also implies that range checking of the user provided value is then done by the sysctl framework? Absolutely, I have to admit, I didn't check all the various types available when fetching the patch.

sys/netinet/siftr.c
322

With SYSCTL_U16, this will wraparound and cause unexpected storage. Prefer the SYSCTL_UINT.

ccui@FBSD11:~/siftr % sudo sysctl net.inet.siftr.port_filter=22
net.inet.siftr.port_filter: 0 -> 22
ccui@FBSD11:~/siftr % sudo sysctl net.inet.siftr.port_filter=65535
net.inet.siftr.port_filter: 22 -> 65535
ccui@FBSD11:~/siftr % sudo sysctl net.inet.siftr.port_filter=65536 << using SYSCTL_U16
net.inet.siftr.port_filter: 65535 -> 0
ccui@FBSD11:~/siftr % sudo sysctl net.inet.siftr.port_filter=65537
net.inet.siftr.port_filter: 0 -> 1
ccui@FBSD11:~/siftr % sudo sysctl net.inet.siftr.port_filter
net.inet.siftr.port_filter: 1

sys/netinet/siftr.c
322

Crappy kernel API design... The handler should reject over-long inputs, but it's probably unfixable at this date. I'm not sure I care about this case of user error...

sys/netinet/siftr.c
322

Suggestion:

  • The uint16_t would probably require 4 bytes due to alignment anyway; perhaps we use SYSCTL_INT, and log an error when set >65535 into the siftr log?
  • Alternatively keep it as a uint16_t for siftr, but have our own SYSCTL_PROC handler with CTLINT when changing the value...
  • Slightly preferred: keep it as SYSCTL_U16 and be done with it...

Whichever way, I believe having a light-weight port filtering available at the siftr level reduces the clutter in the log files enough to be worthwhile.

(Also, sysctl returns the actually set value already; if misconfigurations happen frequently, a check should be scripted when setting the filter value, that the set value is the expected value...)

This revision is now accepted and ready to land.Jan 29 2019, 9:11 PM
This revision was automatically updated to reflect the committed changes.