Fix ipsp_spd_lookup() for transport mode (was Re: Fix IPsec NAT-T for L2TP/IPsec)
Hi,
On Wed, 12 May 2021 19:11:09 +0900 (JST)
YASUOKA Masahiko wrote:
> Radek reported a problem to misc@ that multiple Windows clients behind
> a NAT cannot use a L2TP/IPsec server simultaneously.
>
> https://marc.info/?t=16099681611=1=2
>
> There is two problems. First is pipex(4) doesn't pass the proper
> ipsecflowinfo to ip_output(). Second is the IPsec policy check which
> is done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is
> not cached. This happens when its flow is shared by another tdb (for
> another client of the same NAT).
This problem is not fixed yet. The diff for the second problem was
not committed in. It was to fix the check in ipsp_spd_lookup() by
making a IPsec policy have a list of IDs.
Also my colleague Kawai pointed out there is another problem if there
is a Linux client among with Windows clients behind a NAT. Windows
uses 1701/udp for its local ID, but the Linux uses ANY/udp for its
local ID.
In the situation, policies will be overlapped.
(a) Windows: REMOTE_IP:1701/udp <=> LOCAL_IP:1701/udp
(b) Linux:REMOTE_IP:ANY/udp <=> LOCAL_IP:1701/udp
Since we use a radix tree for the policies, when rn_match() is used to
find a policy, as it's best match, (b) is never selected.
Let me update the diff.
As for the incomming, we know the tdb when is used. The diff uses the
tdb to find the proper policy.
As for the outgoing, other than using "ipsecflowinfo" there is no way
to select a proper policy. So only when "ipsecflowinfo" is used, get
a tdb from the packet flow and the IDs (retributed by the
ipsecflowinfo), then we can find the proper policy by the tdb.
Also the diff skips the IDs check against the policy only if it is
transport mode and using NAT-T. Since when NAT-T is used for a policy
for transport mode is shared by multiple clients which has a different
IDs, checking the IDs is difficult and I think the checks other than
is enough.
ok? comments?
Fix some problems when accepting IPsec transport mode connections from
multiple clients behind a NAT. In the situation, policies can be
overlapped, but previous could not choice a proper policy both for
incoming and outgoing. To solve this problem, use
tdb->tdb_filter{,mask} to find a proper policy for incoming and find the
tdb by the given ipsecflowinfo and use it for outgoing. Also skip
checking IDs of the policy since a policy is shared by multiple clients
in the situation.
Index: sys/netinet/ip_ipsp.c
===
RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.251
diff -u -p -r1.251 ip_ipsp.c
--- sys/netinet/ip_ipsp.c 18 Nov 2021 11:04:10 - 1.251
+++ sys/netinet/ip_ipsp.c 20 Nov 2021 12:42:36 -
@@ -91,6 +91,8 @@ void tdb_firstuse(void *);
void tdb_soft_timeout(void *);
void tdb_soft_firstuse(void *);
inttdb_hash(u_int32_t, union sockaddr_union *, u_int8_t);
+intsockaddr_encap_match(struct sockaddr_encap *,
+ struct sockaddr_encap *, struct sockaddr_encap *);
int ipsec_in_use = 0;
u_int64_t ipsec_last_added = 0;
@@ -501,6 +503,76 @@ gettdbbysrc(u_int rdomain, union sockadd
mtx_leave(_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(, 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(_sadb_mtx);
+ hashval = tdb_hash(0, , 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 &&
+ ((direction == IPSP_DIRECTION_OUT &&
+ !memcmp(>tdb_dst, , srcdst.sa.sa_len)) ||
+ (direction ==
Re: Fix IPsec NAT-T for L2TP/IPsec
On Thu, May 13, 2021 at 09:20:22AM +0900, YASUOKA Masahiko wrote:
> On Wed, 12 May 2021 19:11:09 +0900 (JST)
> YASUOKA Masahiko wrote:
> > Radek reported a problem to misc@ that multiple Windows clients behind
> > a NAT cannot use a L2TP/IPsec server simultaneously.
> >
> > https://marc.info/?t=16099681611=1=2
> >
> > There is two problems. First is pipex(4) doesn't pass the proper
> > ipsecflowinfo to ip_output(). Second is the IPsec policy check which
> > is done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is
> > not cached. This happens when its flow is shared by another tdb (for
> > another client of the same NAT).
> >
> > The following 2 diffs fix these problem.
> >
> > comment?
> > ok?
> >
> > diff #1
> >
> > Fix IPsec NAT-T work with pipex.
>
> The original diff #1 used m_tag to specify the ipsecflowinfo.
>
> I noticed "ph_cookie" is usable instead of the m_tag. It seems simpler.
>
> Is it better?
>
No, I don't like this.
> Index: sys/net/if_etherip.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/if_etherip.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 if_etherip.c
> --- sys/net/if_etherip.c 9 Jan 2021 21:00:58 - 1.48
> +++ sys/net/if_etherip.c 12 May 2021 23:29:41 -
> @@ -547,7 +547,7 @@ ip_etherip_output(struct ifnet *ifp, str
> etheripstat_pkt(etherips_opackets, etherips_obytes, m->m_pkthdr.len -
> (sizeof(struct ip) + sizeof(struct etherip_header)));
>
> - ip_send(m);
> + ip_send(m, 0);
>
> return (0);
> }
> Index: sys/net/if_gif.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/if_gif.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 if_gif.c
> --- sys/net/if_gif.c 20 Feb 2021 04:58:29 - 1.132
> +++ sys/net/if_gif.c 12 May 2021 23:29:45 -
> @@ -340,7 +340,7 @@ gif_send(struct gif_softc *sc, struct mb
> ip->ip_src = sc->sc_tunnel.t_src4;
> ip->ip_dst = sc->sc_tunnel.t_dst4;
>
> - ip_send(m);
> + ip_send(m, 0);
> break;
> }
> #ifdef INET6
> Index: sys/net/if_gre.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/if_gre.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 if_gre.c
> --- sys/net/if_gre.c 10 Mar 2021 10:21:47 - 1.171
> +++ sys/net/if_gre.c 12 May 2021 23:29:52 -
> @@ -1999,7 +1999,7 @@ gre_ip_output(const struct gre_tunnel *t
>
> switch (tunnel->t_af) {
> case AF_INET:
> - ip_send(m);
> + ip_send(m, 0);
> break;
> #ifdef INET6
> case AF_INET6:
> Index: sys/net/pf.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pf.c,v
> retrieving revision 1.1116
> diff -u -p -r1.1116 pf.c
> --- sys/net/pf.c 27 Apr 2021 09:38:29 - 1.1116
> +++ sys/net/pf.c 12 May 2021 23:29:56 -
> @@ -2896,7 +2896,7 @@ pf_send_tcp(const struct pf_rule *r, sa_
>
> switch (af) {
> case AF_INET:
> - ip_send(m);
> + ip_send(m, 0);
> break;
> #ifdef INET6
> case AF_INET6:
> Index: sys/net/pipex.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 pipex.c
> --- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
> +++ sys/net/pipex.c 12 May 2021 23:31:24 -
> @@ -1258,7 +1258,7 @@ pipex_pptp_output(struct mbuf *m0, struc
> gre->flags = htons(gre->flags);
>
> m0->m_pkthdr.ph_ifidx = session->ifindex;
> - ip_send(m0);
> + ip_send(m0, 0);
> if (len > 0) { /* network layer only */
> /* countup statistics */
> session->stat.opackets++;
> @@ -1704,7 +1704,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
> ip->ip_tos = 0;
> ip->ip_off = 0;
>
> - ip_send(m0);
> + ip_send(m0, session->proto.l2tp.ipsecflowinfo);
> break;
> #ifdef INET6
> case AF_INET6:
> Index: sys/netinet/ip_icmp.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.186
> diff -u -p -r1.186 ip_icmp.c
> --- sys/netinet/ip_icmp.c 30 Mar 2021 08:37:10 - 1.186
> +++ sys/netinet/ip_icmp.c 12 May 2021 23:31:57 -
> @@ -860,7 +860,7 @@ icmp_send(struct mbuf *m, struct mbuf *o
> ipstat_inc(ips_localout);
> ip_send_raw(m);
> } else
> - ip_send(m);
> + ip_send(m, 0);
> }
>
> u_int32_t
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
>
Re: Fix IPsec NAT-T for L2TP/IPsec
On Wed, 12 May 2021 19:11:09 +0900 (JST)
YASUOKA Masahiko wrote:
> Radek reported a problem to misc@ that multiple Windows clients behind
> a NAT cannot use a L2TP/IPsec server simultaneously.
>
> https://marc.info/?t=16099681611=1=2
>
> There is two problems. First is pipex(4) doesn't pass the proper
> ipsecflowinfo to ip_output(). Second is the IPsec policy check which
> is done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is
> not cached. This happens when its flow is shared by another tdb (for
> another client of the same NAT).
>
> The following 2 diffs fix these problem.
>
> comment?
> ok?
>
> diff #1
>
> Fix IPsec NAT-T work with pipex.
The original diff #1 used m_tag to specify the ipsecflowinfo.
I noticed "ph_cookie" is usable instead of the m_tag. It seems simpler.
Is it better?
Index: sys/net/if_etherip.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/if_etherip.c,v
retrieving revision 1.48
diff -u -p -r1.48 if_etherip.c
--- sys/net/if_etherip.c9 Jan 2021 21:00:58 - 1.48
+++ sys/net/if_etherip.c12 May 2021 23:29:41 -
@@ -547,7 +547,7 @@ ip_etherip_output(struct ifnet *ifp, str
etheripstat_pkt(etherips_opackets, etherips_obytes, m->m_pkthdr.len -
(sizeof(struct ip) + sizeof(struct etherip_header)));
- ip_send(m);
+ ip_send(m, 0);
return (0);
}
Index: sys/net/if_gif.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/if_gif.c,v
retrieving revision 1.132
diff -u -p -r1.132 if_gif.c
--- sys/net/if_gif.c20 Feb 2021 04:58:29 - 1.132
+++ sys/net/if_gif.c12 May 2021 23:29:45 -
@@ -340,7 +340,7 @@ gif_send(struct gif_softc *sc, struct mb
ip->ip_src = sc->sc_tunnel.t_src4;
ip->ip_dst = sc->sc_tunnel.t_dst4;
- ip_send(m);
+ ip_send(m, 0);
break;
}
#ifdef INET6
Index: sys/net/if_gre.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/if_gre.c,v
retrieving revision 1.171
diff -u -p -r1.171 if_gre.c
--- sys/net/if_gre.c10 Mar 2021 10:21:47 - 1.171
+++ sys/net/if_gre.c12 May 2021 23:29:52 -
@@ -1999,7 +1999,7 @@ gre_ip_output(const struct gre_tunnel *t
switch (tunnel->t_af) {
case AF_INET:
- ip_send(m);
+ ip_send(m, 0);
break;
#ifdef INET6
case AF_INET6:
Index: sys/net/pf.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pf.c,v
retrieving revision 1.1116
diff -u -p -r1.1116 pf.c
--- sys/net/pf.c27 Apr 2021 09:38:29 - 1.1116
+++ sys/net/pf.c12 May 2021 23:29:56 -
@@ -2896,7 +2896,7 @@ pf_send_tcp(const struct pf_rule *r, sa_
switch (af) {
case AF_INET:
- ip_send(m);
+ ip_send(m, 0);
break;
#ifdef INET6
case AF_INET6:
Index: sys/net/pipex.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
retrieving revision 1.132
diff -u -p -r1.132 pipex.c
--- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
+++ sys/net/pipex.c 12 May 2021 23:31:24 -
@@ -1258,7 +1258,7 @@ pipex_pptp_output(struct mbuf *m0, struc
gre->flags = htons(gre->flags);
m0->m_pkthdr.ph_ifidx = session->ifindex;
- ip_send(m0);
+ ip_send(m0, 0);
if (len > 0) { /* network layer only */
/* countup statistics */
session->stat.opackets++;
@@ -1704,7 +1704,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
ip->ip_tos = 0;
ip->ip_off = 0;
- ip_send(m0);
+ ip_send(m0, session->proto.l2tp.ipsecflowinfo);
break;
#ifdef INET6
case AF_INET6:
Index: sys/netinet/ip_icmp.c
===
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.186
diff -u -p -r1.186 ip_icmp.c
--- sys/netinet/ip_icmp.c 30 Mar 2021 08:37:10 - 1.186
+++ sys/netinet/ip_icmp.c 12 May 2021 23:31:57 -
@@ -860,7 +860,7 @@ icmp_send(struct mbuf *m, struct mbuf *o
ipstat_inc(ips_localout);
ip_send_raw(m);
} else
- ip_send(m);
+ ip_send(m, 0);
}
u_int32_t
Index: sys/netinet/ip_input.c
===
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
retrieving revision 1.359
diff -u -p -r1.359 ip_input.c
--- sys/netinet/ip_input.c 30 Apr 2021 13:52:48 - 1.359
+++ sys/netinet/ip_input.c 12 May 2021 23:29:01 -
@@ -1790,6 +1790,7 @@ ip_send_do_dispatch(void *xmq, int flags
Re: Fix IPsec NAT-T for L2TP/IPsec
ok mvs@
> On 13 May 2021, at 02:43, YASUOKA Masahiko wrote:
>
> On Wed, 12 May 2021 19:15:29 +0300
> Vitaliy Makkoveev wrote:
>>> On 12 May 2021, at 18:42, YASUOKA Masahiko wrote:
>>> On Wed, 12 May 2021 17:26:51 +0300
>>> Vitaliy Makkoveev wrote:
On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote:
> Radek reported a problem to misc@ that multiple Windows clients behind a
> NAT
> cannot use a L2TP/IPsec server simultaneously.
>
> https://marc.info/?t=16099681611=1=2
>
> There is two problems. First is pipex(4) doesn't pass the proper
> ipsecflowinfo to ip_output(). Second is the IPsec policy check which is
> done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not
> cached. This happens when its flow is shared by another tdb (for another
> client of the same NAT).
>
> The following 2 diffs fix these problem.
>
> comment?
> ok?
>
Hi.
I have two comments for the diff 1:
1. You should add PACKET_TAG_IPSEC_FLOWINFO description to
m_tag_get(9).
2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I
pointed the place in your diff.
>>>
>>> Good catch. Thanks.
>>>
>>
>> m_freem(9) accepts NULL so this check before is redundant.
>
> Yes,
>
>> It seems to me that "Used by the IPv4 stack to specify the IPsec flow
>> of an output IP packet. The tag contains a u_int32_t identifying the
>> IPsec flow.” is enough. Anyway it’s better to ask jmc@.
>
> Ok,
>
>> Also I like to remove PACKET_TAG_PIPEX with separate diff.
>
> I removed PACKET_TAG_PIPEX separetely.
>
> Let me update the diff.
>
> Index: sys/net/pipex.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 pipex.c
> --- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
> +++ sys/net/pipex.c 12 May 2021 23:18:52 -
> @@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
> #ifdef INET6
> struct ip6_hdr *ip6;
> #endif
> + struct m_tag *mtag;
>
> hlen = sizeof(struct pipex_l2tp_header) +
> ((pipex_session_is_l2tp_data_sequencing_on(session))
> @@ -1704,6 +1705,15 @@ pipex_l2tp_output(struct mbuf *m0, struc
> ip->ip_tos = 0;
> ip->ip_off = 0;
>
> + if (session->proto.l2tp.ipsecflowinfo > 0) {
> + if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO,
> + sizeof(u_int32_t), M_NOWAIT)) == NULL)
> + goto drop;
> + *(u_int32_t *)(mtag + 1) =
> + session->proto.l2tp.ipsecflowinfo;
> + m_tag_prepend(m0, mtag);
> + }
> +
> ip_send(m0);
> break;
> #ifdef INET6
> @@ -1733,6 +1743,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
>
> return;
> drop:
> + m_freem(m0);
> session->stat.oerrors++;
> }
>
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
> retrieving revision 1.359
> diff -u -p -r1.359 ip_input.c
> --- sys/netinet/ip_input.c30 Apr 2021 13:52:48 - 1.359
> +++ sys/netinet/ip_input.c12 May 2021 23:18:52 -
> @@ -1790,6 +1790,8 @@ ip_send_do_dispatch(void *xmq, int flags
> struct mbuf_queue *mq = xmq;
> struct mbuf *m;
> struct mbuf_list ml;
> + struct m_tag *mtag;
> + u_int32_t ipsecflowinfo = 0;
>
> mq_delist(mq, );
> if (ml_empty())
> @@ -1797,7 +1799,12 @@ ip_send_do_dispatch(void *xmq, int flags
>
> NET_LOCK();
> while ((m = ml_dequeue()) != NULL) {
> - ip_output(m, NULL, NULL, flags, NULL, NULL, 0);
> + if ((mtag = m_tag_find(m, PACKET_TAG_IPSEC_FLOWINFO, NULL))
> + != NULL) {
> + ipsecflowinfo = *(u_int32_t *)(mtag + 1);
> + m_tag_delete(m, mtag);
> + }
> + ip_output(m, NULL, NULL, flags, NULL, NULL, ipsecflowinfo);
> }
> NET_UNLOCK();
> }
> Index: sys/sys/mbuf.h
> ===
> RCS file: /disk/cvs/openbsd/src/sys/sys/mbuf.h,v
> retrieving revision 1.252
> diff -u -p -r1.252 mbuf.h
> --- sys/sys/mbuf.h25 Feb 2021 02:43:31 - 1.252
> +++ sys/sys/mbuf.h12 May 2021 23:18:52 -
> @@ -469,6 +469,7 @@ struct m_tag *m_tag_next(struct mbuf *,
> /* Packet tag types */
> #define PACKET_TAG_IPSEC_IN_DONE 0x0001 /* IPsec applied, in */
> #define PACKET_TAG_IPSEC_OUT_DONE 0x0002 /* IPsec applied, out */
> +#define PACKET_TAG_IPSEC_FLOWINFO0x0004 /* IPsec flowinfo */
> #define PACKET_TAG_WIREGUARD 0x0040 /* WireGuard data */
> #define PACKET_TAG_GRE0x0080 /* GRE processing done
> */
Re: Fix IPsec NAT-T for L2TP/IPsec
On Wed, 12 May 2021 19:15:29 +0300
Vitaliy Makkoveev wrote:
>> On 12 May 2021, at 18:42, YASUOKA Masahiko wrote:
>> On Wed, 12 May 2021 17:26:51 +0300
>> Vitaliy Makkoveev wrote:
>>> On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote:
Radek reported a problem to misc@ that multiple Windows clients behind a
NAT
cannot use a L2TP/IPsec server simultaneously.
https://marc.info/?t=16099681611=1=2
There is two problems. First is pipex(4) doesn't pass the proper
ipsecflowinfo to ip_output(). Second is the IPsec policy check which is
done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not
cached. This happens when its flow is shared by another tdb (for another
client of the same NAT).
The following 2 diffs fix these problem.
comment?
ok?
>>>
>>> Hi.
>>>
>>> I have two comments for the diff 1:
>>>
>>> 1. You should add PACKET_TAG_IPSEC_FLOWINFO description to
>>>m_tag_get(9).
>>> 2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I
>>> pointed the place in your diff.
>>
>> Good catch. Thanks.
>>
>
> m_freem(9) accepts NULL so this check before is redundant.
Yes,
> It seems to me that "Used by the IPv4 stack to specify the IPsec flow
> of an output IP packet. The tag contains a u_int32_t identifying the
> IPsec flow.” is enough. Anyway it’s better to ask jmc@.
Ok,
> Also I like to remove PACKET_TAG_PIPEX with separate diff.
I removed PACKET_TAG_PIPEX separetely.
Let me update the diff.
Index: sys/net/pipex.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
retrieving revision 1.132
diff -u -p -r1.132 pipex.c
--- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
+++ sys/net/pipex.c 12 May 2021 23:18:52 -
@@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
#ifdef INET6
struct ip6_hdr *ip6;
#endif
+ struct m_tag *mtag;
hlen = sizeof(struct pipex_l2tp_header) +
((pipex_session_is_l2tp_data_sequencing_on(session))
@@ -1704,6 +1705,15 @@ pipex_l2tp_output(struct mbuf *m0, struc
ip->ip_tos = 0;
ip->ip_off = 0;
+ if (session->proto.l2tp.ipsecflowinfo > 0) {
+ if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO,
+ sizeof(u_int32_t), M_NOWAIT)) == NULL)
+ goto drop;
+ *(u_int32_t *)(mtag + 1) =
+ session->proto.l2tp.ipsecflowinfo;
+ m_tag_prepend(m0, mtag);
+ }
+
ip_send(m0);
break;
#ifdef INET6
@@ -1733,6 +1743,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
return;
drop:
+ m_freem(m0);
session->stat.oerrors++;
}
Index: sys/netinet/ip_input.c
===
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
retrieving revision 1.359
diff -u -p -r1.359 ip_input.c
--- sys/netinet/ip_input.c 30 Apr 2021 13:52:48 - 1.359
+++ sys/netinet/ip_input.c 12 May 2021 23:18:52 -
@@ -1790,6 +1790,8 @@ ip_send_do_dispatch(void *xmq, int flags
struct mbuf_queue *mq = xmq;
struct mbuf *m;
struct mbuf_list ml;
+ struct m_tag *mtag;
+ u_int32_t ipsecflowinfo = 0;
mq_delist(mq, );
if (ml_empty())
@@ -1797,7 +1799,12 @@ ip_send_do_dispatch(void *xmq, int flags
NET_LOCK();
while ((m = ml_dequeue()) != NULL) {
- ip_output(m, NULL, NULL, flags, NULL, NULL, 0);
+ if ((mtag = m_tag_find(m, PACKET_TAG_IPSEC_FLOWINFO, NULL))
+ != NULL) {
+ ipsecflowinfo = *(u_int32_t *)(mtag + 1);
+ m_tag_delete(m, mtag);
+ }
+ ip_output(m, NULL, NULL, flags, NULL, NULL, ipsecflowinfo);
}
NET_UNLOCK();
}
Index: sys/sys/mbuf.h
===
RCS file: /disk/cvs/openbsd/src/sys/sys/mbuf.h,v
retrieving revision 1.252
diff -u -p -r1.252 mbuf.h
--- sys/sys/mbuf.h 25 Feb 2021 02:43:31 - 1.252
+++ sys/sys/mbuf.h 12 May 2021 23:18:52 -
@@ -469,6 +469,7 @@ struct m_tag *m_tag_next(struct mbuf *,
/* Packet tag types */
#define PACKET_TAG_IPSEC_IN_DONE 0x0001 /* IPsec applied, in */
#define PACKET_TAG_IPSEC_OUT_DONE 0x0002 /* IPsec applied, out */
+#define PACKET_TAG_IPSEC_FLOWINFO 0x0004 /* IPsec flowinfo */
#define PACKET_TAG_WIREGUARD 0x0040 /* WireGuard data */
#define PACKET_TAG_GRE 0x0080 /* GRE processing done */
#define PACKET_TAG_DLT 0x0100 /* data link layer type */
@@ -479,7 +480,7 @@ struct m_tag *m_tag_next(struct mbuf *,
#define PACKET_TAG_CARP_BAL_IP 0x4000 /* carp(4) ip
Re: Fix IPsec NAT-T for L2TP/IPsec
> On 12 May 2021, at 18:42, YASUOKA Masahiko wrote:
>
> On Wed, 12 May 2021 17:26:51 +0300
> Vitaliy Makkoveev wrote:
>> On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote:
>>> Radek reported a problem to misc@ that multiple Windows clients behind a NAT
>>> cannot use a L2TP/IPsec server simultaneously.
>>>
>>> https://marc.info/?t=16099681611=1=2
>>>
>>> There is two problems. First is pipex(4) doesn't pass the proper
>>> ipsecflowinfo to ip_output(). Second is the IPsec policy check which is
>>> done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not
>>> cached. This happens when its flow is shared by another tdb (for another
>>> client of the same NAT).
>>>
>>> The following 2 diffs fix these problem.
>>>
>>> comment?
>>> ok?
>>>
>>
>> Hi.
>>
>> I have two comments for the diff 1:
>>
>> 1. You should add PACKET_TAG_IPSEC_FLOWINFO description to
>>m_tag_get(9).
>> 2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I
>> pointed the place in your diff.
>
> Good catch. Thanks.
>
m_freem(9) accepts NULL so this check before is redundant.
It seems to me that "Used by the IPv4 stack to specify the IPsec flow
of an output IP packet. The tag contains a u_int32_t identifying the
IPsec flow.” is enough. Anyway it’s better to ask jmc@.
Also I like to remove PACKET_TAG_PIPEX with separate diff.
The rest of this diff looks ok by me.
>
> Let me update the diff.
>
> Index: sys/net/pipex.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 pipex.c
> --- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
> +++ sys/net/pipex.c 12 May 2021 15:33:33 -
> @@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
> #ifdef INET6
> struct ip6_hdr *ip6;
> #endif
> + struct m_tag *mtag;
>
> hlen = sizeof(struct pipex_l2tp_header) +
> ((pipex_session_is_l2tp_data_sequencing_on(session))
> @@ -1704,6 +1705,15 @@ pipex_l2tp_output(struct mbuf *m0, struc
> ip->ip_tos = 0;
> ip->ip_off = 0;
>
> + if (session->proto.l2tp.ipsecflowinfo > 0) {
> + if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO,
> + sizeof(u_int32_t), M_NOWAIT)) == NULL)
> + goto drop;
> + *(u_int32_t *)(mtag + 1) =
> + session->proto.l2tp.ipsecflowinfo;
> + m_tag_prepend(m0, mtag);
> + }
> +
> ip_send(m0);
> break;
> #ifdef INET6
> @@ -1733,6 +1743,8 @@ pipex_l2tp_output(struct mbuf *m0, struc
>
> return;
> drop:
> + if (m0 != NULL)
> + m_freem(m0);
> session->stat.oerrors++;
> }
>
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
> retrieving revision 1.359
> diff -u -p -r1.359 ip_input.c
> --- sys/netinet/ip_input.c30 Apr 2021 13:52:48 - 1.359
> +++ sys/netinet/ip_input.c12 May 2021 15:31:52 -
> @@ -1790,6 +1790,8 @@ ip_send_do_dispatch(void *xmq, int flags
> struct mbuf_queue *mq = xmq;
> struct mbuf *m;
> struct mbuf_list ml;
> + struct m_tag *mtag;
> + u_int32_t ipsecflowinfo = 0;
>
> mq_delist(mq, );
> if (ml_empty())
> @@ -1797,7 +1799,12 @@ ip_send_do_dispatch(void *xmq, int flags
>
> NET_LOCK();
> while ((m = ml_dequeue()) != NULL) {
> - ip_output(m, NULL, NULL, flags, NULL, NULL, 0);
> + if ((mtag = m_tag_find(m, PACKET_TAG_IPSEC_FLOWINFO, NULL))
> + != NULL) {
> + ipsecflowinfo = *(u_int32_t *)(mtag + 1);
> + m_tag_delete(m, mtag);
> + }
> + ip_output(m, NULL, NULL, flags, NULL, NULL, ipsecflowinfo);
> }
> NET_UNLOCK();
> }
> Index: sys/sys/mbuf.h
> ===
> RCS file: /disk/cvs/openbsd/src/sys/sys/mbuf.h,v
> retrieving revision 1.252
> diff -u -p -r1.252 mbuf.h
> --- sys/sys/mbuf.h25 Feb 2021 02:43:31 - 1.252
> +++ sys/sys/mbuf.h12 May 2021 15:31:52 -
> @@ -469,6 +469,7 @@ struct m_tag *m_tag_next(struct mbuf *,
> /* Packet tag types */
> #define PACKET_TAG_IPSEC_IN_DONE 0x0001 /* IPsec applied, in */
> #define PACKET_TAG_IPSEC_OUT_DONE 0x0002 /* IPsec applied, out */
> +#define PACKET_TAG_IPSEC_FLOWINFO0x0004 /* IPsec flowinfo */
> #define PACKET_TAG_WIREGUARD 0x0040 /* WireGuard data */
> #define PACKET_TAG_GRE0x0080 /* GRE processing done
> */
> #define PACKET_TAG_DLT0x0100 /* data link layer type
> */
> @@ -479,7 +480,7 @@ struct m_tag *m_tag_next(struct mbuf *,
> #define PACKET_TAG_CARP_BAL_IP0x4000 /*
Re: Fix IPsec NAT-T for L2TP/IPsec
On Wed, 12 May 2021 17:26:51 +0300
Vitaliy Makkoveev wrote:
> On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote:
>> Radek reported a problem to misc@ that multiple Windows clients behind a NAT
>> cannot use a L2TP/IPsec server simultaneously.
>>
>> https://marc.info/?t=16099681611=1=2
>>
>> There is two problems. First is pipex(4) doesn't pass the proper
>> ipsecflowinfo to ip_output(). Second is the IPsec policy check which is
>> done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not
>> cached. This happens when its flow is shared by another tdb (for another
>> client of the same NAT).
>>
>> The following 2 diffs fix these problem.
>>
>> comment?
>> ok?
>>
>
> Hi.
>
> I have two comments for the diff 1:
>
> 1. You should add PACKET_TAG_IPSEC_FLOWINFO description to
> m_tag_get(9).
> 2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I
>pointed the place in your diff.
Good catch. Thanks.
Let me update the diff.
Index: sys/net/pipex.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
retrieving revision 1.132
diff -u -p -r1.132 pipex.c
--- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
+++ sys/net/pipex.c 12 May 2021 15:33:33 -
@@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
#ifdef INET6
struct ip6_hdr *ip6;
#endif
+ struct m_tag *mtag;
hlen = sizeof(struct pipex_l2tp_header) +
((pipex_session_is_l2tp_data_sequencing_on(session))
@@ -1704,6 +1705,15 @@ pipex_l2tp_output(struct mbuf *m0, struc
ip->ip_tos = 0;
ip->ip_off = 0;
+ if (session->proto.l2tp.ipsecflowinfo > 0) {
+ if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO,
+ sizeof(u_int32_t), M_NOWAIT)) == NULL)
+ goto drop;
+ *(u_int32_t *)(mtag + 1) =
+ session->proto.l2tp.ipsecflowinfo;
+ m_tag_prepend(m0, mtag);
+ }
+
ip_send(m0);
break;
#ifdef INET6
@@ -1733,6 +1743,8 @@ pipex_l2tp_output(struct mbuf *m0, struc
return;
drop:
+ if (m0 != NULL)
+ m_freem(m0);
session->stat.oerrors++;
}
Index: sys/netinet/ip_input.c
===
RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
retrieving revision 1.359
diff -u -p -r1.359 ip_input.c
--- sys/netinet/ip_input.c 30 Apr 2021 13:52:48 - 1.359
+++ sys/netinet/ip_input.c 12 May 2021 15:31:52 -
@@ -1790,6 +1790,8 @@ ip_send_do_dispatch(void *xmq, int flags
struct mbuf_queue *mq = xmq;
struct mbuf *m;
struct mbuf_list ml;
+ struct m_tag *mtag;
+ u_int32_t ipsecflowinfo = 0;
mq_delist(mq, );
if (ml_empty())
@@ -1797,7 +1799,12 @@ ip_send_do_dispatch(void *xmq, int flags
NET_LOCK();
while ((m = ml_dequeue()) != NULL) {
- ip_output(m, NULL, NULL, flags, NULL, NULL, 0);
+ if ((mtag = m_tag_find(m, PACKET_TAG_IPSEC_FLOWINFO, NULL))
+ != NULL) {
+ ipsecflowinfo = *(u_int32_t *)(mtag + 1);
+ m_tag_delete(m, mtag);
+ }
+ ip_output(m, NULL, NULL, flags, NULL, NULL, ipsecflowinfo);
}
NET_UNLOCK();
}
Index: sys/sys/mbuf.h
===
RCS file: /disk/cvs/openbsd/src/sys/sys/mbuf.h,v
retrieving revision 1.252
diff -u -p -r1.252 mbuf.h
--- sys/sys/mbuf.h 25 Feb 2021 02:43:31 - 1.252
+++ sys/sys/mbuf.h 12 May 2021 15:31:52 -
@@ -469,6 +469,7 @@ struct m_tag *m_tag_next(struct mbuf *,
/* Packet tag types */
#define PACKET_TAG_IPSEC_IN_DONE 0x0001 /* IPsec applied, in */
#define PACKET_TAG_IPSEC_OUT_DONE 0x0002 /* IPsec applied, out */
+#define PACKET_TAG_IPSEC_FLOWINFO 0x0004 /* IPsec flowinfo */
#define PACKET_TAG_WIREGUARD 0x0040 /* WireGuard data */
#define PACKET_TAG_GRE 0x0080 /* GRE processing done */
#define PACKET_TAG_DLT 0x0100 /* data link layer type */
@@ -479,7 +480,7 @@ struct m_tag *m_tag_next(struct mbuf *,
#define PACKET_TAG_CARP_BAL_IP 0x4000 /* carp(4) ip balanced marker */
#define MTAG_BITS \
-("\20\1IPSEC_IN_DONE\2IPSEC_OUT_DONE\3IPSEC_IN_CRYPTO_DONE" \
+("\20\1IPSEC_IN_DONE\2IPSEC_OUT_DONE\3IPSEC_FLOWINFO" \
"\4IPSEC_OUT_CRYPTO_NEEDED\5IPSEC_PENDING_TDB\6BRIDGE\7WG\10GRE\11DLT" \
"\12PF_DIVERT\14PF_REASSEMBLED\15SRCROUTE\16TUNNEL\17CARP_BAL_IP")
Index: share/man/man9/mbuf_tags.9
===
RCS file: /disk/cvs/openbsd/src/share/man/man9/mbuf_tags.9,v
retrieving revision 1.41
diff -u -p -r1.41
Re: Fix IPsec NAT-T for L2TP/IPsec
On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote:
> Hi,
>
> Radek reported a problem to misc@ that multiple Windows clients behind a NAT
> cannot use a L2TP/IPsec server simultaneously.
>
> https://marc.info/?t=16099681611=1=2
>
> There is two problems. First is pipex(4) doesn't pass the proper
> ipsecflowinfo to ip_output(). Second is the IPsec policy check which is
> done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not
> cached. This happens when its flow is shared by another tdb (for another
> client of the same NAT).
>
> The following 2 diffs fix these problem.
>
> comment?
> ok?
>
Hi.
I have two comments for the diff 1:
1. You should add PACKET_TAG_IPSEC_FLOWINFO description to
m_tag_get(9).
2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I
pointed the place in your diff.
I'll see diff 2 later.
> diff #1
>
> Fix IPsec NAT-T work with pipex.
>
> Index: sys/net/pipex.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 pipex.c
> --- sys/net/pipex.c 10 Mar 2021 10:21:48 - 1.132
> +++ sys/net/pipex.c 12 May 2021 09:38:32 -
> @@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
> #ifdef INET6
> struct ip6_hdr *ip6;
> #endif
> + struct m_tag *mtag;
>
> hlen = sizeof(struct pipex_l2tp_header) +
> ((pipex_session_is_l2tp_data_sequencing_on(session))
> @@ -1703,6 +1704,15 @@ pipex_l2tp_output(struct mbuf *m0, struc
> ip->ip_ttl = MAXTTL;
> ip->ip_tos = 0;
> ip->ip_off = 0;
> +
> + if (session->proto.l2tp.ipsecflowinfo > 0) {
> + if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO,
> + sizeof(u_int32_t), M_NOWAIT)) == NULL)
> + goto drop;
mbuf(9) will leak here.
> + *(u_int32_t *)(mtag + 1) =
> + session->proto.l2tp.ipsecflowinfo;
> + m_tag_prepend(m0, mtag);
> + }
>
> ip_send(m0);
> break;
