| Message ID | 6dcb6a58-2699-9cde-3e34-57c142dbcf14@strongswan.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec] xfrm: Ensure consistent address families when resolving templates | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, Apr 24, 2023 at 03:23:02PM +0200, Tobias Brunner wrote: > xfrm_state_find() uses `encap_family` of the current template with > the passed local and remote addresses to find a matching state. > This check makes sure that there is no mismatch and out-of-bounds > read in mixed-family scenarios where optional tunnel or BEET mode > templates were skipped that would have changed the addresses to > match the current template's family. > > This basically enforces the same check as validate_tmpl(), just at > runtime when one or more optional templates might have been skipped. > > Signed-off-by: Tobias Brunner <tobias@strongswan.org> > --- > net/xfrm/xfrm_policy.c | 5 +++++ > 1 file changed, 5 insertions(+) I'm confused. By skipping, you're presumably referring to IPcomp. For IPcomp, skipping should only occur on inbound, but your patch is changing a code path that's only invoked for outbound. What's going on? Thanks,
On Tue, Apr 25, 2023 at 01:34:44PM +0800, Herbert Xu wrote: > On Mon, Apr 24, 2023 at 03:23:02PM +0200, Tobias Brunner wrote: > > xfrm_state_find() uses `encap_family` of the current template with > > the passed local and remote addresses to find a matching state. > > This check makes sure that there is no mismatch and out-of-bounds > > read in mixed-family scenarios where optional tunnel or BEET mode > > templates were skipped that would have changed the addresses to > > match the current template's family. > > > > This basically enforces the same check as validate_tmpl(), just at > > runtime when one or more optional templates might have been skipped. > > > > Signed-off-by: Tobias Brunner <tobias@strongswan.org> > > --- > > net/xfrm/xfrm_policy.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > I'm confused. By skipping, you're presumably referring to IPcomp. > > For IPcomp, skipping should only occur on inbound, but your patch > is changing a code path that's only invoked for outbound. What's > going on? The problem is, that you can configure it for outbound too. Even though, it does not make much sense. syzbot reported a stack-out-of-bounds issue with intermediate optional templates that change the address family: https://www.spinics.net/lists/netdev/msg890567.html I tried to fix this by rejecting such a configuration: https://lore.kernel.org/netdev/ZCZ79IlUW53XxaVr@gauss3.secunet.de/T/ This broke some strongswan configurations. Tobias patch is the next attempt to fix that.
Hi Herbert, > I'm confused. By skipping, you're presumably referring to IPcomp. > > For IPcomp, skipping should only occur on inbound, but your patch > is changing a code path that's only invoked for outbound. What's > going on? At least in theory, there could be applications for optional outbound templates, e.g. an optional ESP transform that's only applied to some of the traffic matching the policy (based on the selector on the state, which is matched against the original flow) followed by a mandatory AH transform (there could even be multiple optional transforms, e.g. using different algorithms, that are selectively applied to traffic). No idea if anybody actually uses this, but the API allows configuring it. And syzbot showed that some combinations are problematic. Regards, Tobias
On Tue, Apr 25, 2023 at 08:47:15AM +0200, Steffen Klassert wrote: > > The problem is, that you can configure it for outbound too. > Even though, it does not make much sense. syzbot reported > a stack-out-of-bounds issue with intermediate optional > templates that change the address family: Does anyone actually use this in the real world? If not we should try to restrict its usage rather than supporting pointless features. I think it should be safe to limit the use of optional transforms on outbound policies to transport mode only. You can then easily verify the sanity of the policy in xfrm_user. Cheers,
On Tue, Apr 25, 2023 at 10:00:32AM +0200, Tobias Brunner wrote: > > At least in theory, there could be applications for optional outbound > templates, e.g. an optional ESP transform that's only applied to some of the > traffic matching the policy (based on the selector on the state, which is > matched against the original flow) followed by a mandatory AH transform > (there could even be multiple optional transforms, e.g. using different > algorithms, that are selectively applied to traffic). No idea if anybody > actually uses this, but the API allows configuring it. And syzbot showed > that some combinations are problematic. OK, if nobody actually is using this we should restrict its usage. How about limiting optional transforms on outbound policies to transport mode only? Cheers,
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 62be042f2ebc..e6dfa55f1c3a 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2440,6 +2440,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl, struct net *net = xp_net(policy); int nx; int i, error; + unsigned short prev_family = family; xfrm_address_t *daddr = xfrm_flowi_daddr(fl, family); xfrm_address_t *saddr = xfrm_flowi_saddr(fl, family); xfrm_address_t tmp; @@ -2462,6 +2463,9 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl, goto fail; local = &tmp; } + } else if (prev_family != tmpl->encap_family) { + error = -EINVAL; + goto fail; } x = xfrm_state_find(remote, local, fl, tmpl, policy, &error, @@ -2471,6 +2475,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl, xfrm[nx++] = x; daddr = remote; saddr = local; + prev_family = tmpl->encap_family; continue; } if (x) {
xfrm_state_find() uses `encap_family` of the current template with the passed local and remote addresses to find a matching state. This check makes sure that there is no mismatch and out-of-bounds read in mixed-family scenarios where optional tunnel or BEET mode templates were skipped that would have changed the addresses to match the current template's family. This basically enforces the same check as validate_tmpl(), just at runtime when one or more optional templates might have been skipped. Signed-off-by: Tobias Brunner <tobias@strongswan.org> --- net/xfrm/xfrm_policy.c | 5 +++++ 1 file changed, 5 insertions(+)