[Bug 296838] pf: cannot reload ruleset in securelevel 2

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=3D296838

--- Comment #8 from Christian Ullrich <[email protected]> ---
(In reply to crest from comment #5)
> -	    securelevel_ge(nlp_get_cred(nlp), cmd->cmd_securelevel)) {
> +	    securelevel_gt(nlp_get_cred(nlp), cmd->cmd_securelevel)) {
>  		NLP_LOG(LOG_DEBUG, nlp, "family %s: cmd %d securelevel_gt() failed",

Note that the error message even now mentions the presumably correct functi=
on.
I think we can be confident that _gt() really was meant.

However, ISTM there is a major bug in the entire idea. Commit 47c12f20bf58b
introduced the feature of associating a securelevel with a netlink command =
(as
well as the check above), and this new field is documented like this:

--- a/sys/netlink/netlink_ctl.h
+++ b/sys/netlink/netlink_ctl.h
@@ -89,6 +89,9 @@ struct genl_cmd {
        uint32_t        cmd_flags;
        uint32_t        cmd_priv;
        uint32_t        cmd_num;
+
+       /* Disallow this call from this level up (inclusive). */
+       uint32_t        cmd_securelevel;
 };

This requires comparing _ge(). However, the same commit then sets that leve=
l to
either 2 or 3 for every single netlink command pf has.

Before the change it was possible to get the ruleset, state table, table
contents, etc. even in securelevel 3; now _every_ interaction with pf is
forbidden at that level. The levels added to the netlink commands are clear=
ly
meant as "allowed up to here" rather than "forbidden from here on".

I think the author first decided to introduce genl_cmd.cmd_securelevel as t=
he
comment says (forbidden at this level and above), then became confused when
updating the pf netlink calls: Most of the status queries would either have=
 to
be not forbidden at all, or starting _above_ securelevel 3, and I find it
really difficult to think of "securelevel 4" when you are used to the featu=
re.

They were also persuaded to special-case cmd_securelevel=3D=3D0 in D56390. =
This was
necessary to prevent a difference in behavior between securelevel -1 and 0.=
 The
code does "if securelevel > 0 then compare _ge()" to maintain the
"cmd_securelevel is the lowest prohibited level but 0 means always allowed"
logic.

It would have been better, IMHO, to define cmd_securelevel as the highest
permitted level and always compare _gt(), as suggested here and apparently
intended at least for pf (based on the error message) but contradicted by t=
he
comment to the new cmd_securelevel field. It would also make much more sens=
e in
the context of the genl_cmd struct itself because, seeing a restriction def=
ined
for a command, I'd expect it to describe when that command works, not when =
it
doesn't.

Cleaning this up, when it may be used elsewhere, is going to be so much fun=
 for
whoever gets to do it.

--=20
You are receiving this mail because:
You are the assignee for the bug.=