On Tue, Jun 21, 2016 at 16:08 +0200, Alexander Bluhm wrote:
> On Tue, Jun 21, 2016 at 02:45:42PM +0200, Mike Belopuhov wrote:
> > You're testing the sequence number
> > of the new state with an existing one which has seen some
> > traffic.. Are you sure this is correct?
>
> This is exactly what the stack does to distinguish between packets
> that belong to an old connection and between a new one.
>
Right, I've found it, but how can you tell that this is a new
connection if iss changes a lot and you just test if it's greater
than? The actual test should be if it's ouside of the window,
isn't it?
> > But
> > since they don't belong to the same connection, this is
> > inherently incorrect.
>
> Here is my logical error. The receiving TCP stack only thinks it
> is a reuse of an existing connection. In this case, the sender is
> reponsible to increase the initial sequence number. But the sender
> has a different port or even a different IP. It is only NAT reusing
> the port and address which confuses the stack.
>
Yep.
> So my diff fixes it because now pf state key attach and tcp reuse
> are confused in the same way.
>
Rather a bit sloppy, but yeah.
> > Unless I'm wrong, I have to retract my OK and ask you to fix
> > the sport bit instead.
>
> Yes, fixing it in pf_get_sport() is more correct. I will try
> to make a diff.
>
Cool.
> It would help a lot debugging such problems, if we would have more
> log messages. So extend the existing log to the reuse case in
> pf_state_key_attach() ?
>
> ok?
>
Sure.
> bluhm
>
> Index: net/pf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.977
> diff -u -p -r1.977 pf.c
> --- net/pf.c 15 Jun 2016 11:49:34 -0000 1.977
> +++ net/pf.c 21 Jun 2016 13:50:01 -0000
> @@ -669,34 +669,34 @@ pf_state_key_attach(struct pf_state_key
> si->s->key[PF_SK_STACK]->af &&
> sk->af == si->s->key[PF_SK_STACK]->af &&
> si->s->direction != s->direction))) {
> + int reuse = 0;
> +
> if (sk->proto == IPPROTO_TCP &&
> si->s->src.state >= TCPS_FIN_WAIT_2 &&
> - si->s->dst.state >= TCPS_FIN_WAIT_2) {
> + si->s->dst.state >= TCPS_FIN_WAIT_2)
> + reuse = 1;
> + if (pf_status.debug >= LOG_NOTICE) {
> + log(LOG_NOTICE,
> + "pf: %s key attach %s on %s: ",
> + (idx == PF_SK_WIRE) ?
> + "wire" : "stack",
> + reuse ? "reuse" : "failed",
> + s->kif->pfik_name);
> + pf_print_state_parts(s,
> + (idx == PF_SK_WIRE) ? sk : NULL,
> + (idx == PF_SK_STACK) ? sk : NULL);
> + addlog(", existing: ");
> + pf_print_state_parts(si->s,
> + (idx == PF_SK_WIRE) ? sk : NULL,
> + (idx == PF_SK_STACK) ? sk : NULL);
> + addlog("\n");
> + }
> + if (reuse) {
> si->s->src.state = si->s->dst.state =
> TCPS_CLOSED;
> /* remove late or sks can go away */
> olds = si->s;
> } else {
> - if (pf_status.debug >= LOG_NOTICE) {
> - log(LOG_NOTICE,
> - "pf: %s key attach "
> - "failed on %s: ",
> - (idx == PF_SK_WIRE) ?
> - "wire" : "stack",
> - s->kif->pfik_name);
> - pf_print_state_parts(s,
> - (idx == PF_SK_WIRE) ?
> - sk : NULL,
> - (idx == PF_SK_STACK) ?
> - sk : NULL);
> - addlog(", existing: ");
> - pf_print_state_parts(si->s,
> - (idx == PF_SK_WIRE) ?
> - sk : NULL,
> - (idx == PF_SK_STACK) ?
> - sk : NULL);
> - addlog("\n");
> - }
> pool_put(&pf_state_key_pl, sk);
> return (-1); /* collision! */
> }
