Bug 57486 (CVE-2004-0689)

<!-- SBZ_reproduce  -->
-

During login lnusertemp is supposed to catch these situations. 
 
For example, if I chown /tmp/kde-bastian to root I will get: 
 
> lnusertemp tmp 
Link points to "/tmp/kde-bastian" 
Error: "/tmp/kde-bastian" is owned by uid 0 instead of uid 500. 
Created link from "/home/bastian/.kde/tmp-linux" to "/tmp/kde-bastianquXkhT" 
 
lnusertemp is responsible for the "kde-<user>", "kdecache-<user>" and 
"ksocket-<user>" links. 
 
mcop seems to be handled differently though. 
 
 

There might be room for failure in the following two cases: 
 
* User doesn't use KDE but other window manager and starts KDE application 
* User runs KDE application as root without being logged in as root. 

Created attachment 21703 [details]
patch

Attached patch should solve the two issues I mentioned in #3. Coolo, please
review this patch.

The code that creates the mcop link already does santity checks, see 
arts/mcop/mcoputils.cpp: MCOPUtils::createFilePath

arts will fail if someone has messed with the mcop directory.

some comments might be useful here :) 
 
From what I can tell the patch looks correct, but I'm sure the security guys will some 
nitpicks. 

Judging from the code snippet there is still a race condition in it (a gap 
between point-of-check and point-of-use). 
But as long as we operate on a +t directory like /tmp this will not harm us. 
 
 

I have reviewd function/method 'createFilePath(string name)' as it exists in 
the 9.0 arts code. 
 
string MCOPUtils::createFilePath(string name) 
{ 
// changed to get logname from uid instead of environment variable 
// LOGNAME to avoid trouble when LOGNAME does not exist or 
// is wrong (such as in kdesu environment) 
// S. Voitzsch Sebastian.Voitzsch@web.de 
// 2002-12-03 
        struct passwd *pwd = NULL; 
        // XXX why is get_e_uid() used here, access control is done 
        // with getuid() later!!! 
        pwd = getpwuid(geteuid()); 
        if (pwd == NULL) 
                arts_fatal("could not get user name from user id"); 
 
        string logname = pwd->pw_name; 
 
        string tmpdir = "/tmp/mcop-"+uglify(logname); 
        // XXX the direc is allowed to exist 
        if(mkdir(tmpdir.c_str(),0700) != 0 && errno != EEXIST) 
                arts_fatal("can't create %s (%s)", 
tmpdir.c_str(),strerror(errno)); 
 
        /** check that /tmp/mcop-<username>/ is a secure temporary dir **/ 
        struct stat st; 
        if(lstat(tmpdir.c_str(),&st) != 0) 
                arts_fatal("can't stat %s (%s)", 
tmpdir.c_str(),strerror(errno)); 
 
        if (st.st_uid != getuid ()) 
                arts_fatal("%s is not owned by user", tmpdir.c_str()); 
 
        if(st.st_mode & 0077) 
                arts_fatal("%s is accessible owned by user", tmpdir.c_str()); 
 
        if(!S_ISDIR(st.st_mode)) 
                arts_fatal("%s is not a directory", tmpdir.c_str()); 
 
        // XXX the checks above allows links to user owned, mode 7700 
directories 
        // XXX we can pass this check by linking to an user owned direc and 
then 
        // remove the link afterwards 
 
        string::iterator si; 
        for(si = name.begin(); si != name.end(); si++) 
        { 
                unsigned char c = (unsigned char)*si; 
                if((c >= 'a' && c <= 'z') || 
                   (c >= 'A' && c <= 'Z') || 
                   (c >= '0' && c <= '9') || 
                   (c == '-')) 
                { 
                        // ok, these are fine for our filesystem ;) 
                } 
                else 
                { 
                        *si = '_'; 
                } 
        } 
        return tmpdir+"/"+name; // XXX at this point we may end up in a direc 
        // created by a local attacker with 'name' as a symbolic link 
} 

... my comments in the above pasted code snippet are prefixed by 'XXX' 

lstat will not follow symlinks, which invalidates your comments I think. 

arg, yes I missed it... too bad. Thanks Marcus. 

Waldo can you build some update for testing please. 
 
Which packages are affected? kdelibs3 only? 

Created attachment 22205 [details]
patchinfo-box.kdelibs3

Created attachment 22206 [details]
patchinfo.kdelibs3

