Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11557025:
Linux sandbox: add a new low-level broker process mechanism. (Closed)
|
Created:
8 years ago by jln (very slow on Chromium) Modified:
8 years ago CC:
chromium-reviews, agl, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux sandbox: add a new low-level broker process mechanism.
We add a new low-level broker process mechanism that can be
async signal safe and is suitable for use in the seccomp-bpf sandbox.
Also fix UnixDomainSocket::SendMsg() to never generate a SIGPIPE.
BUG=165837
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173064
Patch Set 1 #
Total comments: 13
Patch Set 2 : Overcome C++03 limitation by using a default parameter. #Patch Set 3 : No SIGPIPE in sendmsg. #Patch Set 4 : Address a few nits pointed out by Jorge. #Patch Set 5 : Move implementation constants out of interface. #
Total comments: 7
Patch Set 6 : Add a BPF test that makes use of the broker process. #Patch Set 7 : Address Jorge's comments. #
Total comments: 22
Patch Set 8 : Address most of Markus' comments. #Patch Set 9 : Support openat() properly in test. #Patch Set 10 : Support both RO and RW files in constructors. RW is still unsupported. #Patch Set 11 : Add support for more flags and writing. #Patch Set 12 : Add file writing test. #
Total comments: 22
Patch Set 13 : Address comments from Jorge. #Patch Set 14 : Better support for flags. #
Total comments: 12
Patch Set 15 : More centralized flags checking. #Patch Set 16 : Add copyright notice #
Messages
Total messages: 31 (0 generated)
This CL is not final yet, I need to add support for the sandboxing call back, but I really would like to land it tomorrow. A first look would be appreciated. I'll send another CL to make use of this in the BPF sandbox.
Some high-level comments. Will take another look in the shuttle. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:22: // string for the white list. from? https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:23: // Even if an attacker manager to fool the string comparison mechanism, we managed? https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:132: LOG(ERROR) << "Error parsing Open request"; This should be a generic error right? It lives outside of the case: clause. E.g. "Error parsing request" https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:232: // Send a request (in write_pickle) as well as a new temporary socketpair. Maybe clarify that SendRecvMsg takes care of creating the temp socketpair? It was very confusing at first. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.h (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.h:40: // The implementation only supports O_RDONLY for now. I need to check if the /dev/dri/card0 thing libva was opening was being opened RDONLY. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_unittest.cc:73: // Compare the broker's cpuinfo fd with the real one. This comment is confusing as you are comparing the contents of the file in both cases, right?
Thanks! PTAL. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:22: // string for the white list. On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > from? Done. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:23: // Even if an attacker manager to fool the string comparison mechanism, we On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > managed? Done. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:132: LOG(ERROR) << "Error parsing Open request"; On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > This should be a generic error right? It lives outside of the case: clause. E.g. > "Error parsing request" Good point, done. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:232: // Send a request (in write_pickle) as well as a new temporary socketpair. On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > Maybe clarify that SendRecvMsg takes care of creating the temp socketpair? It > was very confusing at first. Done. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.h (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.h:40: // The implementation only supports O_RDONLY for now. On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > I need to check if the /dev/dri/card0 thing libva was opening was being opened > RDONLY. Ok. It's trivial to add support for RD_WRITE anyway. Let me know if I should do it right now. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_unittest.cc:73: // Compare the broker's cpuinfo fd with the real one. On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > This comment is confusing as you are comparing the contents of the file in both > cases, right? Done.
I think you should add this in the first take. No use to land something that will not actually allow closing the GPU hole. https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.h (right): https://chromiumcodereview.appspot.com/11557025/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.h:40: // The implementation only supports O_RDONLY for now. On 2012/12/13 02:33:14, Julien Tinnes wrote: > On 2012/12/13 02:14:54, Jorge Lucangeli Obes wrote: > > I need to check if the /dev/dri/card0 thing libva was opening was being opened > > RDONLY. > > Ok. It's trivial to add support for RD_WRITE anyway. Let me know if I should do > it right now. Confirmed libva is opening /dev/dri/card0 O_RDWR.
This looks good. I'd rather LGTM tomorrow after running the unit tests on ARM. https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.cc:110: PLOG(ERROR) << "Error reading message from the parent."; You don't seem to be using periods in other comment lines. https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.cc:225: // temporary socketpair (created internally by SendRecvMsg(). Missing closing ')'. https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.cc:235: //LOG(ERROR) << "Could not make request to broker process"; Is this supposed to be commented? https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... File sandbox/linux/services/broker_process.h (right): https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.h:35: // Can be used in lieu of open(). Will be async signal safe. Nit: In place?
On 2012/12/13 04:46:33, Jorge Lucangeli Obes wrote: > This looks good. I'd rather LGTM tomorrow after running the unit tests on ARM. > D'oh I always forget about that =P. I'll try the unit tests tomorrow. > https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... > File sandbox/linux/services/broker_process.cc (right): > > https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... > sandbox/linux/services/broker_process.cc:110: PLOG(ERROR) << "Error reading > message from the parent."; > You don't seem to be using periods in other comment lines. > > https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... > sandbox/linux/services/broker_process.cc:225: // temporary socketpair (created > internally by SendRecvMsg(). > Missing closing ')'. > > https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... > sandbox/linux/services/broker_process.cc:235: //LOG(ERROR) << "Could not make > request to broker process"; > Is this supposed to be commented? > > https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... > File sandbox/linux/services/broker_process.h (right): > > https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... > sandbox/linux/services/broker_process.h:35: // Can be used in lieu of open(). > Will be async signal safe. > Nit: In place?
Thanks. PTAL, I've added new tests and a few tweaks. There is now a test that specifically uses the broker in a signal handler. https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.cc:110: PLOG(ERROR) << "Error reading message from the parent."; On 2012/12/13 04:46:33, Jorge Lucangeli Obes wrote: > You don't seem to be using periods in other comment lines. Done. https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.cc:225: // temporary socketpair (created internally by SendRecvMsg(). On 2012/12/13 04:46:33, Jorge Lucangeli Obes wrote: > Missing closing ')'. Done. https://chromiumcodereview.appspot.com/11557025/diff/8/sandbox/linux/services... sandbox/linux/services/broker_process.cc:235: //LOG(ERROR) << "Could not make request to broker process"; On 2012/12/13 04:46:33, Jorge Lucangeli Obes wrote: > Is this supposed to be commented? Done.
https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:510: void *aux) { I would probably have moved the TrapHandler inside of the class, but that's really just a question of personal preference. If you move it inside of the class, it would still need to be static, but you could make "aux" be a pointer to "this". https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:518: InitializedOpenBroker* iob = static_cast<InitializedOpenBroker*>(aux); I probably also would have moved the body of this method into the class. That can potentially make some of the code a little cleaner. See my pending CL for argument inspection for an example of how I envisioned the use of classes in BPF_TESTs. But as mentioned earlier, it really turns out to be a personal preference. If you think of the class as mere container, then what you are doing makes sense. If you think of the class as the object that is being tested, then it makes more sense to move as much as possible into the class. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:529: iob->broker_process())); Indentation is a little funny, I think https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:535: // We use a InitializedOpenBroker class since, so that we can run unsandboxed s/ since// https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:538: class InitializedOpenBroker /* BPF_AUX */) { The "class" is superfluous here, and I don't think our code typically adds it. But feel free to correct me if the style guide says otherwise. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:30: std::string* file_to_open) { Do you want to set "file_to_open" to NULL before you do anything else? https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:100: sizeof(buf), &fds); What does RecvMsg() do for truncated messages? They can certainly confuse things quite a bit and we probably will completely fail to parse subsequent data correctly, when that happens. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:109: if (msg_len < 0 || fds.size() != 1 || fds.at(0) < 0) { Instead of saying fds.at(0) all over the place, I would probably assign this value to a suitably named local variable. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:165: if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_pair)) { You need to know about package boundaries don't you? That probably means you want to use SOCK_DGRAM. This currently works, because the kernel cannot coalesce messages that contain ancillary data. But that sounds really fragile. You shouldn't rely on this very subtle feature. I know you currently have no intention to do so, but if you ever added a new IPC that didn't require ancillary data (e.g. because it is a "void" function), your code would break in a rather hard-to-diagnose fashion -- and only when under load from multiple threads. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:181: shutdown(socket_pair[0], SHUT_RD); While it is somewhat arbitrary, your choice of indices is confusing. It would be little more obvious if you followed the convention that pipe() uses. Index "0" is used for reading, index "1" is used for writing. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:183: initialized_ = true; Why do we set initialized_ to true here? We later fall through to code that does that again. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:214: flags != O_RDONLY) { Can you add some comments that the handling of flags is overly simplified. For example, it would be perfectly legitimate and safe for somebody to set the O_NONBLOCK or O_CLOEXEC flag in addition to O_RDONLY. But these are flags that must be handled in the caller and cannot be handled by the broker process. So, we have to mask them prior to passing the flags to the broker. And then we later have to do the right thing to make the flags effective. See SOCK_NONBLOCK and SOCK_CLOEXEC for how to do that.
There's one of the errno checks in UseOpenBroker that's failing mysteriously on daisy. I'm debugging now. Everything else passes.
There's one of the errno checks in UseOpenBroker that's failing mysteriously on daisy. I'm debugging now. Everything else passes.
I've addressed most of your comments. I'll handle O_RDWR and other flags in a next iteration. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:529: iob->broker_process())); On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > Indentation is a little funny, I think Done. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:535: // We use a InitializedOpenBroker class since, so that we can run unsandboxed On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > s/ since// Done. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/secc... sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:538: class InitializedOpenBroker /* BPF_AUX */) { On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > The "class" is superfluous here, and I don't think our code typically adds it. > But feel free to correct me if the style guide says otherwise. Done. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:30: std::string* file_to_open) { On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > Do you want to set "file_to_open" to NULL before you do anything else? Good point. I'll test that it's empty. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:100: sizeof(buf), &fds); On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > What does RecvMsg() do for truncated messages? They can certainly confuse things > quite a bit and we probably will completely fail to parse subsequent data > correctly, when that happens. It does check msg_flags for MSG_TRUNC and MSG_CTRUNK and returns -1 in that case. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:109: if (msg_len < 0 || fds.size() != 1 || fds.at(0) < 0) { On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > Instead of saying fds.at(0) all over the place, I would probably assign this > value to a suitably named local variable. Done. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:165: if (socketpair(AF_UNIX, SOCK_STREAM, 0, socket_pair)) { On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > You need to know about package boundaries don't you? That probably means you > want to use SOCK_DGRAM. > > This currently works, because the kernel cannot coalesce messages that contain > ancillary data. But that sounds really fragile. You shouldn't rely on this very > subtle feature. > > I know you currently have no intention to do so, but if you ever added a new IPC > that didn't require ancillary data (e.g. because it is a "void" function), your > code would break in a rather hard-to-diagnose fashion -- and only when under > load from multiple threads. Very good point, thanks! SOCK_DGRAM wouldn't work, because I really want recvmsg() to return an error when the connection has died. I'll use SOCK_SEQPACKET instead. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:181: shutdown(socket_pair[0], SHUT_RD); On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > While it is somewhat arbitrary, your choice of indices is confusing. It would be > little more obvious if you followed the convention that pipe() uses. Index "0" > is used for reading, index "1" is used for writing. Done. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:183: initialized_ = true; On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > Why do we set initialized_ to true here? We later fall through to code that does > that again. Done. https://chromiumcodereview.appspot.com/11557025/diff/21001/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:214: flags != O_RDONLY) { On 2012/12/13 09:48:52, Markus (顧孟勤) wrote: > Can you add some comments that the handling of flags is overly simplified. > > For example, it would be perfectly legitimate and safe for somebody to set the > O_NONBLOCK or O_CLOEXEC flag in addition to O_RDONLY. > > But these are flags that must be handled in the caller and cannot be handled by > the broker process. So, we have to mask them prior to passing the flags to the > broker. > > And then we later have to do the right thing to make the flags effective. > > See SOCK_NONBLOCK and SOCK_CLOEXEC for how to do that. I'll send a next iteration soon that also supports O_RDWR and calls fcntl properly.
Will: that CL is not finished yet, but the tiny change to base/ is final. Would you mind approving that now? Thanks, Julien
I'm inclined to believe you that this makes sense, but can you explain why and put it in the changelist description?
On 2012/12/13 19:59:22, willchan wrote: > I'm inclined to believe you that this makes sense, but can you explain why and > put it in the changelist description? sendmsg() can SIGPIPE if the other end doesn't exist. The code obviously doesn't expect this to happen. I've amended the CL description. Thanks! Julien
lgtm
jorgelo / markus: PTAL!
lgtm but please check the alignment comment. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:32: // Make sure that callers never pass a non empty string. In case callers nit: non-empty https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:51: // i.e. here is where we wouldn't add O_RESET_FILE_SYSTEM. nit: I.e. (i.e. with a capital 'I'). https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:53: const int known_flags = Do we need to keep this inside the function? I understand the desire to scope everything as tight as possible, but we're recreating |unsupported_flags| on every call. The compiler might optimize that away though. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:103: (void) HANDLE_EINTR(close(socket_pair[0])); Is this the right alignment? https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:309: COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY_has_weird_value); I know right? O_RDONLY is weird =P
unit tests pass on ARM
https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:32: // Make sure that callers never pass a non empty string. In case callers On 2012/12/13 23:52:46, Jorge Lucangeli Obes wrote: > nit: non-empty Done. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:51: // i.e. here is where we wouldn't add O_RESET_FILE_SYSTEM. On 2012/12/13 23:52:46, Jorge Lucangeli Obes wrote: > nit: I.e. (i.e. with a capital 'I'). Done. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:53: const int known_flags = On 2012/12/13 23:52:46, Jorge Lucangeli Obes wrote: > Do we need to keep this inside the function? I understand the desire to scope > everything as tight as possible, but we're recreating |unsupported_flags| on > every call. > > The compiler might optimize that away though. Indeed, I would be shocked if it wasn't optimized. That's why I use many temp variables liberally to explicit every step. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:103: (void) HANDLE_EINTR(close(socket_pair[0])); On 2012/12/13 23:52:46, Jorge Lucangeli Obes wrote: > Is this the right alignment? Good point! https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:309: COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY_has_weird_value); On 2012/12/13 23:52:46, Jorge Lucangeli Obes wrote: > I know right? O_RDONLY is weird =P The second part is the magic variable name that the macro will use as an array with negative size. It's pretty much an error message. I changed it to make it more clear.
https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:54: O_RDONLY | O_WRONLY | O_RDWR | These do look like bit fields, don't they. It's deceptive though, they aren't O_RDONLY is 0 (clearly not a bit field value) O_WRONLY is 1, and O_RDWR is 2 So, a caller could try passing in "3", which is undefined by the kernel, but which would pass your checks. You really need to treat the access mode differently from the file flags. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:55: O_APPEND | O_ASYNC | O_CLOEXEC | O_CREAT | O_DIRECT | O_CLOEXEC doesn't make any sense here. It is lost when you send the file descriptor over the socket. Instead, you have to handle it on the receiving side by setting the MSG_CMSG_CLOEXEC flag on recvmsg(). N.B.: You can't just use fcntl() to change the value after the fact. That introduces a race condition. And F_CLOEXEC is used exactly, because you want to avoid these type of race conditions. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:57: O_NOFOLLOW | O_NONBLOCK | O_NDELAY | O_SYNC | O_TRUNC; O_NONBLOCK and O_NDELAY are the same value. Don't list both of them, that's very confusing. If need be, add a comment: O_NOFOLLOW | O_NONBLOCK /* aka O_NDELAY */ | O_SYNC | ... https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:310: if (requested_flags & O_WRONLY) { As I pointed out earlier, these are not flags and treating them as such as at best confusing. How about: switch (requested_flags & O_ACCMODE) { case O_RDONLY: ... case O_WRONLY: ... case O_RDWR: ... default: There be dragons } https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... File sandbox/linux/services/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process_unittest.cc:90: } Can you test with some invalid value too? E.g. O_RDONLY|O_WRONLY|O_RDWR https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process_unittest.cc:203: bzero(tempfile_full_path, sizeof(tempfile_full_path)); bzero()? Really? Do we use that anywhere else? I thought we don't typically use BSD extensions. memset() would be the more commonly used function that is available in C89 and POSIX (and a few other standards to boot)
Thanks, PTAL! https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:54: O_RDONLY | O_WRONLY | O_RDWR | On 2012/12/14 00:40:34, Markus (顧孟勤) wrote: > These do look like bit fields, don't they. It's deceptive though, they aren't > > O_RDONLY is 0 (clearly not a bit field value) > O_WRONLY is 1, and > O_RDWR is 2 > > So, a caller could try passing in "3", which is undefined by the kernel, but > which would pass your checks. > > You really need to treat the access mode differently from the file flags. Done. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:55: O_APPEND | O_ASYNC | O_CLOEXEC | O_CREAT | O_DIRECT | On 2012/12/14 00:40:34, Markus (顧孟勤) wrote: > O_CLOEXEC doesn't make any sense here. It is lost when you send the file > descriptor over the socket. > > Instead, you have to handle it on the receiving side by setting the > MSG_CMSG_CLOEXEC flag on recvmsg(). > > N.B.: You can't just use fcntl() to change the value after the fact. That > introduces a race condition. And F_CLOEXEC is used exactly, because you want to > avoid these type of race conditions. Done. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:57: O_NOFOLLOW | O_NONBLOCK | O_NDELAY | O_SYNC | O_TRUNC; Do you feel strongly about this ? I prefer to list everything and let the compiler figure out which one are the same. I think it's more correct. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:310: if (requested_flags & O_WRONLY) { On 2012/12/14 00:40:34, Markus (顧孟勤) wrote: > As I pointed out earlier, these are not flags and treating them as such as at > best confusing. > > How about: > > switch (requested_flags & O_ACCMODE) { > case O_RDONLY: ... > case O_WRONLY: ... > case O_RDWR: ... > default: There be dragons > } Done. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... File sandbox/linux/services/broker_process_unittest.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process_unittest.cc:90: } On 2012/12/14 00:40:34, Markus (顧孟勤) wrote: > Can you test with some invalid value too? E.g. O_RDONLY|O_WRONLY|O_RDWR Done. https://chromiumcodereview.appspot.com/11557025/diff/20007/sandbox/linux/serv... sandbox/linux/services/broker_process_unittest.cc:203: bzero(tempfile_full_path, sizeof(tempfile_full_path)); On 2012/12/14 00:40:34, Markus (顧孟勤) wrote: > bzero()? Really? Do we use that anywhere else? > > I thought we don't typically use BSD extensions. memset() would be the more > commonly used function that is available in C89 and POSIX (and a few other > standards to boot) Done.
https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:26: int UnspportedFlagsMask() { s/UnspportedFlagsMask/UnsupportedFlagsMask/g You might even consider calling them ClientSideFlagsMask, instead. Because that's really what they are. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:77: O_NOFOLLOW | O_NONBLOCK | O_NDELAY | O_SYNC | O_TRUNC; I would really prefer if you took out the flags that we are not prepared to handle. The code is hard enough to read as is, including flags that we then immediately remove again just makes the code harder to explain. I am already a little confused by the fact that you sort the flag values alphabetically. But I understand why you are doing that. Personally, I would have preferred grouping the flags logically (including a comment explaining why the flags in this group are safe). But that's not easy to do either. So, your solution is more expedient. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:79: const int unsuported_flags = ~known_flags; If you keep things like O_NONBLOCK and O_CLOEXEC in your list of known_flags, then the above line is not technically correct. Yes, these flags are "known", but they are not "supported" in the broker process, as they are client-side only. I maintain that it is a fatal bug if you ever see them in the broker process, as they change the behavior of the broker. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:172: write_pickle.WriteInt(flags); This should read: write_pickle.WriteInt(flags & ~UnsupportedFlagsMask); Don't send requests to the broker that the broker can't possibly handle. It makes the code much harder to reason about. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:285: int opened_fd = open(file_to_open, flags); You should probably do "open(file_to_open, flags | O_CLOEXEC)" This is security in depth. You currently don't expect that your broker ever execs something else. But should it decide to do so in the future, then there is the possibility that you could leak file descriptors. That would result in really subtle bugs in the client. N.B. it doesn't affect the client one bit, whether you have O_CLOEXEC set or not; the kernel overrides this flag when sending the descriptor. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:285: int opened_fd = open(file_to_open, flags); I am undecided on whether you should also set O_NONBLOCK in the call to open(). In principle, this is a good idea, because your broker is single-threaded and causing it to dead-lock on open() would be very bad indeed. In practice, I am not quite sure what the kernel does if you try to send a file descriptor that has a pending open() operation. And I don't have any good idea how to quickly test this. I don't have any file systems that block in open(). So, maybe, this could just be a TODO?
Thank, PTAL! I've centralized the flags checking more. Hopefully this is much more clear now. Everything is done though GetFileNameIfAllowedAccess(), which is called both by the client and the broker. The tests will test both, since they can set fast_check_in_client_ to false. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:26: int UnspportedFlagsMask() { On 2012/12/14 01:36:23, Markus (顧孟勤) wrote: > s/UnspportedFlagsMask/UnsupportedFlagsMask/g > > You might even consider calling them ClientSideFlagsMask, instead. Because > that's really what they are. I renamed it to ForCurrentProcessFlagsMask. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:79: const int unsuported_flags = ~known_flags; On 2012/12/14 01:36:23, Markus (顧孟勤) wrote: > If you keep things like O_NONBLOCK and O_CLOEXEC in your list of known_flags, > then the above line is not technically correct. > > Yes, these flags are "known", but they are not "supported" in the broker > process, as they are client-side only. > > I maintain that it is a fatal bug if you ever see them in the broker process, as > they change the behavior of the broker. Hopefully I've made this whole area more clear now :) https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:172: write_pickle.WriteInt(flags); On 2012/12/14 01:36:23, Markus (顧孟勤) wrote: > This should read: write_pickle.WriteInt(flags & ~UnsupportedFlagsMask); > > Don't send requests to the broker that the broker can't possibly handle. It > makes the code much harder to reason about. I really want all the relevant checks in *one* function that I can call from both the client and the server. I've modified GetFileNameIfAllowedAcess() along those lines. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:285: int opened_fd = open(file_to_open, flags); On 2012/12/14 01:36:23, Markus (顧孟勤) wrote: > You should probably do "open(file_to_open, flags | O_CLOEXEC)" > > This is security in depth. You currently don't expect that your broker ever > execs something else. But should it decide to do so in the future, then there is > the possibility that you could leak file descriptors. That would result in > really subtle bugs in the client. > > N.B. it doesn't affect the client one bit, whether you have O_CLOEXEC set or > not; the kernel overrides this flag when sending the descriptor. Done. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:285: int opened_fd = open(file_to_open, flags); On 2012/12/14 01:36:23, Markus (顧孟勤) wrote: > I am undecided on whether you should also set O_NONBLOCK in the call to open(). > In principle, this is a good idea, because your broker is single-threaded and > causing it to dead-lock on open() would be very bad indeed. > > In practice, I am not quite sure what the kernel does if you try to send a file > descriptor that has a pending open() operation. And I don't have any good idea > how to quickly test this. I don't have any file systems that block in open(). > > So, maybe, this could just be a TODO? I think that we do want to block in this case. The typical use case for this is the initialization of a third-party library. We need it for now for the GPU process for some particular cards. Most notably, it needs to open a device file, which I could imagine could block. If we need to support multiple threads efficiently, we'll probably need to make many changes.
lgtm As I mentioned before, I don't think it's ideal. But it'll do for now. You'll probably end up changing things anyway, once you add proper support for the missing flags. https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/11557025/diff/21006/sandbox/linux/serv... sandbox/linux/services/broker_process.cc:172: write_pickle.WriteInt(flags); I am not going to fight you more over this. But I think you are aiming for the wrong goal. Client and broker are fundamentally different and need a different set of checks. On 2012/12/14 02:01:19, Julien Tinnes wrote: > On 2012/12/14 01:36:23, Markus (顧孟勤) wrote: > > This should read: write_pickle.WriteInt(flags & ~UnsupportedFlagsMask); > > > > Don't send requests to the broker that the broker can't possibly handle. It > > makes the code much harder to reason about. > > I really want all the relevant checks in *one* function that I can call from > both the client and the server. > > I've modified GetFileNameIfAllowedAcess() along those lines.
Unit tests still passing.
On 2012/12/14 02:10:41, Markus (顧孟勤) wrote: > lgtm > > As I mentioned before, I don't think it's ideal. But it'll do for now. You'll > probably end up changing things anyway, once you add proper support for the > missing flags. Let's talk about this in person tomorrow. I feel strongly that broker checks and client checks should be the same. All client checks should be done on the broker, for security reasons. All broker checks should be done on the client, for performance reasons. And in tests, we do test both. I really think this is the right thing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/11557025/6021
Presubmit check for 11557025-6021 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: sandbox/linux/services/broker_process.cc \ sandbox/linux/services/broker_process.h \ sandbox/linux/services/broker_process_unittest.cc Presubmit checks took 1.2s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/11557025/19015
Message was sent while issue was closed.
Change committed as 173064
Message was sent while issue was closed.
Reverted because of a compile failure on MAC. Re-landing in https://codereview.chromium.org/11564030 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
