| Message ID | 1306220681.2638.40.camel@edumazet-laptop |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2011-05-24 at 09:04 +0200, Eric Dumazet wrote: > Le mardi 24 mai 2011 à 08:57 +0200, Ingo Molnar a écrit : > > * Joe Perches <joe@perches.com> wrote: > > > Maybe for clarity it'd be better to use a switch/case > > > or something like: > Here we are, thanks. Hey Eric. Just more trivia: > [PATCH v2] inet_diag: hide socket pointers > Provide a mayber_hide_ptr() helper and use it in inet_diag to not typo maybe_hide_ptr > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > @@ -798,6 +798,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr, > } > > int kptr_restrict __read_mostly; > +/** > + * maybe_hide_ptr - Eventually nullify a kernel pointer given to user Not a great description. Maybe something like: * maybe_hide_ptr - Set user values of kernel pointers to null * when appropriate [] > diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c > index 6ffe94c..b5646a3 100644 > --- a/net/ipv4/inet_diag.c > +++ b/net/ipv4/inet_diag.c > @@ -84,6 +84,7 @@ static int inet_csk_diag_fill(struct sock *sk, > struct inet_diag_meminfo *minfo = NULL; > unsigned char *b = skb_tail_pointer(skb); > const struct inet_diag_handler *handler; > + u64 ptr; > > handler = inet_diag_table[unlh->nlmsg_type]; > BUG_ON(handler == NULL); > @@ -114,8 +115,9 @@ static int inet_csk_diag_fill(struct sock *sk, > r->idiag_retrans = 0; > > r->id.idiag_if = sk->sk_bound_dev_if; > - r->id.idiag_cookie[0] = (u32)(unsigned long)sk; > - r->id.idiag_cookie[1] = (u32)(((unsigned long)sk >> 31) >> 1); > + ptr = (u64)maybe_hide_ptr(sk); > + r->id.idiag_cookie[0] = (u32)ptr; > + r->id.idiag_cookie[1] = (u32)(ptr >> 32); I think it's be better without the casts using the standard kernel.h macros. void *ptr; ptr = maybe_hide_ptr(sk); r->id.idiag_cookie[0] = lower_32_bits(ptr); r->id.idiag_cookie[1] = upper_32_bits(ptr); -- 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
Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit : > I think it's be better without the casts > using the standard kernel.h macros. > > void *ptr; > > ptr = maybe_hide_ptr(sk); > r->id.idiag_cookie[0] = lower_32_bits(ptr); > r->id.idiag_cookie[1] = upper_32_bits(ptr); > I am not sure I want to patch lower_32_bits() and upper_32_bits() for this. They dont work on pointers, but on "numbers", according to kerneldoc Andrew wrote years ago. gcc agrees : net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’: net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size net/ipv4/inet_diag.c:120: error: invalid operands to binary >> make[1]: *** [net/ipv4/inet_diag.o] Error 1 -- 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
On Tue, 24 May 2011 03:58:01 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 24 May 2011 09:45:01 +0200 > > > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit : > > > >> I think it's be better without the casts > >> using the standard kernel.h macros. > >> > >> void *ptr; > >> > >> ptr = maybe_hide_ptr(sk); > >> r->id.idiag_cookie[0] = lower_32_bits(ptr); > >> r->id.idiag_cookie[1] = upper_32_bits(ptr); > >> > > > > I am not sure I want to patch lower_32_bits() and upper_32_bits() for > > this. > > > > They dont work on pointers, but on "numbers", according to kerneldoc > > Andrew wrote years ago. gcc agrees : > > > > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’: > > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size > > net/ipv4/inet_diag.c:120: error: invalid operands to binary >> > > make[1]: *** [net/ipv4/inet_diag.o] Error 1 > > Also you can't do this, the "cookie" is used by the kernel future > lookups to find sockets. > > The kernel pointer is part of the API, so sorry you can't "hide" > kernel pointers in this case without really breaking user visible > things. > [Error decoding BASE64] Couldn't the pointer be replaced by a socket index?
From: Stephen Hemminger <shemminger@vyatta.com> Date: Tue, 24 May 2011 07:43:55 -0700 > Couldn't the pointer be replaced by a socket index? Well, it's a pointer exactly so we don't have to index into a table. -- 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
Hi David, On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 24 May 2011 09:45:01 +0200 > > > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit : > > > >> I think it's be better without the casts > >> using the standard kernel.h macros. > >> > >> void *ptr; > >> > >> ptr = maybe_hide_ptr(sk); > >> r->id.idiag_cookie[0] = lower_32_bits(ptr); > >> r->id.idiag_cookie[1] = upper_32_bits(ptr); > >> > > > > I am not sure I want to patch lower_32_bits() and upper_32_bits() for > > this. > > > > They dont work on pointers, but on "numbers", according to kerneldoc > > Andrew wrote years ago. gcc agrees : > > > > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’: > > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size > > net/ipv4/inet_diag.c:120: error: invalid operands to binary >> > > make[1]: *** [net/ipv4/inet_diag.o] Error 1 > > Also you can't do this, the "cookie" is used by the kernel future > lookups to find sockets. > > The kernel pointer is part of the API, so sorry you can't "hide" > kernel pointers in this case without really breaking user visible > things. But this is precisely what we're trying to control with kptr_restrict. Setting kptr_restrict will make inet_diag (and some details of similar things in /proc) meaningless. Based on the name, "diag" isn't going to be used in normal operation, and kptr_restrict is 0 by default, so only system owners interested in this will enable it and effectively disable inet_diag. It seems like everything that fills idiag_cookie needs to be adjusted, not just the one instance, too: $ fgrep 'idiag_cookie[0] = ' net/ipv4/inet_diag.c r->id.idiag_cookie[0] = (u32)(unsigned long)sk; r->id.idiag_cookie[0] = (u32)(unsigned long)tw; r->id.idiag_cookie[0] = (u32)(unsigned long)req; -Kees
From: Kees Cook <kees.cook@canonical.com> Date: Wed, 25 May 2011 16:29:21 -0700 > Hi David, > > On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote: >> From: Eric Dumazet <eric.dumazet@gmail.com> >> Date: Tue, 24 May 2011 09:45:01 +0200 >> >> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit : >> > >> >> I think it's be better without the casts >> >> using the standard kernel.h macros. >> >> >> >> void *ptr; >> >> >> >> ptr = maybe_hide_ptr(sk); >> >> r->id.idiag_cookie[0] = lower_32_bits(ptr); >> >> r->id.idiag_cookie[1] = upper_32_bits(ptr); >> >> >> > >> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for >> > this. >> > >> > They dont work on pointers, but on "numbers", according to kerneldoc >> > Andrew wrote years ago. gcc agrees : >> > >> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’: >> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size >> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >> >> > make[1]: *** [net/ipv4/inet_diag.o] Error 1 >> >> Also you can't do this, the "cookie" is used by the kernel future >> lookups to find sockets. >> >> The kernel pointer is part of the API, so sorry you can't "hide" >> kernel pointers in this case without really breaking user visible >> things. > > But this is precisely what we're trying to control with kptr_restrict. > Setting kptr_restrict will make inet_diag (and some details of similar > things in /proc) meaningless. Based on the name, "diag" isn't going to be > used in normal operation, and kptr_restrict is 0 by default, so only system > owners interested in this will enable it and effectively disable inet_diag. Are you kidding me? inet_diag is the standard way to dump sockets using netlink. It's not a special obscure debugging facility, it's for real users. And the encoded kernel pointer here is used as a shortcut to looking up precise sockets.
On Wed, May 25, 2011 at 09:50:40PM -0400, David Miller wrote: > From: Kees Cook <kees.cook@canonical.com> > Date: Wed, 25 May 2011 16:29:21 -0700 > > > Hi David, > > > > On Tue, May 24, 2011 at 03:58:01AM -0400, David Miller wrote: > >> From: Eric Dumazet <eric.dumazet@gmail.com> > >> Date: Tue, 24 May 2011 09:45:01 +0200 > >> > >> > Le mardi 24 mai 2011 à 00:35 -0700, Joe Perches a écrit : > >> > > >> >> I think it's be better without the casts > >> >> using the standard kernel.h macros. > >> >> > >> >> void *ptr; > >> >> > >> >> ptr = maybe_hide_ptr(sk); > >> >> r->id.idiag_cookie[0] = lower_32_bits(ptr); > >> >> r->id.idiag_cookie[1] = upper_32_bits(ptr); > >> >> > >> > > >> > I am not sure I want to patch lower_32_bits() and upper_32_bits() for > >> > this. > >> > > >> > They dont work on pointers, but on "numbers", according to kerneldoc > >> > Andrew wrote years ago. gcc agrees : > >> > > >> > net/ipv4/inet_diag.c: In function ‘inet_csk_diag_fill’: > >> > net/ipv4/inet_diag.c:119: warning: cast from pointer to integer of different size > >> > net/ipv4/inet_diag.c:120: error: invalid operands to binary >> > >> > make[1]: *** [net/ipv4/inet_diag.o] Error 1 > >> > >> Also you can't do this, the "cookie" is used by the kernel future > >> lookups to find sockets. > >> > >> The kernel pointer is part of the API, so sorry you can't "hide" > >> kernel pointers in this case without really breaking user visible > >> things. > > > > But this is precisely what we're trying to control with kptr_restrict. > > Setting kptr_restrict will make inet_diag (and some details of similar > > things in /proc) meaningless. Based on the name, "diag" isn't going to be > > used in normal operation, and kptr_restrict is 0 by default, so only system > > owners interested in this will enable it and effectively disable inet_diag. > > Are you kidding me? > > inet_diag is the standard way to dump sockets using netlink. > It's not a special obscure debugging facility, it's for real > users. > > And the encoded kernel pointer here is used as a shortcut to looking > up precise sockets. We got this dropped from the /proc view; why can't we do the same for this netlink interface? -Kees
Le jeudi 26 mai 2011 à 22:44 -0400, David Miller a écrit : > From: Kees Cook <kees.cook@canonical.com> > Date: Thu, 26 May 2011 17:14:49 -0700 > > > > We got this dropped from the /proc view; why can't we do the same for > > this netlink interface? > > Because it's not only an opaque "output" blob, it's also an input key > for lookups which the user can trigger. Yes, we wan add a layer to obfuscate the real pointers. We dont trust values given by user, only match them. Either we use a XOR with a boot time random value (but let the NULL cookie being the NULL one), or we generate an unique 64bit socket id for the cookie (and keep a 64bit cookie in all sockets, increasing ram usage) -- 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
* Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le jeudi 26 mai 2011 à 22:44 -0400, David Miller a écrit : > > From: Kees Cook <kees.cook@canonical.com> > > Date: Thu, 26 May 2011 17:14:49 -0700 > > > > > > We got this dropped from the /proc view; why can't we do the same for > > > this netlink interface? > > > > Because it's not only an opaque "output" blob, it's also an input key > > for lookups which the user can trigger. > > Yes, we wan add a layer to obfuscate the real pointers. We dont > trust values given by user, only match them. > > Either we use a XOR with a boot time random value (but let the NULL > cookie being the NULL one), or we generate an unique 64bit socket > id for the cookie (and keep a 64bit cookie in all sockets, > increasing ram usage) FYI, Dan Rosenberg is currently working on a kernel image randomization feature, see this lkml thread: [RFC][PATCH] Randomize kernel base address on boot That will come with an easy vsprintf method to 'unrandomize' IPs. ( this will be used to display a real-looking /proc/kallsyms and all IPs that the kernel passes to user-space (via perf, etc.) will be unrandomized as well, protecting the randomization seed. ) Once that code goes upstream the networking code could rather simply use it to 'randomize' these real data pointers as well. (Assuming you never ever pass in zero, that would expose the secret seed.) The only worry would be statistical analysis performed by local attackers: by creating and closing enough sockets on a busy system you can over time cover almost all ranges of RAM, so if you can observe a pattern of 'over the address space limit' addresses at the top or the bottom of the address space you can estimate the random seed to within 4-5 bits realistically, maybe even more. The upper and lower limit of the address space (big holes are useful too) can be calculated rather precisely based on RAM size, the identity of the system and the kernel version. So maybe networking real-pointer randomization should be separate from kernel IP randomization after all. Thanks, Ingo -- 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
diff --git a/include/linux/printk.h b/include/linux/printk.h index ee048e7..47c0cef 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -111,6 +111,7 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies, extern int printk_delay_msec; extern int dmesg_restrict; extern int kptr_restrict; +void *maybe_hide_ptr(void *ptr); void log_buf_kexec_setup(void); #else diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 1d659d7..db1a193 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -798,6 +798,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr, } int kptr_restrict __read_mostly; +/** + * maybe_hide_ptr - Eventually nullify a kernel pointer given to user + * @ptr: kernel pointer + * + * kptr_restrict = 0 : kernel pointer not changed + * kptr_restrict = 1 : if the current user does not have CAP_SYSLOG, + * kernel pointer is replaced by 0 + * kptr_restrict = 2 : kernel pointer is replaced by 0 for all users. + */ +void *maybe_hide_ptr(void *ptr) +{ + if (!kptr_restrict) + return ptr; + + if (kptr_restrict == 1 && has_capability_noaudit(current, CAP_SYSLOG)) + return ptr; + + return NULL; +} +EXPORT_SYMBOL(maybe_hide_ptr); /* * Show a '%p' thing. A kernel extension is that the '%p' is followed @@ -911,10 +931,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, spec.field_width = 2 * sizeof(void *); return string(buf, end, "pK-error", spec); } - if (!((kptr_restrict == 0) || - (kptr_restrict == 1 && - has_capability_noaudit(current, CAP_SYSLOG)))) - ptr = NULL; + ptr = maybe_hide_ptr(ptr); break; } spec.flags |= SMALL; diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c index 6ffe94c..b5646a3 100644 --- a/net/ipv4/inet_diag.c +++ b/net/ipv4/inet_diag.c @@ -84,6 +84,7 @@ static int inet_csk_diag_fill(struct sock *sk, struct inet_diag_meminfo *minfo = NULL; unsigned char *b = skb_tail_pointer(skb); const struct inet_diag_handler *handler; + u64 ptr; handler = inet_diag_table[unlh->nlmsg_type]; BUG_ON(handler == NULL); @@ -114,8 +115,9 @@ static int inet_csk_diag_fill(struct sock *sk, r->idiag_retrans = 0; r->id.idiag_if = sk->sk_bound_dev_if; - r->id.idiag_cookie[0] = (u32)(unsigned long)sk; - r->id.idiag_cookie[1] = (u32)(((unsigned long)sk >> 31) >> 1); + ptr = (u64)maybe_hide_ptr(sk); + r->id.idiag_cookie[0] = (u32)ptr; + r->id.idiag_cookie[1] = (u32)(ptr >> 32); r->id.idiag_sport = inet->inet_sport; r->id.idiag_dport = inet->inet_dport;