| Message ID | 20190319050824.24742-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] xfrm: unify xfrm protocol checks | expand |
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,
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.
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,
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 } };
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,
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!
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;
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(-)