* Re: Fwd: Netlink XFRM socket subsystem NULL pointer dereference [not found] ` <20171019092625.GA12863@gondor.apana.org.au> @ 2017-10-19 9:57 ` Herbert Xu 2017-10-19 11:33 ` Timo Teras 0 siblings, 1 reply; 4+ messages in thread From: Herbert Xu @ 2017-10-19 9:57 UTC (permalink / raw) To: Eric Dumazet; +Cc: Steffen Klassert, Timo Teras, netdev On Thu, Oct 19, 2017 at 05:26:25PM +0800, Herbert Xu wrote: > > So it's an netlink API issue. It is possible for cb->done to be > called without cb->dump ever being called. And xfrm_user doesn't > deal with that. Let me survey the others to see whether we should > fix this in netlink, xfrm, or both. OK it looks like an XFRM bug pure and simple. Funnily enough the xfrm sa dump code which got changed by the same commit that added this bug isn't buggy. ---8<--- Subject: ipsec: Fix aborted xfrm policy dump crash An independent security researcher, Mohamed Ghannam, has reported this vulnerability to Beyond Security's SecuriTeam Secure Disclosure program. The xfrm_dump_policy_done function expects xfrm_dump_policy to have been called at least once or it will crash. This can be triggered if a dump fails because the target socket's receive buffer is full. This patch fixes it by using the cb->start mechanism to ensure that the initialisation is always done regardless of the buffer situation. Fixes: 4c563f7669c1 ("[XFRM]: Speed up xfrm_policy and xfrm_state...") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 2bfbd91..98a6d65 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -1692,32 +1692,34 @@ static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr static int xfrm_dump_policy_done(struct netlink_callback *cb) { - struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1]; + struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args; struct net *net = sock_net(cb->skb->sk); xfrm_policy_walk_done(walk, net); return 0; } +static int xfrm_dump_policy_start(struct netlink_callback *cb) +{ + struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args; + + BUILD_BUG_ON(sizeof(*walk) > sizeof(cb->args)); + + xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY); + return 0; +} + static int xfrm_dump_policy(struct sk_buff *skb, struct netlink_callback *cb) { struct net *net = sock_net(skb->sk); - struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1]; + struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args; struct xfrm_dump_info info; - BUILD_BUG_ON(sizeof(struct xfrm_policy_walk) > - sizeof(cb->args) - sizeof(cb->args[0])); - info.in_skb = cb->skb; info.out_skb = skb; info.nlmsg_seq = cb->nlh->nlmsg_seq; info.nlmsg_flags = NLM_F_MULTI; - if (!cb->args[0]) { - cb->args[0] = 1; - xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY); - } - (void) xfrm_policy_walk(net, walk, dump_one_policy, &info); return skb->len; @@ -2473,6 +2475,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type, static const struct xfrm_link { int (*doit)(struct sk_buff *, struct nlmsghdr *, struct nlattr **); + int (*start)(struct netlink_callback *); int (*dump)(struct sk_buff *, struct netlink_callback *); int (*done)(struct netlink_callback *); const struct nla_policy *nla_pol; @@ -2486,6 +2489,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type, [XFRM_MSG_NEWPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_add_policy }, [XFRM_MSG_DELPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_get_policy }, [XFRM_MSG_GETPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_get_policy, + .start = xfrm_dump_policy_start, .dump = xfrm_dump_policy, .done = xfrm_dump_policy_done }, [XFRM_MSG_ALLOCSPI - XFRM_MSG_BASE] = { .doit = xfrm_alloc_userspi }, @@ -2538,6 +2542,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, { struct netlink_dump_control c = { + .start = link->start, .dump = link->dump, .done = link->done, }; -- 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 related [flat|nested] 4+ messages in thread
* Re: Netlink XFRM socket subsystem NULL pointer dereference 2017-10-19 9:57 ` Fwd: Netlink XFRM socket subsystem NULL pointer dereference Herbert Xu @ 2017-10-19 11:33 ` Timo Teras 2017-10-19 12:51 ` [PATCH v2] ipsec: Fix aborted xfrm policy dump crash Herbert Xu 0 siblings, 1 reply; 4+ messages in thread From: Timo Teras @ 2017-10-19 11:33 UTC (permalink / raw) To: Herbert Xu; +Cc: Eric Dumazet, Steffen Klassert, netdev On Thu, 19 Oct 2017 17:57:04 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Thu, Oct 19, 2017 at 05:26:25PM +0800, Herbert Xu wrote: > > > > So it's an netlink API issue. It is possible for cb->done to be > > called without cb->dump ever being called. And xfrm_user doesn't > > deal with that. Let me survey the others to see whether we should > > fix this in netlink, xfrm, or both. > > OK it looks like an XFRM bug pure and simple. Funnily enough > the xfrm sa dump code which got changed by the same commit that > added this bug isn't buggy. > > ---8<--- > Subject: ipsec: Fix aborted xfrm policy dump crash > > An independent security researcher, Mohamed Ghannam, has reported > this vulnerability to Beyond Security's SecuriTeam Secure Disclosure > program. > > The xfrm_dump_policy_done function expects xfrm_dump_policy to > have been called at least once or it will crash. This can be > triggered if a dump fails because the target socket's receive > buffer is full. > > This patch fixes it by using the cb->start mechanism to ensure that > the initialisation is always done regardless of the buffer situation. > > Fixes: 4c563f7669c1 ("[XFRM]: Speed up xfrm_policy and xfrm_state...") This is not correct. The original commit works just fine. The bug was introduced later. And SA side had the same issue, it got fixed in commit: 1ba5bf993c6a3142e18e "xfrm: fix crash in XFRM_MSG_GETSA netlink handler" The commit introducing these issues is 12a169e7d8f4b1c95252 "ipsec: Put dumpers on the dump list". where the walker state was modified to contain list entry instead of pointer reference. At that time there was no .start which got added just few years ago. I suggest to do the same fix for SA side since it had same issue fixed on the other commit. Your approach with defining the .start is cleaner. With updated Fixed line, and preferably applying the same to SA side, you can add: Acked-by: Timo Teräs <timo.teras@iki.fi> Thanks, Timo ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] ipsec: Fix aborted xfrm policy dump crash 2017-10-19 11:33 ` Timo Teras @ 2017-10-19 12:51 ` Herbert Xu 2017-10-23 11:08 ` Steffen Klassert 0 siblings, 1 reply; 4+ messages in thread From: Herbert Xu @ 2017-10-19 12:51 UTC (permalink / raw) To: Timo Teras; +Cc: Eric Dumazet, Steffen Klassert, netdev On Thu, Oct 19, 2017 at 02:33:20PM +0300, Timo Teras wrote: > > > Fixes: 4c563f7669c1 ("[XFRM]: Speed up xfrm_policy and xfrm_state...") > > This is not correct. The original commit works just fine. OK, I'll change it. > At that time there was no .start which got added just few years ago. I > suggest to do the same fix for SA side since it had same issue fixed on > the other commit. Your approach with defining the .start is cleaner. No we can't use the start on the SA side because as it is start is not allowed to fail. Thanks, ---8<--- An independent security researcher, Mohamed Ghannam, has reported this vulnerability to Beyond Security's SecuriTeam Secure Disclosure program. The xfrm_dump_policy_done function expects xfrm_dump_policy to have been called at least once or it will crash. This can be triggered if a dump fails because the target socket's receive buffer is full. This patch fixes it by using the cb->start mechanism to ensure that the initialisation is always done regardless of the buffer situation. Fixes: 12a169e7d8f4 ("ipsec: Put dumpers on the dump list") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 2bfbd91..98a6d65 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -1692,32 +1692,34 @@ static int dump_one_policy(struct xfrm_policy *xp, int dir, int count, void *ptr static int xfrm_dump_policy_done(struct netlink_callback *cb) { - struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1]; + struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args; struct net *net = sock_net(cb->skb->sk); xfrm_policy_walk_done(walk, net); return 0; } +static int xfrm_dump_policy_start(struct netlink_callback *cb) +{ + struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args; + + BUILD_BUG_ON(sizeof(*walk) > sizeof(cb->args)); + + xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY); + return 0; +} + static int xfrm_dump_policy(struct sk_buff *skb, struct netlink_callback *cb) { struct net *net = sock_net(skb->sk); - struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *) &cb->args[1]; + struct xfrm_policy_walk *walk = (struct xfrm_policy_walk *)cb->args; struct xfrm_dump_info info; - BUILD_BUG_ON(sizeof(struct xfrm_policy_walk) > - sizeof(cb->args) - sizeof(cb->args[0])); - info.in_skb = cb->skb; info.out_skb = skb; info.nlmsg_seq = cb->nlh->nlmsg_seq; info.nlmsg_flags = NLM_F_MULTI; - if (!cb->args[0]) { - cb->args[0] = 1; - xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY); - } - (void) xfrm_policy_walk(net, walk, dump_one_policy, &info); return skb->len; @@ -2473,6 +2475,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type, static const struct xfrm_link { int (*doit)(struct sk_buff *, struct nlmsghdr *, struct nlattr **); + int (*start)(struct netlink_callback *); int (*dump)(struct sk_buff *, struct netlink_callback *); int (*done)(struct netlink_callback *); const struct nla_policy *nla_pol; @@ -2486,6 +2489,7 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type, [XFRM_MSG_NEWPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_add_policy }, [XFRM_MSG_DELPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_get_policy }, [XFRM_MSG_GETPOLICY - XFRM_MSG_BASE] = { .doit = xfrm_get_policy, + .start = xfrm_dump_policy_start, .dump = xfrm_dump_policy, .done = xfrm_dump_policy_done }, [XFRM_MSG_ALLOCSPI - XFRM_MSG_BASE] = { .doit = xfrm_alloc_userspi }, @@ -2538,6 +2542,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, { struct netlink_dump_control c = { + .start = link->start, .dump = link->dump, .done = link->done, }; -- 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 related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipsec: Fix aborted xfrm policy dump crash 2017-10-19 12:51 ` [PATCH v2] ipsec: Fix aborted xfrm policy dump crash Herbert Xu @ 2017-10-23 11:08 ` Steffen Klassert 0 siblings, 0 replies; 4+ messages in thread From: Steffen Klassert @ 2017-10-23 11:08 UTC (permalink / raw) To: Herbert Xu; +Cc: Timo Teras, Eric Dumazet, netdev On Thu, Oct 19, 2017 at 08:51:10PM +0800, Herbert Xu wrote: > On Thu, Oct 19, 2017 at 02:33:20PM +0300, Timo Teras wrote: > > > > > Fixes: 4c563f7669c1 ("[XFRM]: Speed up xfrm_policy and xfrm_state...") > > > > This is not correct. The original commit works just fine. > > OK, I'll change it. > > > At that time there was no .start which got added just few years ago. I > > suggest to do the same fix for SA side since it had same issue fixed on > > the other commit. Your approach with defining the .start is cleaner. > > No we can't use the start on the SA side because as it is start > is not allowed to fail. > > Thanks, > > ---8<--- > An independent security researcher, Mohamed Ghannam, has reported > this vulnerability to Beyond Security's SecuriTeam Secure Disclosure > program. > > The xfrm_dump_policy_done function expects xfrm_dump_policy to > have been called at least once or it will crash. This can be > triggered if a dump fails because the target socket's receive > buffer is full. > > This patch fixes it by using the cb->start mechanism to ensure that > the initialisation is always done regardless of the buffer situation. > > Fixes: 12a169e7d8f4 ("ipsec: Put dumpers on the dump list") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Applied, thanks a lot Herbert! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-23 11:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAHqykcSO0vQvh0uXwDNgXr0ZpP8m4AGZ=ks9FNshJZw0=GWHpw@mail.gmail.com>
[not found] ` <CANn89iLBO2RzqMzb61Qn=HFncsfYLnXLcUfSqFsDTPakQ2o4FQ@mail.gmail.com>
[not found] ` <20171017061832.GA3323@secunet.com>
[not found] ` <CANn89iJmtPQ=HV6xLjTaLS6WnVmiN+PgaObmWWnevcEzLitRTQ@mail.gmail.com>
[not found] ` <20171018151159.GA5188@gondor.apana.org.au>
[not found] ` <CANn89iL_QRZUTDXdci+Nsn=oM471N4r6im0zLgHkJANx_X6wbQ@mail.gmail.com>
[not found] ` <20171019092625.GA12863@gondor.apana.org.au>
2017-10-19 9:57 ` Fwd: Netlink XFRM socket subsystem NULL pointer dereference Herbert Xu
2017-10-19 11:33 ` Timo Teras
2017-10-19 12:51 ` [PATCH v2] ipsec: Fix aborted xfrm policy dump crash Herbert Xu
2017-10-23 11:08 ` Steffen Klassert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).