pf.conf - max state logging weirdness

[AdSchellevis]

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

• I have read the contributing guide lines at https://github.com/opnsense/src/blob/master/CONTRIBUTING.md
• I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/src/issues?q=is%3Aissue

Describe the bug

Not sure if it's a bug or a feature, but the upstream documentation doesn't seem to be very helpful on this subject.
When the maximum state limit is reached, packets are being logged as "pass", but in reality they are dropped.

To Reproduce

Consider a random (tcp) rule with a max state limit set to a destination address, for example:

pass in log quick inet from {any} to {y.y.y.y} keep state ( max 1  ) 

When the first client opens a session, it passes and logs a rule like below:

97,,,53009487a9b33e07a1d1b4738b5e5bb0,igc0,match,pass,in,4,0x10,,64,0,0,none,6,tcp,64,x.x.x.x,y.y.y.y,56475,80,0,S,280130658,,65535,,mss;nop;wscale;nop;nop;TS;sackOK;eol

The next client then tries the same, which will timeout, but logs the same in our pf log.

Expected behavior

I'm not sure, but I would expect that either the action or reason would be different and help to distinguish between successful and unsuccessful connection attempts.

Describe alternatives you considered

none

Environment

OPNsense 24.7.x (amd64)

[fichtner]

This reminds me of opnsense/core#6800 where pflog just logs the wrong thing due to an implementation issue. We have an OpenBSD patch for the situation, but FreeBSD decided to do this differently, but never merged it to stable/14. So I don't know if this is another case that needs patching or if this works on FreeBSD main already. 🤷‍♂️

OpenBSD/OPNsense: 385d8a743
FreeBSD: freebsd/freebsd-src@948e8413ab

[AdSchellevis]

does the upstream patch fit on our end? The openbsd patch looks nice, but as the reason for a state deny doesn't change, it will mark as pass where in reality it was dropped. upstream version does seem to pass the proper reason in the logging.

EDIT: what is strange by the way is that in openbsd the proper action seems to be in rm->action, where the upstream commit tries to pass more parameters.... it feels like some "action" update is missing somewhere (assuming openbsd is doing the right thing)

[fichtner]

@AdSchellevis we can try the upstream one. I think the openbsd one may fix the original issue but maybe not all. All of this is a bit annoying because my first proposal was a different patch but that was ignored upstream, OpenBSD changed the approach, I changed it for upstream, it was ignored again and finally something was committed to "match" the OpenBSD patch. A bit of communication would have gone a long way.

[AdSchellevis]

just dumping this here for my own understanding, when max states is reached, reason should be set on our end to PFRES_MAXSTATES

src/sys/netpfil/pf/pf.c

Lines 4852 to 4855 in 6e45a8d

	if (r->max_states &&
	(counter_u64_fetch(r->states_cur) >= r->max_states)) {
	counter_u64_add(V_pf_status.lcounters[LCNT_STATES], 1);
	REASON_SET(&reason, PFRES_MAXSTATES);

But since the rule number ->nr is likely non negative, it would use the pass as specified in the rule. not sure how this should work, but confusing it certainly is