| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20180725223647.141180-1-nharold@google.com>
Date: Wed, 25 Jul 2018 15:36:47 -0700
From: Nathan Harold <nharold@...gle.com>
To: netdev@...r.kernel.org
Cc: Nathan Harold <nharold@...gle.com>
Subject: [PATCH RFC ipsec-next] xfrm: Check Reverse-Mark Lookup Before ADDSA/DELSA
It's possible to insert an SA into the SADB that will
preclude the lookup of other SAs when using the MARK
attribute. The problem occurs based on a particular
sequencing with the marks where a new mark matches
requests for the SA mark of an existing SA but the
inverse is not true. For an example:
1) Add SA with mark=0, mask=0
2) Add an otherwise-identical SA
with mark=0x1234, mask=0xFFFFFFFF
This will fail; however, if done in the reverse order
the second add will succeed:
1) Add an SA with mark=0x1234, mask=0xFFFFFFFF
2) Add an otherwise-identical SA
except with mark=0, mask=0
Then:
3) Delete the SA using mark=0x1234, mask=0xFFFFFFF
4) Dump the SADB, and there will be one SA, and it will
have mark=0x1234, mask=0xFFFFFFFF; the 0/0 SA will
be deleted
This patch addresses the problem by performing a
reverse-match on the mark and preventing ADDSA for
any SA that would 'shadow' an existing SA in the SADB.
This patch also address a bug where it was possible to
add an SA with a mark broader than its mask, which
could never be deleted:
1) ADDSA with mark=0x1234, mask=0xFF
2) DELSA with mark=0x1234, mask=ZZZZ; error=ESRCH
By applying both masks to each mark, bits outside the
mask of a given SA mark will be ignored, and the match
will succeed.
This patch does not make any changes to the 'data'
path, so SAs with such oddly-defined marks will still
be unmatch-able.
Signed-off-by: Nathan Harold <nharold@...gle.com>
---
net/xfrm/xfrm_state.c | 44 +++++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index b669262682c9..ee212a7c91a9 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -815,10 +815,10 @@ xfrm_init_tempstate(struct xfrm_state *x, const struct flowi *fl,
afinfo->init_temprop(x, tmpl, daddr, saddr);
}
-static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
- const xfrm_address_t *daddr,
- __be32 spi, u8 proto,
- unsigned short family)
+static struct xfrm_state *
+__xfrm_state_lookup(struct net *net, u32 mark, u32 mask,
+ const xfrm_address_t *daddr,
+ __be32 spi, u8 proto, unsigned short family)
{
unsigned int h = xfrm_spi_hash(net, daddr, spi, proto, family);
struct xfrm_state *x;
@@ -830,7 +830,7 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
!xfrm_addr_equal(&x->id.daddr, daddr, family))
continue;
- if ((mark & x->mark.m) != x->mark.v)
+ if ((mark ^ x->mark.v) & mask & x->mark.m)
continue;
if (!xfrm_state_hold_rcu(x))
continue;
@@ -840,10 +840,11 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
return NULL;
}
-static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark,
- const xfrm_address_t *daddr,
- const xfrm_address_t *saddr,
- u8 proto, unsigned short family)
+static struct xfrm_state *
+__xfrm_state_lookup_byaddr(struct net *net, u32 mark, u32 mask,
+ const xfrm_address_t *daddr,
+ const xfrm_address_t *saddr,
+ u8 proto, unsigned short family)
{
unsigned int h = xfrm_src_hash(net, daddr, saddr, family);
struct xfrm_state *x;
@@ -855,7 +856,7 @@ static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark,
!xfrm_addr_equal(&x->props.saddr, saddr, family))
continue;
- if ((mark & x->mark.m) != x->mark.v)
+ if ((mark ^ x->mark.v) & mask & x->mark.m)
continue;
if (!xfrm_state_hold_rcu(x))
continue;
@@ -869,15 +870,14 @@ static inline struct xfrm_state *
__xfrm_state_locate(struct xfrm_state *x, int use_spi, int family)
{
struct net *net = xs_net(x);
- u32 mark = x->mark.v & x->mark.m;
if (use_spi)
- return __xfrm_state_lookup(net, mark, &x->id.daddr,
- x->id.spi, x->id.proto, family);
+ return __xfrm_state_lookup(net, x->mark.v, x->mark.m,
+ &x->id.daddr, x->id.spi,
+ x->id.proto, family);
else
- return __xfrm_state_lookup_byaddr(net, mark,
- &x->id.daddr,
- &x->props.saddr,
+ return __xfrm_state_lookup_byaddr(net, x->mark.v, x->mark.m,
+ &x->id.daddr, &x->props.saddr,
x->id.proto, family);
}
@@ -985,8 +985,10 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
x = best;
if (!x && !error && !acquire_in_progress) {
if (tmpl->id.spi &&
- (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
- tmpl->id.proto, encap_family)) != NULL) {
+ (x0 = __xfrm_state_lookup(net, mark, 0xFFFFFFFF,
+ daddr, tmpl->id.spi,
+ tmpl->id.proto,
+ encap_family)) != NULL) {
to_put = x0;
error = -EEXIST;
goto out;
@@ -1617,7 +1619,8 @@ xfrm_state_lookup(struct net *net, u32 mark, const xfrm_address_t *daddr, __be32
struct xfrm_state *x;
rcu_read_lock();
- x = __xfrm_state_lookup(net, mark, daddr, spi, proto, family);
+ x = __xfrm_state_lookup(net, mark, 0xFFFFFFFF,
+ daddr, spi, proto, family);
rcu_read_unlock();
return x;
}
@@ -1631,7 +1634,8 @@ xfrm_state_lookup_byaddr(struct net *net, u32 mark,
struct xfrm_state *x;
spin_lock_bh(&net->xfrm.xfrm_state_lock);
- x = __xfrm_state_lookup_byaddr(net, mark, daddr, saddr, proto, family);
+ x = __xfrm_state_lookup_byaddr(net, mark, 0xFFFFFFFF,
+ daddr, saddr, proto, family);
spin_unlock_bh(&net->xfrm.xfrm_state_lock);
return x;
}
--
2.18.0.345.g5c9ce644c3-goog
Powered by blists - more mailing lists