pf uses freed memory

[Vitaly Sinilin]

Hi OpenBSD community,

I have been using OpenBSD for about two weeks. I switched from Linux
because CRUX, my favorite distro, doesn't support i386 anymore and
nearly all my hardware is 32-bit. So I am completely new to BSD systems.

Anyway, recently I installed OpenBSD 5.2 on a i386 box that is a
gateway/firewall for my home network. I setup pf to forward some
external ports to a Linux box in my local network with a rdr-to
rule from only a number of trusted hosts.

After a while I noticed warnings on the Linux box that said that it
receives packets from an untrusted host. All of them had the same
source IP 239.255.170.187 that might would have said something to
some of you. Anyway, we will get back to it later.

I googled this IP and found a couple of mail threads ([1] and [2])
referring to it and all of them were related to OpenBSD and
unfortunately had no solution. (At least as far as switching to Debian
isn't considered a solution :-)

I've done some investigation with tcpdump and found out that about
every fouth or so UDP packet from my SIP provider had its source
IP address changed by the OpenBSD box and was forwarded to the Linux
box. It surprised me a lot because OpenBSD has a very good reputation
and such errors are impossible in it I thought. So I rechecked my pf
rules over and over again in order to understand what I am doing wrong,
but saw no mistakes there.

Finally, I decided to take a look at the sources. Initially, I had no
intention to become a BSD kernel hacker during the first month of using
of the system but life is hard sometimes and I had to do it :-)

Well, after two days of debugging I found out the root cause of the
problem. It was in the function pf_state_key_attach() that under some
circumstances returns its first parameter to a pool (pf.c, line 719),
but at the same time up by stack this parameter (pf_state_key) is
passed to the function pf_translate() (pf.c, line 3616) in order to
override source address of the packet.

I've done some fix, that works for me and is meant to be as small as
possible. Again, as I am a newcomer to BSD the fix may be not
perfect, but at least it should be enough to get the idea.

Now I upgraded to 5.3 and the bug is still in place. So I rebased my
changes for 5.3. The patch is attached.

Lastly, as I promised to tell, the IP 239.255.170.187 is a value of
the macro DEADBEEF1 for the i386 architecture that is used as a magic
number for memory chunks returned to a pool. That guy in Mexico who has
this IP must be a big lucky :-)

Thanks!

[1] http://www.mail-archive.com/p...@benzedrine.cx/msg09231.html
[2] http://www.mail-archive.com/mi...@openbsd.org/msg95116.html

--
Vitaly Sinilin <v...@kp4.ru>

[demime 1.01d removed an attachment of type application/octet-stream which had a name of pf_freed_sk_53.patch]

[Vitaly Sinilin]

Looks like the patch was filtered out from the original message.
So here it is in the body.

Index: net/if_pfsync.c
===================================================================
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.195
diff -u -r1.195 if_pfsync.c
--- net/if_pfsync.c 30 Oct 2012 12:09:05 -0000 1.195
+++ net/if_pfsync.c 2 May 2013 16:50:36 -0000
@@ -613,7 +613,7 @@
if (!ISSET(flags, PFSYNC_SI_IOCTL))
SET(st->state_flags, PFSTATE_NOSYNC);

- if (pf_state_insert(kif, skw, sks, st) != 0) {
+ if (pf_state_insert(kif, &skw, &sks, st) != 0) {
/* XXX when we have anchors, use STATE_DEC_COUNTERS */
r->states_cur--;
error = EEXIST;
Index: net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.818
diff -u -r1.818 pf.c
--- net/pf.c 20 Jan 2013 22:51:16 -0000 1.818
+++ net/pf.c 2 May 2013 16:50:38 -0000
@@ -667,9 +667,10 @@
}

int
-pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int
idx) +pf_state_key_attach(struct pf_state_key **skp, struct pf_state
*s, int idx) {
struct pf_state_item *si;
+ struct pf_state_key *sk = *skp;
struct pf_state_key *cur;
struct pf_state *olds = NULL;

@@ -718,6 +719,7 @@
}
pool_put(&pf_state_key_pl, sk);
s->key[idx] = cur;
+ *skp = cur;
} else
s->key[idx] = sk;

@@ -916,13 +918,13 @@
}

