Linux Sandbox: handle O_CREAT properly in broker process.

sandbox/linux/services/broker_process.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 "sandbox/linux/services/broker_process.h"
 
 #include <fcntl.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
@@ skipping 49 matching lines @@
 // we're ok to allow in the broker.
 // I.e. here is where we wouldn't add O_RESET_FILE_SYSTEM.
 bool IsAllowedOpenFlags(int flags) {
   // First, check the access mode
   const int access_mode = flags & O_ACCMODE;
   if (access_mode != O_RDONLY && access_mode != O_WRONLY &&
       access_mode != O_RDWR) {
     return false;
   }
 
+  // We only support a 2-parameters open, so we forbid O_CREAT.

[Markus (顧孟勤), 2013/01/09 05:38:47]

We might have to support O_CREAT at some point. But I am fine with adding that
code when it is actually needed.

The way your CL looks like right now, that definitely keeps things simpler.

+  if (flags & O_CREAT) {
+    return false;
+  }
+
   // Some flags affect the behavior of the current process. We don't support
   // them and don't allow them for now.
   if (flags & ForCurrentProcessFlagsMask()) {
     return false;
   }
 
   // Now check that all the flags are known to us.
   const int creation_and_status_flags = flags & ~O_ACCMODE;
 
   const int known_flags =
     O_APPEND | O_ASYNC | O_CLOEXEC | O_CREAT | O_DIRECT |

[Markus (顧孟勤), 2013/01/09 05:38:47]

I still feel that this code is confusing and hard to read. Listing O_CREAT in
this line makes it look to me, as if we expect to encounter O_CREAT at this
point.

But we had that argument before, and it's outside of the scope of this CL. So,
just FYI.

     O_DIRECTORY | O_EXCL | O_LARGEFILE | O_NOATIME | O_NOCTTY |
     O_NOFOLLOW | O_NONBLOCK | O_NDELAY | O_SYNC | O_TRUNC;
 
   const int unknown_flags = ~known_flags;
   const bool has_unknown_flags = creation_and_status_flags & unknown_flags;
   return !has_unknown_flags;
 }
 
 }  // namespace
 
@@ skipping 190 matching lines @@
 
   const char* file_to_open = NULL;
   const bool safe_to_open_file = GetFileNameIfAllowedAccess(
       requested_filename.c_str(), flags, &file_to_open);
 
   if (safe_to_open_file) {
     CHECK(file_to_open);
     // O_CLOEXEC doesn't hurt (even though we won't execve()), and this
     // property won't be passed to the client.
     // We may want to think about O_NONBLOCK as well.
-    int opened_fd = open(file_to_open, flags | O_CLOEXEC);
+    // We're doing a 2-parameter open, so we don't support O_CREAT. It doesn't
+    // hurt to always pass a third argument though.
+    int opened_fd = open(file_to_open, flags | O_CLOEXEC, 0);
     if (opened_fd < 0) {
       write_pickle.WriteInt(-errno);
     } else {
       // Success.
       opened_files.push_back(opened_fd);
       write_pickle.WriteInt(0);
     }
   } else {
     write_pickle.WriteInt(-EPERM);
   }
@@ skipping 40 matching lines @@
           GetFileNameInWhitelist(allowed_w_files_, requested_filename,
                                  file_to_open);
       return allowed_for_read_and_write;
     }
     default:
       return false;
   }
 }
 
 }  // namespace sandbox.