Hi,
On Tue, 14 Dec 2021 01:20:49 +0100
Alexander Bluhm <alexander.bl...@gmx.net> wrote:
> I don't know much about l2tp, pipex or npppd. So I cannot say if
> the new logic is correct. But I guess you have tested that.
Yes, I've tested some L2TP/IPsec cases already.
> The tdb mutex and ref counting looks correct.
>
>> + struct tdb *tdb, *tdblocal = NULL;
>
> The variable names tdb and tdbp are used very inconsistently within
> IPsec. Don't use both. I think tdpb and a tdbflow are sufficient.
Ok,
>
>> + if (ipsecflowinfo != 0)
>> + ids = ipsp_ids_lookup(ipsecflowinfo);
>
> Can you move that to the place where it is needed?
Yes,
> Perhaps it is easier to understand this way:
>
> if (ipsecflowinfo != 0) {
Sure. Let me update the diff.
> It is hard to say whether the new
> rn_lookup(tdbp->tdb_filter/tdbp->tdb_filtermask) changes existing
> IPsec behavior for setups without l2tp.
I suppose it has no regression on other setups.
But I'll look it more carefully and test the other setups.
> Do we need it there?
Yes, if there is another better idea, it will be welcome.
For this moment, the diff is the best idea for me.
> I never ran into problems patching the correct policy.
Index: sys/netinet/ip_ipsp.c
===================================================================
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.264
diff -u -p -r1.264 ip_ipsp.c
--- sys/netinet/ip_ipsp.c 11 Dec 2021 16:33:47 -0000 1.264
+++ sys/netinet/ip_ipsp.c 14 Dec 2021 06:32:07 -0000
@@ -91,6 +91,8 @@ void tdb_soft_timeout(void *);
void tdb_soft_firstuse(void *);
int tdb_hash(u_int32_t, union sockaddr_union *, u_int8_t);
void tdb_dodelete(struct tdb *, int locked);
+int sockaddr_encap_match(struct sockaddr_encap *,
+ struct sockaddr_encap *, struct sockaddr_encap *);
int ipsec_in_use = 0;
u_int64_t ipsec_last_added = 0;
@@ -510,6 +512,78 @@ gettdbbysrc(u_int rdomain, union sockadd
tdb_ref(tdbp);
mtx_leave(&tdb_sadb_mtx);
return tdbp;
+}
+
+/*
+ * Get an SA given the flow, the direction, the security protocol type, and
+ * the desired IDs.
+ */
+struct tdb *
+gettdbbyflow(u_int rdomain, int direction, struct sockaddr_encap *senflow,
+ u_int8_t sproto, struct ipsec_ids *ids)
+{
+ u_int32_t hashval;
+ struct tdb *tdbp;
+ union sockaddr_union srcdst;
+
+ if (ids == NULL) /* ids is mandatory */
+ return NULL;
+
+ memset(&srcdst, 0, sizeof(srcdst));
+ switch (senflow->sen_type) {
+ case SENT_IP4:
+ srcdst.sin.sin_len = sizeof(srcdst.sin);
+ srcdst.sin.sin_family = AF_INET;
+ if (direction == IPSP_DIRECTION_OUT)
+ srcdst.sin.sin_addr = senflow->Sen.Sip4.Dst;
+ else
+ srcdst.sin.sin_addr = senflow->Sen.Sip4.Src;
+ break;
+ case SENT_IP6:
+ srcdst.sin6.sin6_len = sizeof(srcdst.sin6);
+ srcdst.sin6.sin6_family = AF_INET6;
+ if (direction == IPSP_DIRECTION_OUT)
+ srcdst.sin6.sin6_addr = senflow->Sen.Sip6.Dst;
+ else
+ srcdst.sin6.sin6_addr = senflow->Sen.Sip6.Src;
+ break;
+ }
+
+ mtx_enter(&tdb_sadb_mtx);
+ hashval = tdb_hash(0, &srcdst, sproto);
+
+ for (tdbp = tdbdst[hashval]; tdbp != NULL; tdbp = tdbp->tdb_dnext)
+ if (tdbp->tdb_sproto == sproto &&
+ tdbp->tdb_rdomain == rdomain &&
+ (tdbp->tdb_flags & TDBF_INVALID) == 0 &&
+ ipsp_ids_match(ids, tdbp->tdb_ids) &&
+ ((direction == IPSP_DIRECTION_OUT &&
+ !memcmp(&tdbp->tdb_dst, &srcdst, srcdst.sa.sa_len)) ||
+ (direction == IPSP_DIRECTION_IN &&
+ !memcmp(&tdbp->tdb_src, &srcdst, srcdst.sa.sa_len)))) {
+ if (sockaddr_encap_match(&tdbp->tdb_filter,
+ &tdbp->tdb_filtermask, senflow))
+ break;
+ }
+
+ tdb_ref(tdbp);
+ mtx_leave(&tdb_sadb_mtx);
+ return tdbp;
+}
+
+int
+sockaddr_encap_match(struct sockaddr_encap *addr, struct sockaddr_encap *mask,
+ struct sockaddr_encap *dest)
+{
+ size_t off;
+
+ for (off = offsetof(struct sockaddr_encap, sen_type);
+ off < dest->sen_len; off++) {
+ if ((*((u_char *)addr + off) & *((u_char *)mask + off)) !=
+ (*((u_char *)dest + off) & *((u_char *)mask + off)))
+ break;
+ }
+ return (off < dest->sen_len)? 0 : 1;
}
#ifdef DDB
Index: sys/netinet/ip_ipsp.h
===================================================================
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.230
diff -u -p -r1.230 ip_ipsp.h
--- sys/netinet/ip_ipsp.h 11 Dec 2021 16:33:47 -0000 1.230
+++ sys/netinet/ip_ipsp.h 14 Dec 2021 06:32:07 -0000
@@ -574,6 +574,8 @@ struct tdb *gettdbbysrcdst_dir(u_int, u_
union sockaddr_union *, u_int8_t, int);
#define gettdbbysrcdst(a,b,c,d,e) gettdbbysrcdst_dir((a),(b),(c),(d),(e),0)
#define gettdbbysrcdst_rev(a,b,c,d,e) gettdbbysrcdst_dir((a),(b),(c),(d),(e),1)
+struct tdb *gettdbbyflow(u_int, int, struct sockaddr_encap *, u_int8_t,
+ struct ipsec_ids *);
void puttdb(struct tdb *);
void puttdb_locked(struct tdb *);
void tdb_delete(struct tdb *);
Index: sys/netinet/ip_spd.c
===================================================================
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_spd.c,v
retrieving revision 1.108
diff -u -p -r1.108 ip_spd.c
--- sys/netinet/ip_spd.c 3 Dec 2021 17:18:34 -0000 1.108
+++ sys/netinet/ip_spd.c 14 Dec 2021 06:32:07 -0000
@@ -150,13 +150,14 @@ ipsp_spd_lookup(struct mbuf *m, int af,
u_int32_t ipsecflowinfo)
{
struct radix_node_head *rnh;
- struct radix_node *rn;
+ struct radix_node *rn = NULL;
union sockaddr_union sdst, ssrc;
struct sockaddr_encap *ddst, dst;
struct ipsec_policy *ipo;
struct ipsec_ids *ids = NULL;
int error, signore = 0, dignore = 0;
u_int rdomain = rtable_l2(m->m_pkthdr.ph_rtableid);
+ struct tdb *tdbflow = NULL;
NET_ASSERT_LOCKED();
@@ -297,9 +298,33 @@ ipsp_spd_lookup(struct mbuf *m, int af,
return EAFNOSUPPORT;
}
+ /*
+ * Prepare tdb for searching the correct SPD by rn_lookup().
+ * "tdb_filtemask" of the tdb is used to find the correct SPD when
+ * multiple policies are overlapped.
+ */
+ if (ipsecflowinfo != 0) {
+ ids = ipsp_ids_lookup(ipsecflowinfo);
+ if (ids != NULL)
+ tdbflow = gettdbbyflow(rdomain, direction, &dst,
+ IPPROTO_ESP, ids);
+ }
+
/* Actual SPD lookup. */
- if ((rnh = spd_table_get(rdomain)) == NULL ||
- (rn = rn_match((caddr_t)&dst, rnh)) == NULL) {
+ rnh = spd_table_get(rdomain);
+ if (rnh != NULL) {
+ if (tdbflow != NULL)
+ rn = rn_lookup((caddr_t)&tdbflow->tdb_filter,
+ (caddr_t)&tdbflow->tdb_filtermask, rnh);
+ else if (tdbp != NULL)
+ rn = rn_lookup((caddr_t)&tdbp->tdb_filter,
+ (caddr_t)&tdbp->tdb_filtermask, rnh);
+ else
+ rn = rn_match((caddr_t)&dst, rnh);
+ }
+ tdb_unref(tdbflow);
+
+ if (rn == NULL) {
/*
* Return whatever the socket requirements are, there are no
* system-wide policies.
@@ -389,9 +414,6 @@ ipsp_spd_lookup(struct mbuf *m, int af,
}
}
- if (ipsecflowinfo)
- ids = ipsp_ids_lookup(ipsecflowinfo);
-
/* Check that the cached TDB (if present), is appropriate. */
if (ipo->ipo_tdb != NULL) {
if ((ipo->ipo_last_searched <= ipsec_last_added) ||
@@ -499,10 +521,19 @@ ipsp_spd_lookup(struct mbuf *m, int af,
goto nomatchin;
/* Match source/dest IDs. */
- if (ipo->ipo_ids)
- if (tdbp->tdb_ids == NULL ||
- !ipsp_ids_match(ipo->ipo_ids,
tdbp->tdb_ids))
+ if (ipo->ipo_ids != NULL) {
+ if ((tdbp->tdb_flags & TDBF_TUNNELING) == 0 &&
+ (tdbp->tdb_flags & TDBF_UDPENCAP) != 0) {
+ /*
+ * Skip IDs check for transport mode
+ * with NAT-T. Multiple clients (IDs)
+ * can use a same policy.
+ */
+ } else if (tdbp->tdb_ids == NULL &&
+ !ipsp_ids_match(ipo->ipo_ids,
+ tdbp->tdb_ids))
goto nomatchin;
+ }
/* Add it to the cache. */
if (ipo->ipo_tdb != NULL) {
Thanks,
