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

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

--- Comment #8 from Christian Ullrich <chris@chrullrich.net> ---
(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 function.
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 level 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 clearly
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 the
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 feature.

They were also persuaded to special-case cmd_securelevel==0 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 the
comment to the new cmd_securelevel field. It would also make much more sense in
the context of the genl_cmd struct itself because, seeing a restriction defined
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.

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