I'm looking for oks on this diff to commit it.
* Leonardo Guardati <leona...@guardati.it> [2012-05-10 21:29]:
> Hi,
> here is a solution to the problem I posted on bugs@ about pf logging
> incoming UDP packets to port 0 as pass while being blocked instead.
>
> action is added to pflog_packet() arguments.
>
> I tried it and works.
>
>
> Here are the diffs:
> --- if_pflog.c 2012-05-10 20:04:40.164000084 +0200
> +++ if_pflog-new.c 2012-05-10 15:21:23.000000000 +0200
> @@ -215,7 +215,7 @@ pflogioctl(struct ifnet *ifp, u_long cmd
> }
>
> int
> -pflog_packet(struct pf_pdesc *pd, u_int8_t reason, struct pf_rule *rm,
> +pflog_packet(struct pf_pdesc *pd, u_int8_t reason, u_int8_t action,
> struct pf_rule *rm,
> struct pf_rule *am, struct pf_ruleset *ruleset)
> {
> #if NBPFILTER > 0
> @@ -230,7 +230,7 @@ pflog_packet(struct pf_pdesc *pd, u_int8
>
> bzero(&hdr, sizeof(hdr));
> hdr.length = PFLOG_REAL_HDRLEN;
> - hdr.action = rm->action;
> + hdr.action = action;
> hdr.reason = reason;
> memcpy(hdr.ifname, pd->kif->pfik_name, sizeof(hdr.ifname));
> --- if_pflog.h 2012-05-10 20:04:46.806000082 +0200
> +++ if_pflog-new.h 2012-05-10 15:22:25.000000000 +0200
> @@ -71,9 +71,9 @@ struct pfloghdr {
> void pflog_bpfcopy(const void *, void *, size_t);
>
> #if NPFLOG > 0
> -#define PFLOG_PACKET(a,b,c,d,e) pflog_packet(a,b,c,d,e)
> +#define PFLOG_PACKET(a,b,c,d,e,f) pflog_packet(a,b,c,d,e,f)
> #else
> -#define PFLOG_PACKET(a,b,c,d,e) ((void)0)
> +#define PFLOG_PACKET(a,b,c,d,e,f) ((void)0)
> #endif /* NPFLOG > 0 */
> #endif /* _KERNEL */
> #endif /* _NET_IF_PFLOG_H_ */
> --- pfvar.h 2012-05-10 20:05:05.316000084 +0200
> +++ pfvar-new.h 2012-05-10 15:40:37.000000000 +0200
> @@ -1795,7 +1795,7 @@ void pf_addr_inc(struct pf_addr *, sa_fa
> void *pf_pull_hdr(struct mbuf *, int, void *, int, u_short *, u_short *,
> sa_family_t);
> void pf_change_a(void *, u_int16_t *, u_int32_t, u_int8_t);
> -int pflog_packet(struct pf_pdesc *, u_int8_t, struct pf_rule *,
> +int pflog_packet(struct pf_pdesc *, u_int8_t, u_int8_t, struct pf_rule *,
> struct pf_rule *, struct pf_ruleset *);
> void pf_send_deferred_syn(struct pf_state *);
> int pf_match_addr(u_int8_t, struct pf_addr *, struct pf_addr *,
> --- pf.c 2012-05-10 20:05:04.902000084 +0200
> +++ pf-new.c 2012-05-10 16:15:46.000000000 +0200
> @@ -3492,14 +3492,16 @@ pf_test_rule(struct pf_pdesc *pd, struct
> goto cleanup;
> }
> if (r->log || act.log & PF_LOG_MATCHES)
> - PFLOG_PACKET(pd, reason, r, a, ruleset);
> + PFLOG_PACKET(pd, reason, r->action, r,
> + a, ruleset);
> } else {
> match = 1;
> *rm = r;
> *am = a;
> *rsm = ruleset;
> if (act.log & PF_LOG_MATCHES)
> - PFLOG_PACKET(pd, reason, r, a, ruleset);
> + PFLOG_PACKET(pd, reason, r->action, r,
> + a, ruleset);
> }
>
> if ((*rm)->quick)
> @@ -3529,7 +3531,7 @@ pf_test_rule(struct pf_pdesc *pd, struct
> REASON_SET(&reason, PFRES_MATCH);
>
> if (r->log || act.log & PF_LOG_MATCHES)
> - PFLOG_PACKET(pd, reason, r, a, ruleset);
> + PFLOG_PACKET(pd, reason, r->action, r, a, ruleset);
>
> if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&
> (r->action == PF_DROP) &&
> @@ -6951,12 +6953,12 @@ done:
> struct pf_rule_item *ri;
>
> if (pd.pflog & PF_LOG_FORCE || r->log & PF_LOG_ALL)
> - PFLOG_PACKET(&pd, reason, r, a, ruleset);
> + PFLOG_PACKET(&pd, reason, action, r, a, ruleset);
> if (s) {
> SLIST_FOREACH(ri, &s->match_rules, entry)
> if (ri->r->log & PF_LOG_ALL)
> - PFLOG_PACKET(&pd, reason, ri->r, a,
> - ruleset);
> + PFLOG_PACKET(&pd, reason, action,
> + ri->r, a, ruleset);
> }
> }
> Are they ok?
>
> Leonardo.
>
>
>
> -------- Original Message --------
> Subject: pf logs: def/(short) pass in , but should say block
> Date: Wed, 09 May 2012 01:29:16 +0200
> From: Leonardo Guardati <leona...@guardati.it>
> To: b...@openbsd.org
>
> Hi,
> here is the setup ( I saw this on 5.0, 5.1, 5.1-current).
>
> On a fresh installation.
>
> /etc/pf.conf:
> block log all
>
> I send to the machine an UDP packet to port 0. I used from another machine:
> nmap -Pn -sU -pU:0 target_ip
>
> pf will log the packet as pass.
> ... rule def/(short) pass in on ...
>
> I tried to debug the code.
>
> I see the packet is dropped (pf_test() returns PF_DROP), but
> pflog_packet() doesn't know it.
>
> Here is what I saw:
>
> in net/pf.c:6557 pf_setup_desc() decides to return (PF_DROP); because
> uh->hu_dport==0. (reason PFRES_SHORT).
>
> in net/pf.c:6716: action = pf_setup_desc(); is set and goto done;
>
> then in net/pf.c:6954
> PFLOG_PACKET(&pd, reason, r, a, ruleset) is called.
>
> pflog_packet() gets the 'reason' code, but doesn't get the 'action'
> value set in net/pf.c:7616, also there is no rule match ( goto done;
> right after setting action=PF_DROP), so 'r' still points to
> pf_default_rule (which has a default PF_PASS).
>
> The outcome is: I read a pass in the logs, while the packet is blocked
> instead.
>
> I tried to think to possible solutions, but this is the first time I
> look into the OpenBSD kernel, so I'm just not aware of the code
> interconnections and how things should be.
> For example: why UDP destintation port 0 doesn't match 'block log all'
> rule in the first place?
>
> Anyway I say what I think:
> - could it be possible to add the 'action' variable to pflog_packet()
> arguments and then if 'r' points to pf_default_rule, 'action' is considered?
> - could make sense specialize the pf_default_rule default action based
> on the reason code? ( in this case, PF_SHORT should imply a PF_DROP)
> (maybe using a switch(reason) in net/if_pflog.c:233 )
>
> Another thing I noticed; in net/pf.c:6950 happen the logging, but after
> that there are other assignments to action. So I suppose there could be
> other ways to log a packet as block or pass with a different effective
> action.
>
> Leonardo.
