[patch] Make NSTemporaryDirectory more secure

[Alexander Malmberg]

Hi,

This patch should fix some of the security issues raised in recent
discussions of this function:

* It makes NSTemporaryDirectory raise an exception if a suitable
directory can't be found/created (and documents this, of course :).

* In addition to checking that the permissions are 0600 or 0700, it
checks that we are the owner (ie. that the owner matches our euid).
Otherwise, the real owner would be able to read our temporary files (the
writable check below doesn't guard against this since the other user
might change the permissions between the checks).

* If we decide to use a secure subdirectory, the permission/owner check
is repeated for it before accepting it.

I think the only questionable part is using geteuid unconditionally. It
would be easy to copy the code from eg. NSUserName, but I don't think
that'd help on windows. Thoughts?

- Alexander Malmberg

Index: NSUser.m
===================================================================
RCS file: /cvsroot/gnustep/gnustep/core/base/Source/NSUser.m,v
retrieving revision 1.85
diff -u -r1.85 NSUser.m
--- NSUser.m    24 Feb 2004 03:57:41 -0000      1.85
+++ NSUser.m    15 Mar 2004 12:00:41 -0000
@@ -772,6 +772,9 @@
 /**
  * Returns the name of a directory in which temporary files can be stored.
  * Under GNUstep this is a location which is not readable by other users.
+ * <br />
+ * If a suitable directory can't be found or created, this function raises an
+ * NSGenericException.
  */
 NSString *
 NSTemporaryDirectory(void)
@@ -780,7 +783,7 @@
   NSString     *tempDirName;
   NSString     *baseTempDirName = nil;
   NSDictionary *attr;
-  int          perm;
+  int          perm, owner;
   BOOL         flag;
 #if    defined(__WIN32__)
   char buffer[1024];
@@ -828,19 +831,22 @@
   if ([manager fileExistsAtPath: tempDirName isDirectory: &flag] == NO
     || flag == NO)
     {
-      NSLog(@"Temporary directory (%@) does not seem to exist", tempDirName);
-      return nil;
+      [NSException raise: NSGenericException
+                 format: @"Temporary directory (%@) does not exist",
+                         tempDirName];
+      return nil; /* Not reached. */
     }
 
   /*
-   * Check that the directory owner (presumably us) has access to it,
-   * and nobody else.  If other people have access, try to create a
-   * secure subdirectory.
+   * Check that we are the directory owner, and that we, and nobody else,
+   * have access to it. If other people have access, try to create a secure
+   * subdirectory.
    */
   attr = [manager fileAttributesAtPath: tempDirName traverseLink: YES];
+  owner = [[attr objectForKey: NSFileOwnerAccountID] intValue];
   perm = [[attr objectForKey: NSFilePosixPermissions] intValue];
   perm = perm & 0777;
-  if (perm != 0700 && perm != 0600)
+  if ((perm != 0700 && perm != 0600) || owner != geteuid())
     {
       /*
       NSLog(@"Temporary directory (%@) may be insecure ... attempting to "
@@ -858,16 +864,35 @@
          if ([manager createDirectoryAtPath: tempDirName
                                  attributes: attr] == NO)
            {
-             tempDirName = baseTempDirName;
-             NSLog(@"Temporary directory (%@) may be insecure", tempDirName);
+             [NSException raise: NSGenericException
+                         format: @"Attempt to create a secure temporary 
directory (%@) failed.",
+                                 tempDirName];
+             return nil; /* Not reached. */
            }
        }
+
+      /*
+       * Check that the new directory is really secure.
+       */
+      attr = [manager fileAttributesAtPath: tempDirName traverseLink: YES];
+      owner = [[attr objectForKey: NSFileOwnerAccountID] intValue];
+      perm = [[attr objectForKey: NSFilePosixPermissions] intValue];
+      perm = perm & 0777;
+      if ((perm != 0700 && perm != 0600) || owner != geteuid())
+       {
+         [NSException raise: NSGenericException
+                     format: @"Attempt to create a secure temporary directory 
(%@) failed.",
+                             tempDirName];
+         return nil; /* Not reached. */
+       }
     }
 
   if ([manager isWritableFileAtPath: tempDirName] == NO)
     {
-      NSLog(@"Temporary directory (%@) is not writable", tempDirName);
-      return nil;
+      [NSException raise: NSGenericException
+                 format: @"Temporary directory (%@) is not writable",
+                         tempDirName];
+      return nil; /* Not reached. */
     }
   return tempDirName;
 }