On Fri, May 03, 2013 at 02:20 +0400, Vitaly Sinilin wrote:
> 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/[email protected]/msg09231.html
> [2] http://www.mail-archive.com/[email protected]/msg95116.html
>
> --
> Vitaly Sinilin <[email protected]>
>
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);
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,
