Posix: fix named SHM mappings permissions.

base/memory/shared_memory_posix.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/memory/shared_memory.h"
 
 #include <errno.h>
 #include <fcntl.h>
+#include <fcntl.h>

[Mark Mentovai, 2013/07/02 14:31:54]

Really? Again?

[jln (very slow on Chromium), 2013/07/02 18:49:08]

Oops! I'll update the presubmit script later to detect these.

 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <sys/stat.h>

[Mark Mentovai, 2013/07/02 14:31:54]

REALLY?

[jln (very slow on Chromium), 2013/07/02 18:49:08]

Done.

+#include <sys/types.h>
 #include <unistd.h>
 
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/process_util.h"
 #include "base/safe_strerror_posix.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
@@ skipping 121 matching lines @@
     // private), and prevents the need for cleanup (once the last fd is closed,
     // it is truly freed).
     if (fp) {
       if (unlink(path.value().c_str()))
         PLOG(WARNING) << "unlink";
     }
   } else {
     if (!FilePathForMemoryName(*options.name, &path))
       return false;
 
-    fp = file_util::OpenFile(path, "w+x");
-    if (fp == NULL && options.open_existing) {
-      // "w+" will truncate if it already exists.
-      fp = file_util::OpenFile(path, "a+");
+    // Make sure that we don't give permissions to access this file

[Mark Mentovai, 2013/07/02 14:31:54]

Nit: permission.
Nit: avoid “we” (only picking on this because I have something else to pick on
on this line).

[jln (very slow on Chromium), 2013/07/02 18:49:08]

Done.

+    // to other users on the system.
+    const mode_t owner_only = S_IRUSR | S_IWUSR;
+    int fd;
+    // First, try to create the file.
+    fd = open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, owner_only);

[Mark Mentovai, 2013/07/02 14:31:54]

You can declare fd on this line.

[Mark Mentovai, 2013/07/02 14:31:54]

HANDLE_EINTR. Same around the open on line 170.

[jln (very slow on Chromium), 2013/07/02 18:49:08]

Done.

[jln (very slow on Chromium), 2013/07/02 18:49:08]

Done.

+    if (fd == -1 && options.open_existing) {
+      // If this doesn't work, try and open an existing file in append mode.
+      // Opening an existing file in a world writable directory has two main
+      // security implications:
+      // - Attackers could plant a file under their control.
+      // - Attackers could plant a symbolic link so that an unexpected file
+      //   is opened.
+      // O_NOFOLLOW makes sure that the latter doesn't happen.
+      // Checking the former happens below.

[Mark Mentovai, 2013/07/02 14:31:54]

Former? Below? Ick.

Why have you separated the problems from how they’re handled? “Attackers could
plant a symbolic link…so use O_NOFOLLOW. Attackers could plant a file…so that’s
checked below.”

+      fd = open(path.value().c_str(), O_RDWR | O_APPEND | O_NOFOLLOW);
+      struct stat stat;

[Markus (顧孟勤), 2013/07/02 02:49:38]

Ideally, avoid using "stat" as a local symbol, as it shadows "stat(2)". That's
just confusing.

I usually use "struct stat sb" (for "stat buf"). I have seen this use in other
projects, but obviously there are other -- more verbose -- choices that work
just as well.

[jln (very slow on Chromium), 2013/07/02 02:51:38]

Yeah, I only used it for consistency with the code below. I agree that it's not
a great choice.

[Mark Mentovai, 2013/07/02 14:31:54]

Pick a different name.

[jln (very slow on Chromium), 2013/07/02 18:49:08]

Done.

+      // Check that the current user owns the file.
+      if (fd >= 0 &&
+          (fstat(fd, &stat) != 0 || stat.st_uid != getuid())) {

[Mark Mentovai, 2013/07/02 14:31:54]

getuid or geteuid?

[jln (very slow on Chromium), 2013/07/02 18:49:08]

I had thought about it and I believe it's slightly more correct to use the uid.

The euid can be changed to get more (setuid binary) or less (root program)
privileges in the fsuid (essentialy the euid), but this check really is about
who the current user is.

This being said, if fsuid != uid, this API should probably be rethought and not
used. I'm happy to add a CHECK_EQ(getuid(), geteuid()).

Let me know what you think.

[Mark Mentovai, 2013/07/02 19:52:56]

Julien Tinnes wrote:
I think that this should match the user that would show up as the owner of the
file if it had been created by the open call above. Isn’t the fsuid normally the
euid? Since you’re assuming that a file created by open with O_CREAT is OK,
don’t you want to assure yourself of the same properties if you’re recycling an
existing file?

[jln (very slow on Chromium), 2013/07/02 21:20:02]

The O_CREAT is O_CREAT | O_EXCL: i.e. we guaranteed that we're not opening an
existing file.

If you think about this code being used, for instance, in a setuid root binary -
a canonical case of euid != uid -, one definitely would want to check for the
file matching the uid, not the euid. It guarantees security insofar as we're not
opening a "privileged" file.

Another way to think of this: the uid check works if multiple users are using
this same setuid binary concurrently, but a euid check wouldn't.

I think the root cause of confusion is that opening an existing file in a shared
temp directory is an anti-pattern. It definitely shouldn't be attempted when uid
!= euid. So I'm now checking for uid != euid as well, let me know if this works.

[Mark Mentovai, 2013/07/02 21:27:59]

Julien Tinnes wrote:
Why is “root” the only example? What mark ran something that was setuid jln?
Should the program have access to mark’s files or jln’s in this case? Should it
create files as mark or jln? Should mark or jln be able to read its memory?
That’s fine.

[jln (very slow on Chromium), 2013/07/02 21:44:52]

Yes, I think my example still works. If there is a setuid mark program
available, mark made it available for other users, such as jln to perform some
actions with more privileges. Let's say append lines to
/home/mark/list_of_reviews_to_do.

The setuid program's resources should still be private, to be able to guarantee
its integrity. That's why the OS won't let jln debug that setuid binary or even
read its memory. So if the setuid program is creating a new shared memory
resource, it should be create with mark's uid.
But if the setuid program needs to open an existing shared memory resource the
safe default is to assume that it doesn't want to use it's additional ambiant
authority (as mark) to do so: jln is running the program, it's ok to read jln's
X shared memory to access the clipboard, but not mark's.

So I think "real uid" is a slightly better default. In any case, I think we both
agree that uid != euid is a subtle security model that would require a different
API.

[Mark Mentovai, 2013/07/02 22:05:27]

Julien Tinnes wrote:
You’re assuming that the program is supposed to append to
/home/mark/list_of_reviews_to_do and not…well, open some of mark’s shared
memory. Neither of these two options seems any more likely than the other.

I don’t see why should the security model should be different if you created the
resource as opposed to if you accessed a resource that already existed. If the
resource doesn’t exist already, a setuid mark program that creates it gives mark
more access than he would have had if it already existed, and forbids jln that
same access.

Regardless, this is all moot now, it’s fine with you guaranteeing that the euid
and real UID are identical.

[jln (very slow on Chromium), 2013/07/02 22:57:05]

Because if creating a new resource mark is not put at any risk. The setuid
program has been made available by mark to give jln a bit more privileges. It's
normal that the security model is skewed in Mark's favor by default.

From mark's (who made the setuid binary available) point of view:
- If creating a new file: integrity is not an issue (there is nothing to trash),
by using the euid when creating the file we're guaranteeing confidentiality.
- If opening an existing file:
a. checking euid risks violating integrity and confidentiality (jln who runs the
program can end-up writing to that file or reading from that file.
b. checking uid risks violating confidentiality (write private stuff to a
jln-owned file)

In addition, and it's perhaps a better argument, the euid is kind of a hack on
top of an existing POSIX API. It's basically: let's change this internal uid
that the kernel will actually use and all the existing APIs can still be used.
So it's not typical to check for euid.
Most checks for euid in open source software are actually wrong, they should
almost always be replaced by checks for whether or not a resource is accessible
(on Linux they break the new file capabilities model for instance).
Yes, it's much better that way. This long discussion proves it ;)

+        HANDLE_EINTR(close(fd));
+        return false;
+      }
+      // An existing file was opened so its size should not be fixed.
       fix_size = false;
     }
+    fp = NULL;
+    if (fd >= 0) {
+      // "a+" is always appropriate: if it's a new file, a+ is similar to w+.
+      fp = fdopen(fd, "a+");
+    }
   }
   if (fp && fix_size) {
     // Get current size.
     struct stat stat;
     if (fstat(fileno(fp), &stat) != 0) {
       file_util::CloseFile(fp);
       return false;
     }
     const size_t current_size = stat.st_size;
     if (current_size != options.size) {
@@ skipping 211 matching lines @@
   new_handle->fd = new_fd;
   new_handle->auto_close = true;
 
   if (close_self)
     Close();
 
   return true;
 }
 
 }  // namespace base