pf: Clear state key pointers after freeing them

[markj]

Summary

A bit further below, if rewrite != 0, then we may dereference these
points if they're not NULL. If that can happen, it would be a UAF.

Clear the pointers. Or perhaps the frees should be deferred to after
the check?

Noticed during code inspection, so I have no test case to offer.

[kp]

This is fine. We don't want to try to do nat64 if we've not created state anyway.
OpenBSD doesn't have this problem here because the sk/nk allocation is different, but they also never return PF_AFRT unless we've created state.

[kp]

I've added this as well:

diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 17bb05b5f6b9..29b1b506ad12 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1836,6 +1836,7 @@ pf_state_insert(struct pfi_kkif *kif, struct pfi_kkif *orig_kif,
        /* Returns with ID locked on success. */
        if ((error = pf_state_key_attach(skw, sks, s)) != 0)
                return (error);
+       skw = sks = NULL;

        ih = &V_pf_idhash[PF_IDHASH(s)];
        PF_HASHROW_ASSERT(ih);
@@ -5970,6 +5971,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
                action = pf_create_state(r, nr, a, pd, nk, sk,
                    &rewrite, sm, tag, bproto_sum, bip_sum,
                    &match_rules, udp_mapping);
+               sk = nk = NULL;
                if (action != PF_PASS) {
                        pf_udp_mapping_release(udp_mapping);
                        pd->act.log |= PF_LOG_FORCE;
@@ -6235,6 +6237,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a,
                goto drop;
        } else
                *sm = s;
+       sk = nk = NULL;

        STATE_INC_COUNTERS(s);

It does need other work too, which I'll post soon. It just seems silly to have two commits doing essentially the same thing to different pieces of code.

[markj]

In D49520#1129648, @kp wrote:

I've added this as well:

diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 17bb05b5f6b9..29b1b506ad12 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1836,6 +1836,7 @@ pf_state_insert(struct pfi_kkif *kif, struct pfi_kkif *orig_kif,
        /* Returns with ID locked on success. */
        if ((error = pf_state_key_attach(skw, sks, s)) != 0)
                return (error);
+       skw = sks = NULL;

        ih = &V_pf_idhash[PF_IDHASH(s)];
        PF_HASHROW_ASSERT(ih);
@@ -5970,6 +5971,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
                action = pf_create_state(r, nr, a, pd, nk, sk,
                    &rewrite, sm, tag, bproto_sum, bip_sum,
                    &match_rules, udp_mapping);
+               sk = nk = NULL;
                if (action != PF_PASS) {
                        pf_udp_mapping_release(udp_mapping);
                        pd->act.log |= PF_LOG_FORCE;
@@ -6235,6 +6237,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a,
                goto drop;
        } else
                *sm = s;
+       sk = nk = NULL;

        STATE_INC_COUNTERS(s);

It does need other work too, which I'll post soon. It just seems silly to have two commits doing essentially the same thing to different pieces of code.

Sure, I'll drop this diff if you're going to roll it into your patch set somehow.

[kp]

In D49520#1129668, @markj wrote:

Sure, I'll drop this diff if you're going to roll it into your patch set somehow.

I meant for you to roll this in your patch, but that works too :)