[PATCH 1/3] netfilter: Fix copy_to_user too small size parametre.

From: Santosh Nayak <redacted>

While copying to userspace, the size of source is 29byte where as
size parametre is 32 byte.  Its leaking extra-information from
kernel space to user space.
Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

Signed-off-by: Santosh Nayak <redacted>
---
 net/bridge/netfilter/ebtables.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 5864cc4..f3fcbd9 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c

@@ -1335,7 +1335,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)m - base);
-	if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }

@@ -1344,7 +1344,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,
     const char *base, char __user *ubase)
 {
 	char __user *hlp = ubase + ((char *)w - base);
-	if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }

@@ -1368,7 +1368,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)
 	ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
 	if (ret != 0)
 		return ret;
-	if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
+	if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
 		return -EFAULT;
 	return 0;
 }

-- 
1.7.4.4

On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:

From: Santosh Nayak <redacted>

While copying to userspace, the size of source is 29byte where as
size parametre is 32 byte.  Its leaking extra-information from
kernel space to user space.
Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

There's no information leak.

Let me clarify this. Have a look at /linux/netfilter/x_tables.h, then
you find:

#define XT_FUNCTION_MAXNAMELEN 30
#define XT_EXTENSION_MAXNAMELEN 29
#define XT_TABLE_MAXNAMELEN 32

For iptables, everything has been 30 bytes, but we stole one
byte to store the revision field for matches/targets.

For ebtables, there's no revision field and the length of the
table name is different.

But linux/netfilter/in ebtables.h, you'll find:

#define EBT_TABLE_MAXNAMELEN 32
#define EBT_CHAIN_MAXNAMELEN EBT_TABLE_MAXNAMELEN
#define EBT_FUNCTION_MAXNAMELEN EBT_TABLE_MAXNAMELEN

Note that someone decided to use 32 bytes for the ebtables
tables/match/target name instead of 30 bytes in iptables.

Yes, it sucks a bit we have to live with these interfaces until
we have some netlink interface for all these things.

Hi Pablo.

copy_to_user( dest, source, length)

Normally,  'length'  is equal to  'sizeof (source) '.

In this case "length"   =   32
       "sizeof(source)"  =   29.

Is it intentional ?
Won't it copy extra 3 bytes of kernel data to userspace ?



regards
santosh



On Thu, Mar 1, 2012 at 3:48 PM, Pablo Neira Ayuso [off-list ref] wrote:

On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:

quoted

From: Santosh Nayak <redacted>

While copying to userspace, the size of source is 29byte where as
size parametre is 32 byte.  Its leaking extra-information from
kernel space to user space.
Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

There's no information leak.

Let me clarify this. Have a look at /linux/netfilter/x_tables.h, then
you find:

#define XT_FUNCTION_MAXNAMELEN 30
#define XT_EXTENSION_MAXNAMELEN 29
#define XT_TABLE_MAXNAMELEN 32

For iptables, everything has been 30 bytes, but we stole one
byte to store the revision field for matches/targets.

For ebtables, there's no revision field and the length of the
table name is different.

But linux/netfilter/in ebtables.h, you'll find:

#define EBT_TABLE_MAXNAMELEN 32
#define EBT_CHAIN_MAXNAMELEN EBT_TABLE_MAXNAMELEN
#define EBT_FUNCTION_MAXNAMELEN EBT_TABLE_MAXNAMELEN

Note that someone decided to use 32 bytes for the ebtables
tables/match/target name instead of 30 bytes in iptables.

Yes, it sucks a bit we have to live with these interfaces until
we have some netlink interface for all these things.

quoted

Signed-off-by: Santosh Nayak <redacted>
---
 net/bridge/netfilter/ebtables.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 5864cc4..f3fcbd9 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c

@@ -1335,7 +1335,7 @@ static inline int ebt_make_matchname(const struct ebt_entry_match *m,

     const char *base, char __user *ubase)
 {
      char __user *hlp = ubase + ((char *)m - base);
-     if (copy_to_user(hlp, m->u.match->name, EBT_FUNCTION_MAXNAMELEN))
+     if (copy_to_user(hlp, m->u.match->name, XT_EXTENSION_MAXNAMELEN))
              return -EFAULT;
      return 0;
 }

@@ -1344,7 +1344,7 @@ static inline int ebt_make_watchername(const struct ebt_entry_watcher *w,

     const char *base, char __user *ubase)
 {
      char __user *hlp = ubase + ((char *)w - base);
-     if (copy_to_user(hlp , w->u.watcher->name, EBT_FUNCTION_MAXNAMELEN))
+     if (copy_to_user(hlp , w->u.watcher->name, XT_EXTENSION_MAXNAMELEN))
              return -EFAULT;
      return 0;
 }

@@ -1368,7 +1368,7 @@ ebt_make_names(struct ebt_entry *e, const char *base, char __user *ubase)

      ret = EBT_WATCHER_ITERATE(e, ebt_make_watchername, base, ubase);
      if (ret != 0)
              return ret;
-     if (copy_to_user(hlp, t->u.target->name, EBT_FUNCTION_MAXNAMELEN))
+     if (copy_to_user(hlp, t->u.target->name, XT_EXTENSION_MAXNAMELEN))
              return -EFAULT;
      return 0;
 }
--
1.7.4.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Actually we had this discussion before.
http://www.spinics.net/lists/netfilter-devel/msg13860.html

Here was how we ended the discussion:
http://www.spinics.net/lists/netfilter-devel/msg14083.html

