Re: mismatch for ICMP state created by inound response
Hello, On Tue, May 19, 2015 at 14:07 +0200, Alexandr Nedvedicky wrote: Hello Mike, I've reworked patch from yesterday. I've done some quick testing to see if it fixes problem. It looks like it works. I have not tested NAT-64 yet. Also I'd like to come up with test case, which will show the state check is still able to block invalid ICMP packet (invalid with respect to state). The idea of fix is to keep icmp_dir in state as well. The icmp_dir indicates whether state got created by ICMP request or response. This is useful later in pf_icmp_state_lookup() to check whether ICMP request/response matches state direction. This feels slightly convoluted... check my diff out! (: nice, I like your XOR Magic! comment. Looks like I was trying to fix the other end... your patch is minimalistic and correct as far as I can tell. P.S. I took discussion off-line not to create extra noise on tech@openbsd.org feel free go get the alias back to loop. Nah, that's what tech@ is for! O.K. I won't do it again... regards sasha
Re: ospfd announces carp interface with physical link down
On 20/05/15(Wed) 14:14, Johan Ymerson wrote: [...] The patch did not apply cleanly to OPENBSD_5_7, I rewrote the patch a bit: Thanks, I committed my diff to -current. With this patch everything (almost) work. At least as good as my patch did. OSPFd still does something wrong with the link state of carp interfaces when starting. Have a look at this: fw2:/usr/src/sys # ospfctl show int Interface AddressState HelloTimer Linkstate Uptimenc ac carp7 195.58.98.145/28 DOWN - backup 00:00:00 0 0 carp5 192.168.253.1/24 DOWN - backup 00:00:00 0 0 carp3 192.168.202.1/24 DOWN - backup 00:00:00 0 0 carp2 192.168.254.1/23 DOWN - invalid00:00:00 0 0 carp1 31.15.61.129/26DOWN - invalid00:00:00 0 0 carp0 92.33.0.202/30 DOWN - backup 00:00:00 0 0 bnx0192.168.200.5/24 OTHER 00:00:00 active 00:13:13 4 2 carp2 is (correctly) invalid, because the cable is plugged. carp1 is _not_ invalid. If I restart ospfd after the system has come up it looks better: carp1 31.15.61.129/26DOWN - backup 00:00:00 0 0 This happens with random interfaces at start-up. I believe this may be the cause: in usr.sbin/ospfd/interface.c, if_act_start(): if (!((iface-flags IFF_UP) LINK_STATE_IS_UP(iface-linkstate))) return (0); This check lack the exception for carp interfaces found in ospfe.c. If the interface already has been initialized when ospfd starts, it will not pick that interface up as a carp interface. I don't know much about ospfd but if changing this check solves your issues, feel free to send a diff. I'd suggest creating a new thread with an obvious name :)
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
Hello,
Hi,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error
nontrivial */
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the cleanup on error nontrivial. Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
We would definitely be interested in such a diff, but at the moment
there's no use for it as pf_test is called directly from the IP stack
and hence IP stack needs to support parallel execution first. This
doesn't mean that we can't start integrating bits and pieces, esp.
if they're presented as separate patches.
Thanks for your quality diffs btw, help is always much appreciated.
Cheers,
Mike
regards
sasha
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 15:20:01 -
@@ -3573,9 +3573,7 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
+ pf_unlink_state(s);
return (PF_DROP);
}
sni-sn = sns[i];
Re: More if_output()
On Tue, May 19, 2015 at 6:49 AM, Martin Pieuchot m...@openbsd.org wrote:
On 15/05/15(Fri) 15:53, Martin Pieuchot wrote:
Some more if_output() conversion. The xl bits are here because I'd
like to reduce the number of places where IFQ_ENQUEUE() is used.
After applying this diff you should only have a couple left.
Anyone?
no xl* here
Ok?
Index: dev/usb/if_upl.c
===
RCS file: /cvs/src/sys/dev/usb/if_upl.c,v
retrieving revision 1.64
diff -u -p -r1.64 if_upl.c
--- dev/usb/if_upl.c 10 Apr 2015 08:41:43 - 1.64
+++ dev/usb/if_upl.c 15 May 2015 13:43:51 -
@@ -888,28 +888,5 @@ int
upl_output(struct ifnet *ifp, struct mbuf *m, struct sockaddr *dst,
struct rtentry *rt0)
{
- int s, len, error;
-
- DPRINTFN(10,(%s: %s: enter\n,
- ((struct upl_softc *)ifp-if_softc)-sc_dev.dv_xname,
- __func__));
-
- len = m-m_pkthdr.len;
- s = splnet();
- /*
- * Queue message on interface, and start output if interface
- * not yet active.
- */
- IFQ_ENQUEUE(ifp-if_snd, m, NULL, error);
- if (error) {
- /* mbuf is already freed */
- splx(s);
- return (error);
- }
- ifp-if_obytes += len;
- if ((ifp-if_flags IFF_OACTIVE) == 0)
- (*ifp-if_start)(ifp);
- splx(s);
-
- return (0);
+ return (if_output(ifp, m));
}
Index: dev/ic/xl.c
===
RCS file: /cvs/src/sys/dev/ic/xl.c,v
retrieving revision 1.123
diff -u -p -r1.123 xl.c
--- dev/ic/xl.c 24 Mar 2015 11:23:02 - 1.123
+++ dev/ic/xl.c 15 May 2015 13:43:24 -
@@ -177,9 +177,6 @@ int xl_list_tx_init_90xB(struct xl_softc
void xl_wait(struct xl_softc *);
void xl_mediacheck(struct xl_softc *);
void xl_choose_xcvr(struct xl_softc *, int);
-#ifdef notdef
-void xl_testpacket(struct xl_softc *);
-#endif
int xl_miibus_readreg(struct device *, int, int);
void xl_miibus_writereg(struct device *, int, int, int);
@@ -659,35 +656,6 @@ xl_iff_905b(struct xl_softc *sc)
XL_SEL_WIN(7);
}
-
-#ifdef notdef
-void
-xl_testpacket(struct xl_softc *sc)
-{
- struct mbuf *m;
- struct ifnet*ifp;
- int error;
-
- ifp = sc-sc_arpcom.ac_if;
-
- MGETHDR(m, M_DONTWAIT, MT_DATA);
-
- if (m == NULL)
- return;
-
- memcpy(mtod(m, struct ether_header *)-ether_dhost,
- sc-sc_arpcom.ac_enaddr, ETHER_ADDR_LEN);
- memcpy(mtod(m, struct ether_header *)-ether_shost,
- sc-sc_arpcom.ac_enaddr, ETHER_ADDR_LEN);
- mtod(m, struct ether_header *)-ether_type = htons(3);
- mtod(m, unsigned char *)[14] = 0;
- mtod(m, unsigned char *)[15] = 0;
- mtod(m, unsigned char *)[16] = 0xE3;
- m-m_len = m-m_pkthdr.len = sizeof(struct ether_header) + 3;
- IFQ_ENQUEUE(ifp-if_snd, m, NULL, error);
- xl_start(ifp);
-}
-#endif
void
xl_setcfg(struct xl_softc *sc)
Index: net80211/ieee80211_input.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
retrieving revision 1.133
diff -u -p -r1.133 ieee80211_input.c
--- net80211/ieee80211_input.c14 Mar 2015 03:38:51 -
1.133
+++ net80211/ieee80211_input.c15 May 2015 13:43:51 -
@@ -827,7 +827,6 @@ ieee80211_deliver_data(struct ieee80211c
!(ic-ic_flags IEEE80211_F_NOBRIDGE)
eh-ether_type != htons(ETHERTYPE_PAE)) {
struct ieee80211_node *ni1;
- int error, len;
if (ETHER_IS_MULTICAST(eh-ether_dhost)) {
m1 = m_copym2(m, 0, M_COPYALL, M_DONTWAIT);
@@ -843,18 +842,8 @@ ieee80211_deliver_data(struct ieee80211c
m = NULL;
}
}
- if (m1 != NULL) {
- len = m1-m_pkthdr.len;
- IFQ_ENQUEUE(ifp-if_snd, m1, NULL, error);
- if (error)
- ifp-if_oerrors++;
- else {
- if (m != NULL)
- ifp-if_omcasts++;
- ifp-if_obytes += len;
- if_start(ifp);
- }
- }
+ if (m1 != NULL)
+ if_output(ifp, m1);
}
#endif
if (m != NULL) {
Index: net80211/ieee80211_output.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_output.c,v
retrieving revision 1.94
diff -u -p -r1.94 ieee80211_output.c
--- net80211/ieee80211_output.c 14 Mar
Pf bikeshedding diff #2: cut down on those ifs
Like really!
OK?
Sanity checked by blambert.
diff --git sys/net/pf.c sys/net/pf.c
index d4cb67c..2ba04d5 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -4488,21 +4488,16 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state
**state,
* Search for an ICMP state.
*/
ret = pf_icmp_state_lookup(pd, key, state,
virtual_id, virtual_type, icmp_dir, iidx,
0, 0);
- if (ret = 0) {
- if (ret == PF_DROP pd-af == AF_INET6
- icmp_dir == PF_OUT) {
- ret = pf_icmp_state_lookup(pd, key, state,
- virtual_id, virtual_type, icmp_dir, iidx,
- 1, 0);
- if (ret = 0)
- return (ret);
- } else
- return (ret);
- }
+ /* IPv6? try matching a multicast address */
+ if (ret == PF_DROP pd-af == AF_INET6 icmp_dir == PF_OUT)
+ ret = pf_icmp_state_lookup(pd, key, state, virtual_id,
+ virtual_type, icmp_dir, iidx, 1, 0);
+ if (ret = 0)
+ return (ret);
(*state)-expire = time_uptime;
(*state)-timeout = PFTM_ICMP_ERROR_REPLY;
/* translate source/destination address, if necessary */
@@ -5101,21 +5096,18 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state
**state,
pd2.hdr.icmp6 = iih;
pf_icmp_mapping(pd2, iih.icmp6_type,
icmp_dir, virtual_id, virtual_type);
ret = pf_icmp_state_lookup(pd2, key, state,
virtual_id, virtual_type, icmp_dir, iidx, 0, 1);
- if (ret = 0) {
- if (ret == PF_DROP pd2.af == AF_INET6
- icmp_dir == PF_OUT) {
- ret = pf_icmp_state_lookup(pd2, key,
- state, virtual_id, virtual_type,
- icmp_dir, iidx, 1, 1);
- if (ret = 0)
- return (ret);
- } else
- return (ret);
- }
+ /* IPv6? try matching a multicast address */
+ if (ret == PF_DROP pd2.af == AF_INET6
+ icmp_dir == PF_OUT)
+ ret = pf_icmp_state_lookup(pd2, key, state,
+ virtual_id, virtual_type, icmp_dir, iidx,
+ 1, 1);
+ if (ret = 0)
+ return (ret);
/* translate source/destination address, if necessary */
if ((*state)-key[PF_SK_WIRE] !=
(*state)-key[PF_SK_STACK]) {
struct pf_state_key *nk;
Pf bikeshedding diff #1: PF_ICMP_MULTI_LINK is just a flag
While looking into Alexandr's report I made a few nits that I consider
worth getting in. This is the first one.
multi is just a flag these days (for better or worse), so having a enum
and a store in pf_icmp_mapping is pointless since the usage is always
the same: try looking up an ICMP state using real addresses and then if
that fails and we happen to be looking up an ICMPv6 state, try multicast.
No need for fancy names here. It used to be a triple value toggle, but
that was changed a few years ago.
OK?
diff --git sys/net/pf.c sys/net/pf.c
index 5bd5864..d4cb67c 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -147,11 +147,11 @@ void pf_change_ap(struct pf_pdesc
*, struct pf_addr *,
int pf_modulate_sack(struct pf_pdesc *,
struct pf_state_peer *);
voidpf_change_a6(struct pf_pdesc *, struct pf_addr *a,
struct pf_addr *an);
int pf_icmp_mapping(struct pf_pdesc *, u_int8_t, int *,
- int *, u_int16_t *, u_int16_t *);
+ u_int16_t *, u_int16_t *);
voidpf_change_icmp(struct pf_pdesc *, struct pf_addr *,
u_int16_t *, struct pf_addr *, struct pf_addr *,
u_int16_t, sa_family_t);
int pf_change_icmp_af(struct mbuf *, int,
struct pf_pdesc *, struct pf_pdesc *,
@@ -242,13 +242,10 @@ struct pf_pool_limit pf_pool_limits[PF_LIMIT_MAX] = {
{ pf_frent_pl, PFFRAG_FRENT_HIWAT, PFFRAG_FRENT_HIWAT },
{ pfr_ktable_pl, PFR_KTABLE_HIWAT, PFR_KTABLE_HIWAT },
{ pfr_kentry_pl, PFR_KENTRY_HIWAT, PFR_KENTRY_HIWAT }
};
-enum { PF_ICMP_MULTI_NONE, PF_ICMP_MULTI_LINK };
-
-
#define STATE_LOOKUP(i, k, d, s, m)\
do {\
s = pf_find_state(i, k, d, m); \
if (s == NULL || (s)-timeout == PFTM_PURGE)\
return (PF_DROP); \
@@ -817,11 +814,11 @@ pf_state_key_addr_setup(struct pf_pdesc *pd, void *arg,
int sidx,
key-addr[didx].addr32[3] = 0;
daddr = NULL; /* overwritten */
}
break;
default:
- if (multi == PF_ICMP_MULTI_LINK) {
+ if (multi) {
key-addr[sidx].addr32[0] = __IPV6_ADDR_INT32_MLL;
key-addr[sidx].addr32[1] = 0;
key-addr[sidx].addr32[2] = 0;
key-addr[sidx].addr32[3] = __IPV6_ADDR_INT32_ONE;
saddr = NULL; /* overwritten */
@@ -1687,20 +1684,19 @@ pf_change_a6(struct pf_pdesc *pd, struct pf_addr *a,
struct pf_addr *an)
PF_ACPY(a, an, AF_INET6);
}
#endif /* INET6 */
int
-pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, int *icmp_dir, int *multi,
+pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, int *icmp_dir,
u_int16_t *virtual_id, u_int16_t *virtual_type)
{
/*
* ICMP types marked with PF_OUT are typically responses to
* PF_IN, and will match states in the opposite direction.
* PF_IN ICMP types need to match a state with that type.
*/
*icmp_dir = PF_OUT;
- *multi = PF_ICMP_MULTI_LINK;
/* Queries (and responses) */
switch (pd-af) {
case AF_INET:
switch (type) {
@@ -3079,11 +3075,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
u_short reason;
int rewrite = 0;
int tag = -1;
int asd = 0;
int match = 0;
- int state_icmp = 0, icmp_dir, multi;
+ int state_icmp = 0, icmp_dir;
u_int16_tvirtual_type, virtual_id;
u_int8_t icmptype = 0, icmpcode = 0;
bzero(act, sizeof(act));
bzero(sns, sizeof(sns));
@@ -3098,11 +3094,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
switch (pd-virtual_proto) {
case IPPROTO_ICMP:
icmptype = pd-hdr.icmp-icmp_type;
icmpcode = pd-hdr.icmp-icmp_code;
state_icmp = pf_icmp_mapping(pd, icmptype,
- icmp_dir, multi, virtual_id, virtual_type);
+ icmp_dir, virtual_id, virtual_type);
if (icmp_dir == PF_IN) {
pd-osport = pd-nsport = virtual_id;
pd-odport = pd-ndport = virtual_type;
} else {
pd-osport = pd-nsport = virtual_type;
@@ -3112,11 +3108,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
iwm(4) fix
The Tx descriptors have 20 scatter-gather slots, but two of those
are used to transfer the command, so only 18 remain for the actual
packet data. So limit the number of dma segments to IWM_NUM_OF_TBS - 2.
We use the same approach in iwn(4), except that there only one slot is
used for the command, so it uses IWN_MAX_SCATTER - 1.
I suspect that this bug is responsible for the
iwm0: hardware error, stopping device
errors I sometimes see when we listing long directories when ssh'ed in
over iwm(4). We all know ssh loves creating long fragmented mbuf
lists.
ok?
Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.40
diff -u -p -r1.40 if_iwm.c
--- if_iwm.c21 May 2015 22:13:55 - 1.40
+++ if_iwm.c21 May 2015 22:31:46 -
@@ -1113,7 +1113,8 @@ iwm_alloc_tx_ring(struct iwm_softc *sc,
paddr += sizeof(struct iwm_device_cmd);
error = bus_dmamap_create(sc-sc_dmat, MCLBYTES,
- IWM_NUM_OF_TBS, MCLBYTES, 0, BUS_DMA_NOWAIT, data-map);
+ IWM_NUM_OF_TBS - 2, MCLBYTES, 0, BUS_DMA_NOWAIT,
+ data-map);
if (error != 0) {
printf(%s: could not create TX buf DMA map\n,
DEVNAME(sc));
goto fail;
Re: ftp(1) rewrite
Sunil Nimmagadda wrote: Hi, The idea is to start with the subset of ftp(1) functionality needed by pkg_add(1): ftp [-o output] url ... i.e., should be able to download files over HTTP(S) and FTP. This implementation works as FETCH_CMD for pkg_add(1) over HTTP(S). FTP is not yet done, but thinking of implementing just PASV and RETR commands and drop command interpreter compeletely. The SMALL variant drops HTTPS and FTP support. Comments? screw ftp. just make a new util http, that just does http.
Re: mismatch for ICMP state created by inound response
Hello, Well, not entirely (: I did it while exploring the code and sent out to provoke further discussion. Today I've talked to reyk@ and we think that it's better to go down a different road: make sure we don't create states on reply packets in the first place. that's actually very wise approach as replies can be spoofed... I've tested this with ICMP, ICMPv6 and NAT64 (slightly). Any OKs? Objections? I have no objections, just a small wish, can you set icmp_dir to -1, if we are not dealing with ICMP? there is a tool we use in Solaris, which yells on us because of uninitialized variable. I know it's false positive, but I've gave up on explaining... patch below combines your fix with my 'wish'. thanks a lot regards sasha Index: pf.c === RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.913 diff -u -r1.913 pf.c --- pf.c11 May 2015 12:22:14 - 1.913 +++ pf.c21 May 2015 18:59:29 - @@ -3124,6 +3124,8 @@ } break; #endif /* INET6 */ + default: + icmp_dir = -1; } ruleset = pf_main_ruleset; @@ -3206,6 +3208,11 @@ TAILQ_NEXT(r, entries)); /* icmp only. type always 0 in other cases */ PF_TEST_ATTRIB((r-code r-code != icmpcode + 1), TAILQ_NEXT(r, entries)); + /* icmp only. don't create states on replies */ + PF_TEST_ATTRIB((r-keep_state !state_icmp + (r-rule_flag PFRULE_STATESLOPPY) == 0 + icmp_dir != PF_IN), TAILQ_NEXT(r, entries)); break;
Re: pf_create_state() is sometimes better to use pf_unlink_state()
Hello,
On Thu, May 21, 2015 at 07:43:51PM +0200, Mike Belopuhov wrote:
On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
Hello,
Hi,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error
nontrivial */
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the cleanup on error nontrivial. Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
perhaps swapping the for loop block with pf_state_insert() will work.
We can then bail out using goto csfailed then (see patch below...)
we should do a pf_state_insert afterwards? This might simplify
locking later on.
speaking of locking... everything is more complicated, however
in this particular case it makes things easier for sure. basically in
our current SMP model, once we insert state to table, the only way
to remove it is to leave the job on garbage collector thread...
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
We would definitely be interested in such a diff, but at the moment
there's no use for it as pf_test is called directly from the IP stack
and hence IP stack needs to support parallel execution first. This
doesn't mean that we can't start integrating bits and pieces, esp.
if they're presented as separate patches.
we use a compile time switch to enable SMP code (-D_PF_SMP_). this is
something I'd like to keep around anyway. I'll start on syncing up our changes
with CURRENT. It will take couple of weeks (ENOCYCLES as usually). Once
I'll have something to show I'll share it and you'll see what can be done
with it.
Thanks for your quality diffs btw, help is always much appreciated.
I'm just trying to be useful. It's pleasure to work with PF sources.
thanks and
regards
sasha
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 19:18:04 -
@@ -3556,15 +3556,6 @@
goto csfailed;
}
- 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;
- REASON_SET(reason, PFRES_STATEINS);
- goto csfailed;
- } else
- *sm = s;
-
/* attach src nodes late, otherwise cleanup on error nontrivial */
for (i = 0; i PF_SN_MAX; i++)
if (sns[i] != NULL) {
@@ -3572,16 +3563,21 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
- REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
- return (PF_DROP);
+ goto csfailed;
}
sni-sn = sns[i];
ftp(1) rewrite
Hi,
The idea is to start with the subset of ftp(1) functionality needed
by pkg_add(1):
ftp [-o output] url ...
i.e., should be able to download files over HTTP(S) and FTP.
This implementation works as FETCH_CMD for pkg_add(1) over HTTP(S).
FTP is not yet done, but thinking of implementing just PASV and
RETR commands and drop command interpreter compeletely.
The SMALL variant drops HTTPS and FTP support.
Comments?
Note: A lot of functionality is still missing including proxy,
redirects, progressmeter, UerAgent, Ranges, etc.
Index: Makefile
===
RCS file: Makefile
diff -N Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ Makefile21 May 2015 18:23:37 -
@@ -0,0 +1,19 @@
+# Define SMALL to disable https and ftp support
+.if defined(SMALL)
+CFLAGS+= -DSMALL
+.endif
+
+PROG= ftp
+NOMAN=
+DEBUG= -g -Wall -O0
+SRCS= main.c http.c
+LDADD+=-lutil
+DPADD+=${LIBUTIL}
+
+.ifndef SMALL
+SRCS+= https.c ftp.c
+LDADD+=-ltls -lssl -lcrypto
+DPADD+=${LIBTLS} ${LIBSSL} ${LIBCRYPTO}
+.endif
+
+.include bsd.prog.mk
Index: ftp.c
===
RCS file: ftp.c
diff -N ftp.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ ftp.c 21 May 2015 18:23:37 -
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2015 Sunil Nimmagadda su...@nimmagadda.net
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include sys/types.h
+
+#include ftp.h
+
+intftp_connect(const char *, const char *);
+intftp_get(int, const char *, const char *, int);
+
+struct proto proto_ftp = {
+ ftp_connect,
+ ftp_get
+};
+
+int
+ftp_connect(const char *host, const char *port)
+{
+ return (-1);
+}
+
+int
+ftp_get(int s, const char *resource, const char *fn, int flags)
+{
+ return (-1);
+}
Index: ftp.h
===
RCS file: ftp.h
diff -N ftp.h
--- /dev/null 1 Jan 1970 00:00:00 -
+++ ftp.h 21 May 2015 18:23:37 -
@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) 2015 Sunil Nimmagadda su...@nimmagadda.net
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#define BUF_SIZ1024
+#defineTMPBUF_LEN 131072
+#define MAX_RETRIES10
+
+struct http_headers {
+ charlocation[BUF_SIZ]; /* Redirect Location */
+ off_t c_len; /* Content-Length */
+};
+
+struct proto {
+ int (*connect)(const char *, const char *);
+ int (*get)(int, const char *, const char *, int);
+};
+
+/* main.c */
+inthost_extract(char *, char *, size_t);
+intheader_insert(struct http_headers *, const char *);
+
+/* http.c */
+inthttp_connect(const char *, const char *);
+inthttp_response_code(char *);
+
Index: http.c
===
RCS file: http.c
diff -N http.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ http.c 21 May 2015 18:23:37 -
@@ -0,0 +1,231 @@
+/*
+ * Copyright (c) 2015 Sunil Nimmagadda su...@nimmagadda.net
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL,
pf.conf from/to negation homogeneous behavior
Dear Tech,
I propose
Index: pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.648
diff -u -p -r1.648 parse.y
--- pfctl/parse.y 21 Apr 2015 16:34:59 - 1.648
+++ pfctl/parse.y 21 May 2015 15:21:54 -
@@ -2563,7 +2563,7 @@ optnl : '\n' optnl
ipspec : ANY { $$ = NULL; }
| xhost { $$ = $1; }
- | '{' optnl host_list '}' { $$ = $3; }
+ | not '{' optnl host_list '}' { $$ = $4; $$-not = $1; }
I tested it on i386 current with a small ruleset ! table and ! {} got now
same behavior,
i can see the ping in pflog0 only if there not to the destination in dns:
# $OpenBSD: pf.conf,v 1.54 2014/08/23 05:49:42 deraadt Exp $
#
# See pf.conf(5) and /etc/examples/pf.conf
set skip on lo
block return# block stateless traffic
table dns { 8.8.8.8, 8.8.4.4 }
match log on vic0 proto icmp from any to !{ 8.8.8.8, 8.8.4.4 }
#match log on vic0 proto icmp from any to ! dns
#match log on vic0 proto icmp from any to dns
#match log on vic0 proto icmp from any to { 8.8.8.8, 8.8.4.4 }
pass# establish keep-state
# By default, do not permit remote connections to X11
block return in on ! lo0 proto tcp to port 6000:6010
Thank you for reading.
--
-
() ascii ribbon campaign - against html e-mail
/\
pf_create_state() is sometimes better to use pf_unlink_state()
Hello,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error nontrivial
*/
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
regards
sasha
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 15:20:01 -
@@ -3573,9 +3573,7 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
+ pf_unlink_state(s);
return (PF_DROP);
}
sni-sn = sns[i];
Re: mismatch for ICMP state created by inound response
On Thu, May 21, 2015 at 11:07 +0200, Alexandr Nedvedicky wrote: Hello, On Tue, May 19, 2015 at 14:07 +0200, Alexandr Nedvedicky wrote: Hello Mike, I've reworked patch from yesterday. I've done some quick testing to see if it fixes problem. It looks like it works. I have not tested NAT-64 yet. Also I'd like to come up with test case, which will show the state check is still able to block invalid ICMP packet (invalid with respect to state). The idea of fix is to keep icmp_dir in state as well. The icmp_dir indicates whether state got created by ICMP request or response. This is useful later in pf_icmp_state_lookup() to check whether ICMP request/response matches state direction. This feels slightly convoluted... check my diff out! (: nice, I like your XOR Magic! comment. (: Looks like I was trying to fix the other end... And you were not wrong. your patch is minimalistic and correct as far as I can tell. Well, not entirely (: I did it while exploring the code and sent out to provoke further discussion. Today I've talked to reyk@ and we think that it's better to go down a different road: make sure we don't create states on reply packets in the first place. Please consider the following: unless sloppy state tracking is used, TCP states can only be set up by the first SYN packet (ok, ok, it's because of flags S/SA, but that is largely irrelevant nowadays since S/SA is now the default and nobody is doing flags any). ICMP was made to adhere to a stricter set of rules and recently I've added sloppy handling there so perhaps we should just prevent state creation for replies (icmp_dir == PF_OUT)? If that is not what sysadmin wants he can always specify keep state (sloopy) and move on with his/her life. I've tested this with ICMP, ICMPv6 and NAT64 (slightly). Any OKs? Objections? diff --git sys/net/pf.c sys/net/pf.c index 39d5cb6..dbc8707 100644 --- sys/net/pf.c +++ sys/net/pf.c @@ -3201,10 +3201,15 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm, PF_TEST_ATTRIB((r-type r-type != icmptype + 1), TAILQ_NEXT(r, entries)); /* icmp only. type always 0 in other cases */ PF_TEST_ATTRIB((r-code r-code != icmpcode + 1), TAILQ_NEXT(r, entries)); + /* icmp only. don't create states on replies */ + PF_TEST_ATTRIB((r-keep_state !state_icmp + (r-rule_flag PFRULE_STATESLOPPY) == 0 + icmp_dir != PF_IN), + TAILQ_NEXT(r, entries)); break; default: break; }
