[RFC][PATCH] utsname: completely overwrite prior information

[RFC][PATCH] utsname: completely overwrite prior information

[Vegard Nossum]

>From 25c69de4760e20cf7562cf92a55b65a71546093e Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Thu, 11 Sep 2008 19:59:50 +0200
Subject: [PATCH] utsname: completely overwrite prior information

On sethostname() and setdomainname(), previous information may be
retained if it was longer than than the new hostname/domainname.

This can be demonstrated trivially by calling sethostname() first
with a long name, then with a short name, and then calling uname()
to retrieve the full buffer that contains the hostname (and
possibly parts of the old hostname), one just has to look past the
terminating zero.

I don't know if we should really care that much (hence the RFC);
the only scenarios I can possibly think of is administrator
putting something sensitive in the hostname (or domain name) by
accident, and changing it back will not undo the mistake entirely,
though it's not like we can recover gracefully from "rm -rf /"
either... The other scenario is namespaces (CLONE_NEWUTS) where
some information may be unintentionally "inherited" from the
previous namespace (a program wants to hide the original name and
does clone + sethostname, but some information is still left).

I think the patch may be defended on grounds of the principle of
least surprise. But I am not adamant :-)

(I guess the question now is whether userspace should be able to
write embedded NULs into the buffer or not...)

At least the observation has been made and the patch has been
presented.

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 kernel/sys.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 038a7bc..78b4515 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1352,7 +1352,8 @@ asmlinkage long sys_sethostname(char __user *name, int len)
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
 		memcpy(utsname()->nodename, tmp, len);
-		utsname()->nodename[len] = 0;
+		memset(utsname()->nodename + len, 0,
+			sizeof(utsname()->nodename) - len);
 		errno = 0;
 	}
 	up_write(&uts_sem);
@@ -1398,7 +1399,8 @@ asmlinkage long sys_setdomainname(char __user *name, int len)
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
 		memcpy(utsname()->domainname, tmp, len);
-		utsname()->domainname[len] = 0;
+		memset(utsname()->domainname + len, 0,
+			sizeof(utsname()->domainname) - len);
 		errno = 0;
 	}
 	up_write(&uts_sem);
-- 
1.5.5.1

Re: [RFC][PATCH] utsname: completely overwrite prior information

[Andrew Morton]

On Fri, 12 Sep 2008 22:36:24 +0200
Vegard Nossum <vegard.nossum@gmail.com> wrote:

> >From 25c69de4760e20cf7562cf92a55b65a71546093e Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegard.nossum@gmail.com>
> Date: Thu, 11 Sep 2008 19:59:50 +0200
> Subject: [PATCH] utsname: completely overwrite prior information
> 
> On sethostname() and setdomainname(), previous information may be
> retained if it was longer than than the new hostname/domainname.
> 
> This can be demonstrated trivially by calling sethostname() first
> with a long name, then with a short name, and then calling uname()
> to retrieve the full buffer that contains the hostname (and
> possibly parts of the old hostname), one just has to look past the
> terminating zero.
> 
> I don't know if we should really care that much (hence the RFC);
> the only scenarios I can possibly think of is administrator
> putting something sensitive in the hostname (or domain name) by
> accident, and changing it back will not undo the mistake entirely,
> though it's not like we can recover gracefully from "rm -rf /"
> either... The other scenario is namespaces (CLONE_NEWUTS) where
> some information may be unintentionally "inherited" from the
> previous namespace (a program wants to hide the original name and
> does clone + sethostname, but some information is still left).
> 
> I think the patch may be defended on grounds of the principle of
> least surprise. But I am not adamant :-)
> 
> (I guess the question now is whether userspace should be able to
> write embedded NULs into the buffer or not...)
> 
> At least the observation has been made and the patch has been
> presented.
> 

fair enuf.  Did you check allthe other fields in 'struct new_utsname'?

