| Message ID | 1330593390-19233-1-git-send-email-santoshprasadnayak@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote: > From: Santosh Nayak <santoshprasadnayak@gmail.com> > > 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. > Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com> > --- > 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 -- 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
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 <pablo@netfilter.org> wrote:
> On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote:
>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>
>> 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.
>
>> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
>> ---
>> 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
--
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
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: > > From: Santosh Nayak <santoshprasadnayak@gmail.com> > > > > 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? -- 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
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: > > On Thu, Mar 01, 2012 at 02:46:30PM +0530, santosh nayak wrote: > > > From: Santosh Nayak <santoshprasadnayak@gmail.com> > > > > > > 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 > -- 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
On Thu, Mar 01, 2012 at 02:06:37PM +0100, Pablo Neira Ayuso wrote: > > 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 <pablo@netfilter.org> wrote: > 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? -- 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 --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; }