I was supposed to send a patch, but I lost track.  Sorry about that.
Perhaps you can send it?

regards,
dan carpenter

On Thu, Mar 01, 2012 at 11:18:09AM +0100, Pablo Neira Ayuso wrote:

On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:

quoted

From: Santosh Nayak <redacted>

While copying to userspace, the size of source is 29byte where as
size parametre is 32 byte.  Its leaking extra-information from
kernel space to user space.
Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

There's no information leak.

Where do we clear "m"? 

include/linux/netfilter/x_tables.h
   287  struct xt_match {
   288          struct list_head list;
   289  
   290          const char name[XT_EXTENSION_MAXNAMELEN];
   291          u_int8_t revision;
   292  

There is a 2 byte holes here between "revision" and "match()".  We
copy three bytes past the end of name, so we include revision and
the hole.

But maybe we memset it somewhere?  I'm not sure.

   293          /* Return true or false: return FALSE and set *hotdrop = 1 to
   294             force immediate packet drop. */
   295          /* Arguments changed since 2.6.9, as this must now handle
   296             non-linear skb, using skb_header_pointer and
   297             skb_ip_make_writable. */
   298          bool (*match)(const struct sk_buff *skb,
   299                        struct xt_action_param *);

regards,
dan carpenter

On Thu, Mar 01, 2012 at 04:15:05PM +0530, santosh prasad nayak wrote:

Hi Pablo.

copy_to_user( dest, source, length)

Normally,  'length'  is equal to  'sizeof (source) '.

In this case "length"   =   32
       "sizeof(source)"  =   29.

Is it intentional ?

ebtables expects 32 bytes names.

Won't it copy extra 3 bytes of kernel data to userspace ?

You're right. We have to copy 29 bytes but we have to fill the
remaining bytes with zeroes. I think something like:

char name[EBT_FUNCTION_MAXNAMELEN] = {};

/* user-space ebtables expects 32 bytes-long names, but xt_match uses
 * 29 bytes for that. */
sprintf(name, "%s", m->u.match->name);
if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
...

will resolve this issue.

Would you resend a new patch?

On Thu, Mar 01, 2012 at 02:37:36PM +0300, Dan Carpenter wrote:

On Thu, Mar 01, 2012 at 11:18:09AM +0100, Pablo Neira Ayuso wrote:

quoted

On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:

quoted

From: Santosh Nayak <redacted>

While copying to userspace, the size of source is 29byte where as
size parametre is 32 byte.  Its leaking extra-information from
kernel space to user space.
Replace EBT_FUNCTION_MAXNAMELEN by XT_EXTENSION_MAXNAMELEN.

There's no information leak.

Where do we clear "m"? 

include/linux/netfilter/x_tables.h
   287  struct xt_match {
   288          struct list_head list;
   289  
   290          const char name[XT_EXTENSION_MAXNAMELEN];
   291          u_int8_t revision;
   292  

There is a 2 byte holes here between "revision" and "match()".  We
copy three bytes past the end of name, so we include revision and
the hole.

But maybe we memset it somewhere?  I'm not sure.

xt_match instances are declared as static for each module so it's
allocated in the BSS (already zeroed), is that what you mean?

   293          /* Return true or false: return FALSE and set *hotdrop = 1 to
   294             force immediate packet drop. */
   295          /* Arguments changed since 2.6.9, as this must now handle
   296             non-linear skb, using skb_header_pointer and
   297             skb_ip_make_writable. */
   298          bool (*match)(const struct sk_buff *skb,
   299                        struct xt_action_param *);

regards,
dan carpenter

On Thu, Mar 01, 2012 at 02:06:37PM +0100, Pablo Neira Ayuso wrote:

quoted

Where do we clear "m"? 

include/linux/netfilter/x_tables.h
   287  struct xt_match {
   288          struct list_head list;
   289  
   290          const char name[XT_EXTENSION_MAXNAMELEN];
   291          u_int8_t revision;
   292  

There is a 2 byte holes here between "revision" and "match()".  We
copy three bytes past the end of name, so we include revision and
the hole.

But maybe we memset it somewhere?  I'm not sure.

xt_match instances are declared as static for each module so it's
allocated in the BSS (already zeroed), is that what you mean?

Yeah.  I didn't know how that worked.  Thanks.

regards,
dan carpenter

Thanks Pablo for clarification.
I will resend the patch.


regards
santosh

On Thu, Mar 1, 2012 at 6:33 PM, Pablo Neira Ayuso [off-list ref] wrote:

On Thu, Mar 01, 2012 at 04:15:05PM +0530, santosh prasad nayak wrote:

quoted

Hi Pablo.

copy_to_user( dest, source, length)

Normally,  'length'  is equal to  'sizeof (source) '.

In this case "length"   =   32
       "sizeof(source)"  =   29.

Is it intentional ?

ebtables expects 32 bytes names.

quoted

Won't it copy extra 3 bytes of kernel data to userspace ?

You're right. We have to copy 29 bytes but we have to fill the
remaining bytes with zeroes. I think something like:

char name[EBT_FUNCTION_MAXNAMELEN] = {};

/* user-space ebtables expects 32 bytes-long names, but xt_match uses
 * 29 bytes for that. */
sprintf(name, "%s", m->u.match->name);
if (copy_to_user(hlp, name, EBT_FUNCTION_MAXNAMELEN))
...

will resolve this issue.

Would you resend a new patch?