> index 038a7bc..78b4515 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1352,7 +1352,8 @@ asmlinkage long sys_sethostname(char __user *name, int len)
>  	errno = -EFAULT;
>  	if (!copy_from_user(tmp, name, len)) {
>  		memcpy(utsname()->nodename, tmp, len);
> -		utsname()->nodename[len] = 0;
> +		memset(utsname()->nodename + len, 0,
> +			sizeof(utsname()->nodename) - len);

We could do the memset before the memcpy.  It's more work, but less
text.  Whatever.


While we're there, the code generation in there is a bit sloppy.  How's
this look?


From: Andrew Morton <akpm@linux-foundation.org>

utsname() is quite expensive to calculate.  Cache it in a local.

          text    data     bss     dec     hex filename
before:  11136     720      16   11872    2e60 kernel/sys.o
after:   11096     720      16   11832    2e38 kernel/sys.o

Cc: Vegard Nossum <vegard.nossum@gmail.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: "Serge E. Hallyn" <serue@us.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/sys.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff -puN kernel/sys.c~kernel-sysc-improve-code-generation kernel/sys.c
--- a/kernel/sys.c~kernel-sysc-improve-code-generation
+++ a/kernel/sys.c
@@ -1415,9 +1415,10 @@ asmlinkage long sys_sethostname(char __u
 	down_write(&uts_sem);
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
-		memcpy(utsname()->nodename, tmp, len);
-		memset(utsname()->nodename + len, 0,
-			sizeof(utsname()->nodename) - len);
+		struct new_utsname *u = utsname();
+
+		memcpy(u->nodename, tmp, len);
+		memset(u->nodename + len, 0, sizeof(u->nodename) - len);
 		errno = 0;
 	}
 	up_write(&uts_sem);
@@ -1429,15 +1430,17 @@ asmlinkage long sys_sethostname(char __u
 asmlinkage long sys_gethostname(char __user *name, int len)
 {
 	int i, errno;
+	struct new_utsname *u;
 
 	if (len < 0)
 		return -EINVAL;
 	down_read(&uts_sem);
-	i = 1 + strlen(utsname()->nodename);
+	u = utsname();
+	i = 1 + strlen(u->nodename);
 	if (i > len)
 		i = len;
 	errno = 0;
-	if (copy_to_user(name, utsname()->nodename, i))
+	if (copy_to_user(name, u->nodename, i))
 		errno = -EFAULT;
 	up_read(&uts_sem);
 	return errno;
@@ -1462,9 +1465,10 @@ asmlinkage long sys_setdomainname(char _
 	down_write(&uts_sem);
 	errno = -EFAULT;
 	if (!copy_from_user(tmp, name, len)) {
-		memcpy(utsname()->domainname, tmp, len);
-		memset(utsname()->domainname + len, 0,
-			sizeof(utsname()->domainname) - len);
+		struct new_utsname *u = utsname();
+
+		memcpy(u->domainname, tmp, len);
+		memset(u->domainname + len, 0, sizeof(u->domainname) - len);
 		errno = 0;
 	}
 	up_write(&uts_sem);
_

Re: [RFC][PATCH] utsname: completely overwrite prior information

[Vegard Nossum]

On Sat, Sep 13, 2008 at 12:11 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> fair enuf.  Did you check allthe other fields in 'struct new_utsname'?

As far as I can tell, only nodename (hostname) and domainname may be
changed by userspace.

>
>> index 038a7bc..78b4515 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -1352,7 +1352,8 @@ asmlinkage long sys_sethostname(char __user *name, int len)
>>       errno = -EFAULT;
>>       if (!copy_from_user(tmp, name, len)) {
>>               memcpy(utsname()->nodename, tmp, len);
>> -             utsname()->nodename[len] = 0;
>> +             memset(utsname()->nodename + len, 0,
>> +                     sizeof(utsname()->nodename) - len);
>
> We could do the memset before the memcpy.  It's more work, but less
> text.  Whatever.

Whatever :-)

> While we're there, the code generation in there is a bit sloppy.  How's
> this look?
>
>
> From: Andrew Morton <akpm@linux-foundation.org>
>
> utsname() is quite expensive to calculate.  Cache it in a local.
>
>          text    data     bss     dec     hex filename
> before:  11136     720      16   11872    2e60 kernel/sys.o
> after:   11096     720      16   11832    2e38 kernel/sys.o
>
> Cc: Vegard Nossum <vegard.nossum@gmail.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: "Serge E. Hallyn" <serue@us.ibm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

I agree with this change. FWIW: Acked-by: Vegard Nossum
<vegard.nossum@gmail.com>

There seems to be a few more places throughout the kernel which
needlessly call utsname() more than once. I will keep it in mind.

Thanks,


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036