On Sun, Sep 03, 2023 at 04:12:35AM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> so there is actually bug. I was able to reproduce it with very simple
> rules on my router:
>
> set skip on em1
> block return all
> pass out on em0 from 192.168.2.0/24 to any nat-to(em0)
>
> em1 is interface, facing to LAN
> em0 is interface to internet where NAT happens.
>
> I did use a scapy to generate ICMP host unreachable:
> d = '77.75.77.222'
> s = '192.168.2.5'
> sp = 1234
> dp = 12345
> pkt = Ether()/IP(dst=d)/ICMP(type=3, code=1, )/\
> IP(src=d', dst=s)/UDP(sport=sp, dport=dp)
> sendp(pkt, iface='iwn0')
>
> the packet has been sent from my notebook via LAN towards router. to my
> surprise router forwards packet untranslated without creating a state.
>
> the packet indeed matches the 'pass out on em0 rule...'
> however 'keep state' and 'nat-to' actions are ignored. To understand
> why we must look here at pf_test_rule():
>
> 4475 if (pd->virtual_proto != PF_VPROTO_FRAGMENT
> 4476 && !ctx.state_icmp && r->keep_state) {
> 4477
> ....
> 4491 action = pf_create_state(pd, r, a, ctx.nr, &skw, &sks,
> 4492 &rewrite, sm, ctx.tag, &ctx.rules, &ctx.act,
> ctx.sns);
> 4493
> 4494 if (action != PF_PASS)
> 4495 goto cleanup;
> 4496 if (sks != skw) {
> 4497 struct pf_state_key *sk;
> 4498
> 4499 if (pd->dir == PF_IN)
> 4500 sk = sks;
> 4501 else
> 4502 sk = skw;
> 4503 rewrite += pf_translate(pd,
> 4504 &sk->addr[pd->af == pd->naf ? pd->sidx :
> pd->didx],
> 4505 sk->port[pd->af == pd->naf ? pd->sidx :
> pd->didx],
> 4506 &sk->addr[pd->af == pd->naf ? pd->didx :
> pd->sidx],
> 4507 sk->port[pd->af == pd->naf ? pd->didx :
> pd->sidx],
> 4508 virtual_type, ctx.icmp_dir);
> 4509 }
> 4510
> 4511 #ifdef INET6
> 4512 if (rewrite && skw->af != sks->af)
> 4513 action = PF_AFRT;
> 4514 #endif /* INET6 */
> 4515
> 4516 } else {
> 4517 while ((ctx.ri = SLIST_FIRST(&ctx.rules))) {
> 4518 SLIST_REMOVE_HEAD(&ctx.rules, entry);
> 4519 pool_put(&pf_rule_item_pl, ctx.ri);
> 4520 }
> 4521 }
>
> note '!ctx.state_icmp' at line 4476. This variable is being set
> by pf_icmp_mapping() function very early in pf_test_rule() function.
>
> 4354 switch (pd->virtual_proto) {
> 4355 case IPPROTO_ICMP:
> 4356 ctx.icmptype = pd->hdr.icmp.icmp_type;
> 4357 ctx.icmpcode = pd->hdr.icmp.icmp_code;
> 4358 ctx.state_icmp = pf_icmp_mapping(pd, ctx.icmptype,
> 4359 &ctx.icmp_dir, &virtual_id, &virtual_type);
> 4360 if (ctx.icmp_dir == PF_IN) {
> 4361 pd->osport = pd->nsport = virtual_id;
>
> for ICMP error messages the ctx.state_icmp value is set to one.
> It happens right here in pf_icmp_mapping():
>
> 2582 /* These ICMP types map to other connections */
> 2583 case ICMP_UNREACH:
> 2584 case ICMP_SOURCEQUENCH:
> 2585 case ICMP_REDIRECT:
> 2586 case ICMP_TIMXCEED:
> 2587 case ICMP_PARAMPROB:
> 2588 /* These will not be used, but set them anyway */
> 2589 *icmp_dir = PF_IN;
> 2590 *virtual_type = htons(type);
> 2591 *virtual_id = 0;
> 2592 return (1); /* These types match to another
> state */
>
>
> this explains why pf ignores 'keep state' and 'nat-to' action for ICMP errors.
>
> It's tempting to fix things up in pf_test_rule() in else branch lines
> 4517-4520.
> However I'm afraid this approach may create some undesired side-effect.
> in my opinion is to fix pf_match_rule() function, so ICMP error message
> will no longer match 'keep state' rule. Diff below is for IPv4. I still
> need to think of more about IPv6. My gut feeling is it will be very similar.
>
> thanks and
> regards
> sashan
Hi Alexandr,
Give me until monday to test this patch as I'm not at home currently. Thank
you for taking time out of your WE to test this. Emotionally I'm happy and
sad at the same time. Happy, that the report was not wrong, and sad that it
is correct. Also my custom spoofer has once again found a bug in OpenBSD I
don't know if I should be proud of that, or not.
Best Regards,
-peter
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 4f0fc3f91a9..0993aed85fb 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -4148,6 +4148,9 @@ enter_ruleset:
> (r->rule_flag & PFRULE_STATESLOPPY) == 0 &&
> ctx->icmp_dir != PF_IN),
> TAILQ_NEXT(r, entries));
> + /* icmp packet must match existing state */
> + PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp,
> + TAILQ_NEXT(r, entries);
> break;
>
> case IPPROTO_ICMPV6:
--
Over thirty years experience on Unix-like Operating Systems starting with QNX.
