* [Patch net] xfrm: unify xfrm protocol checks
@ 2019-03-19 5:08 Cong Wang
2019-03-19 5:17 ` Herbert Xu
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2019-03-19 5:08 UTC (permalink / raw)
To: netdev
Cc: Cong Wang, syzbot+0bf0519d6e0de15914fe, Steffen Klassert, Herbert Xu
In commit 6a53b7593233 ("xfrm: check id proto in validate_tmpl()")
I introduced a check for xfrm protocol, but unfortunately
xfrm_id_proto_match() could still miss IPPROTO_ROUTING which causes
entries left in net->xfrm.state_all.
This patch extracts the check from validate_tmpl() to
xfrm_id_proto_valid() and uses it in all other places, including
the missing one in parse_ipsecrequest().
Fixes: 6a53b7593233 ("xfrm: check id proto in validate_tmpl()")
Reported-by: syzbot+0bf0519d6e0de15914fe@syzkaller.appspotmail.com
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
include/net/xfrm.h | 21 ++++++++++++++++++---
net/key/af_key.c | 4 +++-
net/xfrm/xfrm_user.c | 14 +-------------
3 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 85386becbaea..22ef7617087d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1404,12 +1404,27 @@ static inline int xfrm_state_kern(const struct xfrm_state *x)
return atomic_read(&x->tunnel_users);
}
+static inline bool xfrm_id_proto_valid(u8 proto)
+{
+ switch (proto) {
+ case IPPROTO_AH:
+ case IPPROTO_ESP:
+ case IPPROTO_COMP:
+#if IS_ENABLED(CONFIG_IPV6)
+ case IPPROTO_ROUTING:
+ case IPPROTO_DSTOPTS:
+#endif
+ case IPSEC_PROTO_ANY:
+ return true;
+ default:
+ return false;
+ }
+}
+
static inline int xfrm_id_proto_match(u8 proto, u8 userproto)
{
return (!userproto || proto == userproto ||
- (userproto == IPSEC_PROTO_ANY && (proto == IPPROTO_AH ||
- proto == IPPROTO_ESP ||
- proto == IPPROTO_COMP)));
+ (userproto == IPSEC_PROTO_ANY && xfrm_id_proto_valid(proto)));
}
/*
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 5651c29cb5bd..4af1e1d60b9f 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1951,8 +1951,10 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
if (rq->sadb_x_ipsecrequest_mode == 0)
return -EINVAL;
+ if (!xfrm_id_proto_valid(rq->sadb_x_ipsecrequest_proto))
+ return -EINVAL;
- t->id.proto = rq->sadb_x_ipsecrequest_proto; /* XXX check proto */
+ t->id.proto = rq->sadb_x_ipsecrequest_proto;
if ((mode = pfkey_mode_to_xfrm(rq->sadb_x_ipsecrequest_mode)) < 0)
return -EINVAL;
t->mode = mode;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a131f9ff979e..067ac9ed2918 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1513,20 +1513,8 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
return -EINVAL;
}
- switch (ut[i].id.proto) {
- case IPPROTO_AH:
- case IPPROTO_ESP:
- case IPPROTO_COMP:
-#if IS_ENABLED(CONFIG_IPV6)
- case IPPROTO_ROUTING:
- case IPPROTO_DSTOPTS:
-#endif
- case IPSEC_PROTO_ANY:
- break;
- default:
+ if (!xfrm_id_proto_valid(ut[i].id.proto))
return -EINVAL;
- }
-
}
return 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Patch net] xfrm: unify xfrm protocol checks
2019-03-19 5:08 [Patch net] xfrm: unify xfrm protocol checks Cong Wang
@ 2019-03-19 5:17 ` Herbert Xu
2019-03-19 20:42 ` Cong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2019-03-19 5:17 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, syzbot+0bf0519d6e0de15914fe, Steffen Klassert
On Mon, Mar 18, 2019 at 10:08:24PM -0700, Cong Wang wrote:
>
> +static inline bool xfrm_id_proto_valid(u8 proto)
> +{
> + switch (proto) {
> + case IPPROTO_AH:
> + case IPPROTO_ESP:
> + case IPPROTO_COMP:
> +#if IS_ENABLED(CONFIG_IPV6)
> + case IPPROTO_ROUTING:
> + case IPPROTO_DSTOPTS:
> +#endif
> + case IPSEC_PROTO_ANY:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static inline int xfrm_id_proto_match(u8 proto, u8 userproto)
> {
> return (!userproto || proto == userproto ||
> - (userproto == IPSEC_PROTO_ANY && (proto == IPPROTO_AH ||
> - proto == IPPROTO_ESP ||
> - proto == IPPROTO_COMP)));
> + (userproto == IPSEC_PROTO_ANY && xfrm_id_proto_valid(proto)));
> }
This does not look right. IPSEC_PROTO_ANY should only be allowed
in userproto and your patch is going to let it pass when it's in
proto. Whether IPPROTO_ROUTING/IPPROTO_DSTOPTS should be allowed
in this context is also not obvious.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Patch net] xfrm: unify xfrm protocol checks
2019-03-19 5:17 ` Herbert Xu
@ 2019-03-19 20:42 ` Cong Wang
2019-03-20 5:35 ` Herbert Xu
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2019-03-19 20:42 UTC (permalink / raw)
To: Herbert Xu
Cc: Linux Kernel Network Developers, syzbot+0bf0519d6e0de15914fe,
Steffen Klassert
On Mon, Mar 18, 2019 at 10:17 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Mar 18, 2019 at 10:08:24PM -0700, Cong Wang wrote:
> >
> > +static inline bool xfrm_id_proto_valid(u8 proto)
> > +{
> > + switch (proto) {
> > + case IPPROTO_AH:
> > + case IPPROTO_ESP:
> > + case IPPROTO_COMP:
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + case IPPROTO_ROUTING:
> > + case IPPROTO_DSTOPTS:
> > +#endif
> > + case IPSEC_PROTO_ANY:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static inline int xfrm_id_proto_match(u8 proto, u8 userproto)
> > {
> > return (!userproto || proto == userproto ||
> > - (userproto == IPSEC_PROTO_ANY && (proto == IPPROTO_AH ||
> > - proto == IPPROTO_ESP ||
> > - proto == IPPROTO_COMP)));
> > + (userproto == IPSEC_PROTO_ANY && xfrm_id_proto_valid(proto)));
> > }
>
> This does not look right. IPSEC_PROTO_ANY should only be allowed
> in userproto and your patch is going to let it pass when it's in
> proto. Whether IPPROTO_ROUTING/IPPROTO_DSTOPTS should be allowed
> in this context is also not obvious.
IIRC, it is Steffen who suggested to add IPPROTO_ROUTING/IPPROTO_DSTOPTS
back to commit 6a53b7593233. My xfrm knowledge is not enough to
figure out IPPROTO_ROUTING/IPPROTO_DSTOPTS.
I just assume validate_tmpl() is correct.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Patch net] xfrm: unify xfrm protocol checks
2019-03-19 20:42 ` Cong Wang
@ 2019-03-20 5:35 ` Herbert Xu
2019-03-22 4:06 ` Cong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2019-03-20 5:35 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, syzbot+0bf0519d6e0de15914fe,
Steffen Klassert
On Tue, Mar 19, 2019 at 01:42:53PM -0700, Cong Wang wrote:
>
> IIRC, it is Steffen who suggested to add IPPROTO_ROUTING/IPPROTO_DSTOPTS
> back to commit 6a53b7593233. My xfrm knowledge is not enough to
> figure out IPPROTO_ROUTING/IPPROTO_DSTOPTS.
OK I dug into the history of xfrm_id_proto_match and this is
definitely not right. The intention appears to be that
IPSEC_PROTO_ANY should only match genuine IPsec protocols, i.e.,
AH/ESP/COMP, while the special value of zero will match everything.
So I think what we should do is get rid of the validation function
that you added in 6a5t3b7593233, and then change those internal
functions which were incorrectly using IPSEC_PROTO_ANY to using
zero instead.
Does anybody still use IPPROTO_ROUTING/IPPROTO_DSTOPTS? It's always
a pain when people come and add features and then don't shoulder
the burden of maintaining them.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch net] xfrm: unify xfrm protocol checks
2019-03-20 5:35 ` Herbert Xu
@ 2019-03-22 4:06 ` Cong Wang
2019-03-22 4:11 ` Herbert Xu
0 siblings, 1 reply; 7+ messages in thread
From: Cong Wang @ 2019-03-22 4:06 UTC (permalink / raw)
To: Herbert Xu
Cc: Linux Kernel Network Developers, syzbot+0bf0519d6e0de15914fe,
Steffen Klassert
On Tue, Mar 19, 2019 at 10:35 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Mar 19, 2019 at 01:42:53PM -0700, Cong Wang wrote:
> >
> > IIRC, it is Steffen who suggested to add IPPROTO_ROUTING/IPPROTO_DSTOPTS
> > back to commit 6a53b7593233. My xfrm knowledge is not enough to
> > figure out IPPROTO_ROUTING/IPPROTO_DSTOPTS.
>
> OK I dug into the history of xfrm_id_proto_match and this is
> definitely not right. The intention appears to be that
> IPSEC_PROTO_ANY should only match genuine IPsec protocols, i.e.,
> AH/ESP/COMP, while the special value of zero will match everything.
>
> So I think what we should do is get rid of the validation function
> that you added in 6a5t3b7593233, and then change those internal
> functions which were incorrectly using IPSEC_PROTO_ANY to using
> zero instead.
Good point. Replacing IPSEC_PROTO_ANY with zero should
work too, but on the other hand, id.proto is still never allowed to
be any other protocol than these 6 listed, no?
>
> Does anybody still use IPPROTO_ROUTING/IPPROTO_DSTOPTS? It's always
> a pain when people come and add features and then don't shoulder
> the burden of maintaining them.
Yeah, at least iproute2 does the same check:
static const struct typeent xfrmproto_types[] = {
{ "esp", IPPROTO_ESP }, { "ah", IPPROTO_AH }, { "comp", IPPROTO_COMP },
{ "route2", IPPROTO_ROUTING }, { "hao", IPPROTO_DSTOPTS },
{ "ipsec-any", IPSEC_PROTO_ANY },
{ NULL, -1 }
};
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Patch net] xfrm: unify xfrm protocol checks
2019-03-22 4:06 ` Cong Wang
@ 2019-03-22 4:11 ` Herbert Xu
2019-03-22 4:15 ` Cong Wang
0 siblings, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2019-03-22 4:11 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, syzbot+0bf0519d6e0de15914fe,
Steffen Klassert
On Thu, Mar 21, 2019 at 09:06:05PM -0700, Cong Wang wrote:
>
> Good point. Replacing IPSEC_PROTO_ANY with zero should
> work too, but on the other hand, id.proto is still never allowed to
> be any other protocol than these 6 listed, no?
It should never be IPSEC_PROTO_ANY since that's used as a wildcard.
IOW if you're going to tighten up the check for the id.proto filed
in an actual state, you should distinguish between the case of an
ID that's used to add/modify a state vs. an ID that's be used to
query a state. IPSEC_PROTO_ANY and zero should be denied in the
first case and allowed in the second case.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch net] xfrm: unify xfrm protocol checks
2019-03-22 4:11 ` Herbert Xu
@ 2019-03-22 4:15 ` Cong Wang
0 siblings, 0 replies; 7+ messages in thread
From: Cong Wang @ 2019-03-22 4:15 UTC (permalink / raw)
To: Herbert Xu
Cc: Linux Kernel Network Developers, syzbot+0bf0519d6e0de15914fe,
Steffen Klassert
On Thu, Mar 21, 2019 at 9:11 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Mar 21, 2019 at 09:06:05PM -0700, Cong Wang wrote:
> >
> > Good point. Replacing IPSEC_PROTO_ANY with zero should
> > work too, but on the other hand, id.proto is still never allowed to
> > be any other protocol than these 6 listed, no?
>
> It should never be IPSEC_PROTO_ANY since that's used as a wildcard.
>
> IOW if you're going to tighten up the check for the id.proto filed
> in an actual state, you should distinguish between the case of an
> ID that's used to add/modify a state vs. an ID that's be used to
> query a state. IPSEC_PROTO_ANY and zero should be denied in the
> first case and allowed in the second case.
Yeah, this makes sense. Let me see if I can figure this out correctly.
Thanks for the details!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-22 4:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-19 5:08 [Patch net] xfrm: unify xfrm protocol checks Cong Wang
2019-03-19 5:17 ` Herbert Xu
2019-03-19 20:42 ` Cong Wang
2019-03-20 5:35 ` Herbert Xu
2019-03-22 4:06 ` Cong Wang
2019-03-22 4:11 ` Herbert Xu
2019-03-22 4:15 ` Cong Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.