* [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. @ 2012-03-01 9:16 santosh nayak 2012-03-01 10:18 ` Pablo Neira Ayuso 2012-03-01 11:31 ` Dan Carpenter 0 siblings, 2 replies; 9+ messages in thread From: santosh nayak @ 2012-03-01 9:16 UTC (permalink / raw) To: bart.de.schuymer Cc: pablo, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors, Santosh Nayak 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. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 9:16 [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre santosh nayak @ 2012-03-01 10:18 ` Pablo Neira Ayuso 2012-03-01 10:45 ` santosh prasad nayak 2012-03-01 11:37 ` Dan Carpenter 2012-03-01 11:31 ` Dan Carpenter 1 sibling, 2 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2012-03-01 10:18 UTC (permalink / raw) To: santosh nayak Cc: bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 10:18 ` Pablo Neira Ayuso @ 2012-03-01 10:45 ` santosh prasad nayak 2012-03-01 13:03 ` Pablo Neira Ayuso 2012-03-01 11:37 ` Dan Carpenter 1 sibling, 1 reply; 9+ messages in thread From: santosh prasad nayak @ 2012-03-01 10:45 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 10:45 ` santosh prasad nayak @ 2012-03-01 13:03 ` Pablo Neira Ayuso 2012-03-01 13:51 ` santosh prasad nayak 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2012-03-01 13:03 UTC (permalink / raw) To: santosh prasad nayak Cc: bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors, 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 13:03 ` Pablo Neira Ayuso @ 2012-03-01 13:51 ` santosh prasad nayak 0 siblings, 0 replies; 9+ messages in thread From: santosh prasad nayak @ 2012-03-01 13:51 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, bridge, netdev, linux-kernel, kernel-janitors, 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 10:18 ` Pablo Neira Ayuso 2012-03-01 10:45 ` santosh prasad nayak @ 2012-03-01 11:37 ` Dan Carpenter 2012-03-01 13:06 ` Pablo Neira Ayuso 1 sibling, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2012-03-01 11:37 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: santosh nayak, bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 1383 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 11:37 ` Dan Carpenter @ 2012-03-01 13:06 ` Pablo Neira Ayuso 2012-03-01 13:13 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Pablo Neira Ayuso @ 2012-03-01 13:06 UTC (permalink / raw) To: Dan Carpenter Cc: santosh nayak, bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 13:06 ` Pablo Neira Ayuso @ 2012-03-01 13:13 ` Dan Carpenter 0 siblings, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2012-03-01 13:13 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: santosh nayak, bart.de.schuymer, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 794 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre. 2012-03-01 9:16 [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre santosh nayak 2012-03-01 10:18 ` Pablo Neira Ayuso @ 2012-03-01 11:31 ` Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2012-03-01 11:31 UTC (permalink / raw) To: santosh nayak Cc: bart.de.schuymer, pablo, kaber, shemminger, davem, netfilter-devel, netfilter, coreteam, bridge, netdev, linux-kernel, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 317 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-03-01 13:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-01 9:16 [PATCH 1/3] netfilter: Fix copy_to_user too small size parametre santosh nayak 2012-03-01 10:18 ` Pablo Neira Ayuso 2012-03-01 10:45 ` santosh prasad nayak 2012-03-01 13:03 ` Pablo Neira Ayuso 2012-03-01 13:51 ` santosh prasad nayak 2012-03-01 11:37 ` Dan Carpenter 2012-03-01 13:06 ` Pablo Neira Ayuso 2012-03-01 13:13 ` Dan Carpenter 2012-03-01 11:31 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).