Does the above usage of geteuid() instead of getuid() can cause problems when 
running a setuid KDE application? 
 
imagine the following scenario: 
	+ /tmp/mcop-root doesnt exist 
	+ user creates /tmp/mcop-root 
	+ user execs a setuid KDE app 
	+ createFilePath() uses geteuid() to determine the direc. 
	name, but getuid() to verify access permissions 
	+ the setuid KDE app will end up in a user-controled direc 
	which should only be accessible by root 

Re #11: No, I don't make packages, but maybe Adrian will. 
 
Re #14: setuid KDE apps are a very bad idea and we try hard not to allow that 
(e.g. by aborting when we detect geteuid != getuid)so I'm not sure if there 
are vulnerable applications around (and if there are, there sound would not be 
working because in the normal case because this access check would fail then), 
but I agree that for consistency geteuid() should be used to verify access 
permissions. 

Created attachment 22209 [details]
arts patch

Use same uid for obtaining name and doing owner check

where and when does this euid!=uid detection happen? 
 
concerning the patch I would suggest to use "pid_t uid" instead of "int uid" 
to avoid casting problems. 
 

Created attachment 22219 [details]
arts patch #2

New patch that uses uid_t instead of int (or pid_t ;-)

KApplication::init checks for uid != euid (kdelibs/kdecore/kapplication.cpp)

Hello Waldo, 
can you make a patch for 9.0 too please. The reporter of this bug likes to 
test it. I built 9.1 packages for kdelibs3 and arts but he uses 9.0. 
You patch does no apply to 9.0 kdelibs3. The code looks much different. 

the patch is for the arts package, not kdelibs 

Created attachment 22317 [details]
kdelibs patch for 9.0

Patch for SL9.0 (kdelibs, backport of 13703)

Thanks a lot. 

I did forward the packages to the reporter of the bug for testing...  

Date: Thu, 22 Jul 2004 21:41:41 -0400 
From: Andrew Tuitt <andrew@tuitt.net> 
To: Thomas Biege <thomas@suse.de> 
Subject: Re: [security@suse.de] kde /tmp and /var/tmp DoS and possible file 
overwriting question 
 
Thomas Biege wrote: 
> hi, 
> all 9.0 packages are available. 
> 
> http://www.suse.de/~thomas/RPM/9.0/ 
> 
> Please let me know if the patches work. 
> 
> Bye, 
>      Thomas 
 
I tested the patch and it looks like it fixed it!  It now appears to use a 
randomly named temp folder if its normal temp folder 
is owned by another user and nothing gets overwritten when I put a bunch of 
symlinks in place of files that it normally creates 
in the old temp folder. 

Ok, let's do an update. 
 
I'll propose 11th of august as release date. Is this time enough for 
backporting? (don't forget QA-testing) 

Created attachment 22360 [details]
patchinfo-box.kdelibs3 (new)

Created attachment 22361 [details]
patchinfo.kdelibs3 (new)

Created attachment 22362 [details]
patchinfo-box.arts

Created attachment 22363 [details]
patchinfo.arts

package building / backporting is in progress. 
 
wrt your patchinfo texts: The standard KDE user is not affected as Waldo 
mentioned. Only people who start the first time a KDE application outside of 
their KDE session, after the /tmp/ directory gets cleaned. 
 

Date: Fri, 23 Jul 2004 08:36:34 +0100 (BST) 
From: Mark J Cox <mjc@redhat.com> 
To: Thomas Biege <thomas@suse.de> 
Cc: vendor-sec@lst.de 
Subject: Re: [vendor-sec] [security@suse.de] kde /tmp and /var/tmp DoS and 
possible file overwriting question (fwd) 
 
> What about 11th of August 16:00 MEST as release date? 
 
Fine with us, use CAN-2004-0689 for this. 
 
Mark 
 

packages got checked in. 
 
I leave it open for tracking. 

thx 

Created attachment 22566 [details]
patchinfo.kdelibs3-4vulns

Created attachment 22567 [details]
patchinfo-box.kdelibs3-4vulns

Created attachment 22575 [details]
patchinfo.kdelibs3-3vulns

Created attachment 22576 [details]
patchinfo-box.kdelibs3-3vulns

packages approved 

CVE-2004-0689: CVSS v2 Base Score: 4.6 (AV:L/AC:L/Au:N/C:P/I:P/A:P)