int
-pf_state_insert(struct pfi_kif *kif, struct pf_state_key *skw,
- struct pf_state_key *sks, struct pf_state *s)
+pf_state_insert(struct pfi_kif *kif, struct pf_state_key **skw,
+ struct pf_state_key **sks, struct pf_state *s)
{
splsoftassert(IPL_SOFTNET);

s->kif = kif;
- if (skw == sks) {
+ if (*skw == *sks) {
if (pf_state_key_attach(skw, s, PF_SK_WIRE))
return (-1);
s->key[PF_SK_STACK] = s->key[PF_SK_WIRE];
@@ -3790,7 +3792,7 @@
goto csfailed;
}

- if (pf_state_insert(BOUND_IFACE(r, pd->kif), *skw, *sks, s)) {
+ if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
pf_state_key_detach(s, PF_SK_STACK);
pf_state_key_detach(s, PF_SK_WIRE);
*sks = *skw = NULL;
Index: net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.376
diff -u -r1.376 pfvar.h
--- net/pfvar.h 17 Jan 2013 00:48:04 -0000 1.376
+++ net/pfvar.h 2 May 2013 16:50:39 -0000
@@ -1746,8 +1746,8 @@
extern void pf_unlink_state(struct pf_state *);
extern void pf_free_state(struct pf_state *);
extern int pf_state_insert(struct pfi_kif *,
- struct pf_state_key *,
- struct pf_state_key *,
+ struct pf_state_key **,
+ struct pf_state_key **,
struct pf_state *);
int pf_insert_src_node(struct
pf_src_node **, struct pf_rule *, enum pf_sn_types,
@@ -1827,7 +1827,7 @@
int pf_socket_lookup(struct pf_pdesc *);
struct pf_state_key *pf_alloc_state_key(int);
void pf_pkt_addr_changed(struct mbuf *);
-int pf_state_key_attach(struct pf_state_key *, struct pf_state
*, int); +int pf_state_key_attach(struct pf_state_key **, struct
pf_state *, int); int pf_translate(struct pf_pdesc *, struct
pf_addr *, u_int16_t, struct pf_addr *, u_int16_t, u_int16_t, int);
int pf_translate_af(struct pf_pdesc *);


--
Vitaly Sinilin <v...@kp4.ru>

[Stuart Henderson]

On 2013/05/03 02:27, Vitaly Sinilin wrote:
> Looks like the patch was filtered out from the original message.
> So here it is in the body.

Patch had line-wrap problems, here is a cleaned one (but otherwise
untested).

Index: pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.377
diff -u -p -r1.377 pfvar.h
--- pfvar.h 11 Mar 2013 19:48:40 -0000 1.377
+++ pfvar.h 2 May 2013 22:47:52 -0000
@@ -1748,8 +1748,8 @@ extern void pf_purge_expired_states(u

extern void pf_unlink_state(struct pf_state *);
extern void pf_free_state(struct pf_state *);
extern int pf_state_insert(struct pfi_kif *,
- struct pf_state_key *,
- struct pf_state_key *,
+ struct pf_state_key **,
+ struct pf_state_key **,
struct pf_state *);
int pf_insert_src_node(struct pf_src_node **,
struct pf_rule *, enum pf_sn_types,

@@ -1829,7 +1829,7 @@ int pf_rtlabel_match(struct pf_addr *, s

int pf_socket_lookup(struct pf_pdesc *);
struct pf_state_key *pf_alloc_state_key(int);
void pf_pkt_addr_changed(struct mbuf *);
-int pf_state_key_attach(struct pf_state_key *, struct pf_state *, int);
+int pf_state_key_attach(struct pf_state_key **, struct pf_state *, int);
int pf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t,
struct pf_addr *, u_int16_t, u_int16_t, int);
int pf_translate_af(struct pf_pdesc *);

Index: pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.822
diff -u -p -r1.822 pf.c
--- pf.c 10 Apr 2013 08:50:59 -0000 1.822
+++ pf.c 2 May 2013 22:47:52 -0000
@@ -667,9 +667,10 @@ pf_state_compare_id(struct pf_state *a,

}

int
-pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int idx)
+pf_state_key_attach(struct pf_state_key **skp, struct pf_state *s, int idx)
{
struct pf_state_item *si;
+ struct pf_state_key *sk = *skp;
struct pf_state_key *cur;
struct pf_state *olds = NULL;

@@ -718,6 +719,7 @@ pf_state_key_attach(struct pf_state_key

}
pool_put(&pf_state_key_pl, sk);
s->key[idx] = cur;
+ *skp = cur;
} else
s->key[idx] = sk;

@@ -916,13 +918,13 @@ pf_state_key_setup(struct pf_pdesc *pd,

}

int
-pf_state_insert(struct pfi_kif *kif, struct pf_state_key *skw,
- struct pf_state_key *sks, struct pf_state *s)
+pf_state_insert(struct pfi_kif *kif, struct pf_state_key **skw,
+ struct pf_state_key **sks, struct pf_state *s)
{
splsoftassert(IPL_SOFTNET);

s->kif = kif;
- if (skw == sks) {
+ if (*skw == *sks) {
if (pf_state_key_attach(skw, s, PF_SK_WIRE))
return (-1);
s->key[PF_SK_STACK] = s->key[PF_SK_WIRE];

@@ -3789,7 +3791,7 @@ pf_create_state(struct pf_pdesc *pd, str

goto csfailed;
}

- if (pf_state_insert(BOUND_IFACE(r, pd->kif), *skw, *sks, s)) {
+ if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
pf_state_key_detach(s, PF_SK_STACK);
pf_state_key_detach(s, PF_SK_WIRE);
*sks = *skw = NULL;

Index: if_pfsync.c
===================================================================
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.197
diff -u -p -r1.197 if_pfsync.c
--- if_pfsync.c 28 Mar 2013 16:45:16 -0000 1.197
+++ if_pfsync.c 2 May 2013 22:47:52 -0000
@@ -612,7 +612,7 @@ pfsync_state_import(struct pfsync_state

[Mike Belopuhov]

Hi,

Thanks for figuring this out. Unfortunately, I think your diff
is slightly wrong. Since skw and sks are distinct pointers you
need to keep them in sync when they're equal, otherwise sks will
point to the old memory. The diff below resets state keys after
pf_state_key_attach. Would you be so kind to verify that it
still fixes the original problem?

Cheers,
Mike

diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
index fdbc665..2003325 100644
--- sys/net/if_pfsync.c
+++ sys/net/if_pfsync.c
@@ -612,7 +612,7 @@ pfsync_state_import(struct pfsync_state *sp, int flags)

if (!ISSET(flags, PFSYNC_SI_IOCTL))
SET(st->state_flags, PFSTATE_NOSYNC);

- if (pf_state_insert(kif, skw, sks, st) != 0) {
+ if (pf_state_insert(kif, &skw, &sks, st) != 0) {
/* XXX when we have anchors, use STATE_DEC_COUNTERS */
r->states_cur--;
error = EEXIST;

diff --git sys/net/pf.c sys/net/pf.c
index e98f265..9964288 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -916,25 +916,28 @@ pf_state_key_setup(struct pf_pdesc *pd, struct pf_state_key **skw,

}

int
-pf_state_insert(struct pfi_kif *kif, struct pf_state_key *skw,
- struct pf_state_key *sks, struct pf_state *s)
+pf_state_insert(struct pfi_kif *kif, struct pf_state_key **skw,
+ struct pf_state_key **sks, struct pf_state *s)
{
splsoftassert(IPL_SOFTNET);

s->kif = kif;
- if (skw == sks) {

- if (pf_state_key_attach(skw, s, PF_SK_WIRE))

+ if (*skw == *sks) {

+ if (pf_state_key_attach(*skw, s, PF_SK_WIRE))
return (-1);
+ *skw = *sks = s->key[PF_SK_WIRE];

s->key[PF_SK_STACK] = s->key[PF_SK_WIRE];

} else {
- if (pf_state_key_attach(skw, s, PF_SK_WIRE)) {
+ if (pf_state_key_attach(*skw, s, PF_SK_WIRE)) {
pool_put(&pf_state_key_pl, sks);
return (-1);
}
- if (pf_state_key_attach(sks, s, PF_SK_STACK)) {
+ *skw = s->key[PF_SK_WIRE];
+ if (pf_state_key_attach(*sks, s, PF_SK_STACK)) {
pf_state_key_detach(s, PF_SK_WIRE);
return (-1);
}
+ *sks = s->key[PF_SK_STACK];
}

if (s->id == 0 && s->creatorid == 0) {
@@ -3789,7 +3792,7 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,

goto csfailed;
}

- if (pf_state_insert(BOUND_IFACE(r, pd->kif), *skw, *sks, s)) {
+ if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
pf_state_key_detach(s, PF_SK_STACK);
pf_state_key_detach(s, PF_SK_WIRE);
*sks = *skw = NULL;

diff --git sys/net/pfvar.h sys/net/pfvar.h
index 67eb9a7..19e40b5 100644
--- sys/net/pfvar.h
+++ sys/net/pfvar.h
@@ -1748,8 +1748,8 @@ extern void pf_purge_expired_states(u_int32_t);

[Vitaly Sinilin]

On Fri, 3 May 2013 21:26:57 +0200
Mike Belopuhov <mi...@belopuhov.com> wrote:

[skip]

> Thanks for figuring this out. Unfortunately, I think your diff
> is slightly wrong. Since skw and sks are distinct pointers you
> need to keep them in sync when they're equal, otherwise sks will
> point to the old memory. The diff below resets state keys after
> pf_state_key_attach.

Yes, you are right. As this branch (*sks == *skw) wasn't the case for
my problem I didn't pay enough attention to it. My bad.

> Would you be so kind to verify that it still fixes the original
> problem?

Just tested the kernel with the modified patch applied. It works for me
as well.

> diff --git sys/net/if_pfsync.c sys/net/if_pfsync.c
> index fdbc665..2003325 100644
> --- sys/net/if_pfsync.c
> +++ sys/net/if_pfsync.c

[offtopic]
I see you are using git and wonder if there is a well known convenient
way to use git and cvs together. Could you share your way of doing
this?
[/offtopic]

--
Vitaly Sinilin <v...@kp4.ru>