diff mbox

[nf,v2] ebtables: arpreply: Add the standard target sanity check

Message ID 1494898218-97426-1-git-send-email-gfree.wind@vip.163.com
State Accepted
Delegated to: Pablo Neira
Headers show

Commit Message

Gao Feng May 16, 2017, 1:30 a.m. UTC
From: Gao Feng <gfree.wind@vip.163.com>

The info->target is from userspace and it would be used directly.
So we need to add the sanity check to make sure it is a valid standard
target, although the ebtables tool has already checked it. Kernel need
to check anything from userspace.

If the target was set as an evil value, it would break the ebtables
and cause a panic. Because the non-standard target is treated as one
offset.

Now add one helper function ebt_invalid_target, and we would replace
the macro INVALID_TARGET later.

Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
---
 v2: Add one helper function ebt_invalid_target instead of INVALID_TARGET, per Pablo
 v1: initial version

 include/linux/netfilter_bridge/ebtables.h | 5 +++++
 net/bridge/netfilter/ebt_arpreply.c       | 3 +++
 2 files changed, 8 insertions(+)

Comments

Pablo Neira Ayuso May 16, 2017, 8:24 a.m. UTC | #1
On Tue, May 16, 2017 at 09:30:18AM +0800, gfree.wind@vip.163.com wrote:
> From: Gao Feng <gfree.wind@vip.163.com>
> 
> The info->target is from userspace and it would be used directly.
> So we need to add the sanity check to make sure it is a valid standard
> target, although the ebtables tool has already checked it. Kernel need
> to check anything from userspace.
> 
> If the target was set as an evil value, it would break the ebtables
> and cause a panic. Because the non-standard target is treated as one
> offset.
> 
> Now add one helper function ebt_invalid_target, and we would replace
> the macro INVALID_TARGET later.

Applied, thanks.

There is a few bunch of spots that can use this indeed. Follow up with
a patch for nf-next once merge window opens up.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso May 16, 2017, 9:43 a.m. UTC | #2
On Tue, May 16, 2017 at 10:24:00AM +0200, Pablo Neira Ayuso wrote:
> On Tue, May 16, 2017 at 09:30:18AM +0800, gfree.wind@vip.163.com wrote:
> > From: Gao Feng <gfree.wind@vip.163.com>
> > 
> > The info->target is from userspace and it would be used directly.
> > So we need to add the sanity check to make sure it is a valid standard
> > target, although the ebtables tool has already checked it. Kernel need
> > to check anything from userspace.
> > 
> > If the target was set as an evil value, it would break the ebtables
> > and cause a panic. Because the non-standard target is treated as one
> > offset.
> > 
> > Now add one helper function ebt_invalid_target, and we would replace
> > the macro INVALID_TARGET later.
> 
> Applied, thanks.
> 
> There is a few bunch of spots that can use this indeed. Follow up with
> a patch for nf-next once merge window opens up.

Please, use:

netfilter: ...

as you initial patch subject next time...
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso May 16, 2017, 9:55 a.m. UTC | #3
On Tue, May 16, 2017 at 05:53:51PM +0800, Gao Feng wrote:
> 
> At 2017-05-16 17:43:24, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
> >On Tue, May 16, 2017 at 10:24:00AM +0200, Pablo Neira Ayuso wrote:
> >> On Tue, May 16, 2017 at 09:30:18AM +0800, gfree.wind@vip.163.com wrote:
> >> > From: Gao Feng <gfree.wind@vip.163.com>
> >> > 
> >> > The info->target is from userspace and it would be used directly.
> >> > So we need to add the sanity check to make sure it is a valid standard
> >> > target, although the ebtables tool has already checked it. Kernel need
> >> > to check anything from userspace.
> >> > 
> >> > If the target was set as an evil value, it would break the ebtables
> >> > and cause a panic. Because the non-standard target is treated as one
> >> > offset.
> >> > 
> >> > Now add one helper function ebt_invalid_target, and we would replace
> >> > the macro INVALID_TARGET later.
> >> 
> >> Applied, thanks.
> >> 
> >> There is a few bunch of spots that can use this indeed. Follow up with
> >> a patch for nf-next once merge window opens up.
> 
> I would pay some attention when nf is merged into nf-next.

Thanks.

> >Please, use:
> >
> >netfilter: ...
> >
> >as you initial patch subject next time...
> 
> OK. I thought the ebtables codes should use prefix "ebtables: ", and I checked it with git log.
> There were some commits which uses "ebtables" as prefix.
> 
> Could I assume both of ebtables and arptables uses the netfilter as prefix?

We should converge to netfilter: extension: blah.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/netfilter_bridge/ebtables.h b/include/linux/netfilter_bridge/ebtables.h
index a30efb4..e0cbf17 100644
--- a/include/linux/netfilter_bridge/ebtables.h
+++ b/include/linux/netfilter_bridge/ebtables.h
@@ -125,4 +125,9 @@  extern unsigned int ebt_do_table(struct sk_buff *skb,
 /* True if the target is not a standard target */
 #define INVALID_TARGET (info->target < -NUM_STANDARD_TARGETS || info->target >= 0)
 
+static inline bool ebt_invalid_target(int target)
+{
+	return (target < -NUM_STANDARD_TARGETS || target >= 0);
+}
+
 #endif
diff --git a/net/bridge/netfilter/ebt_arpreply.c b/net/bridge/netfilter/ebt_arpreply.c
index 5929309..db85230 100644
--- a/net/bridge/netfilter/ebt_arpreply.c
+++ b/net/bridge/netfilter/ebt_arpreply.c
@@ -68,6 +68,9 @@  static int ebt_arpreply_tg_check(const struct xt_tgchk_param *par)
 	if (e->ethproto != htons(ETH_P_ARP) ||
 	    e->invflags & EBT_IPROTO)
 		return -EINVAL;
+	if (ebt_invalid_target(info->target))
+		return -EINVAL;
+
 	return 0;
 }