Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
695353005:
Non-SFI mode: Use dummy PID for NaCl's IPC channel on Linux platform. (Closed)
|
Created:
6 years, 1 month ago by hidehiko Modified:
6 years ago Reviewers:
jschuh, jam, dmichael (off chromium), jln (very slow on Chromium), Mark Seaborn, mdempsky CC:
chromium-reviews, darin-cc_chromium.org, jam, hamaji, Junichi Uekawa, mazda, mdempsky, jschuh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionNon-SFI mode: Use dummy PID for NaCl's IPC channel and IPC channel on Linux platform.
We do not want to expose PID from process on Linux, because it does not play any security role.
Specifically, in NaCl processes, now although getpid() syscall is prohibited by seccomp sandbox, it looks working, probably because of the cache in libc layer.
By this CL, Linux IPC, including nacl_helper_nonsfi, uses dummy PID (-1).
Note; as for nacl_helper process, currently, the process is under PID namespace, so "dummy-like-" PID is already used.
BUG=358465
TEST=Ran trybot.
Committed: https://crrev.com/20a9a3ad484caea1f97b9c8824a79a5c8bf8a958
Cr-Commit-Position: refs/heads/master@{#307853}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : Rebase #Patch Set 5 : #Patch Set 6 : Rebase #Patch Set 7 : #
Total comments: 6
Patch Set 8 : #
Messages
Total messages: 30 (5 generated)
hidehiko@chromium.org changed reviewers: + dmichael@chromium.org, jln@chromium.org, mseaborn@chromium.org
Could you take a look? Mark, could you take a look this from project point of view? Julien, could you review this from security point of view? Dave, could you review as an NaCl familiar OWNER of the ipc/... library? Also, I've CC'ed those who help me to discussed PID related stuff. Again, thank you very much for the discussion! Please let me know if you have any security concerns on this. Best regards, - hidehiko
ipc lgtm https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel.cc File ipc/ipc_channel.cc (right): https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel.cc#newcode33 ipc/ipc_channel.cc:33: // nacl_helper_nonsfi does not allow to use getpid() (it would cause SIGSYS nit: the grammar here could be better. How about: // The seccomp sandbox disallows the use of getpid(), so we provide // a dummy PID. https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel_posix.cc#new... ipc/ipc_channel_posix.cc:777: // allowed and would cause a SIGSYS crashing because of seccomp sandbox. nit: of +the+ seccomp sandbox
Thank you for review. PTAL. (Sorry, changes due to rebasing are mixed in revision 2. I hope it's not a problem, as the mixed change is simple macro renaming only). https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel.cc File ipc/ipc_channel.cc (right): https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel.cc#newcode33 ipc/ipc_channel.cc:33: // nacl_helper_nonsfi does not allow to use getpid() (it would cause SIGSYS On 2014/11/05 23:48:41, dmichael wrote: > nit: the grammar here could be better. How about: > // The seccomp sandbox disallows the use of getpid(), so we provide > // a dummy PID. Done. Thank you for suggestion. https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/1/ipc/ipc_channel_posix.cc#new... ipc/ipc_channel_posix.cc:777: // allowed and would cause a SIGSYS crashing because of seccomp sandbox. On 2014/11/05 23:48:41, dmichael wrote: > nit: of +the+ seccomp sandbox Done.
Do you mind if I defer reviewing this until this code path is testable (i.e. when nacl_helper_nonsfi can run some PPAPI tests)?
On 2014/11/06 17:15:12, Mark Seaborn wrote: > Do you mind if I defer reviewing this until this code path is testable (i.e. > when nacl_helper_nonsfi can run some PPAPI tests)? I'm OK, but this affects to nacl_helper in SFI mode, too, so I guess this CL is tested already? Also, doing sandbox related stuff at once cause a gigantic CL, so it would be safer to split it some pieces. This could be one of the pieces, and needs to be landed before sandbox enabled. So, maybe there is not much merit to waiting for it... ?
On 6 November 2014 11:50, <hidehiko@chromium.org> wrote: > On 2014/11/06 17:15:12, Mark Seaborn wrote: > >> Do you mind if I defer reviewing this until this code path is testable >> (i.e. >> when nacl_helper_nonsfi can run some PPAPI tests)? >> > > I'm OK, but this affects to nacl_helper in SFI mode, too, so I guess this > CL is > tested already? > Well, it contains a bunch of "#if defined(OS_NACL_NONSFI)"s that aren't tested by the bots at the moment... > Also, doing sandbox related stuff at once cause a gigantic CL, so it would > be > safer to split it some pieces. > This could be one of the pieces, and needs to be landed before sandbox > enabled. > Can it be landed separately before enabling the sandbox, but after nacl_helper_nonsfi works? I'm OK with a certain amount of committing hard-to-test changes during bring-up, but there's been a lot of those already, and it seems this one can be deferred until it is testable. :-) Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
mdempsky@chromium.org changed reviewers: + mdempsky@chromium.org
https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_helper_linux.cc:411: IPC::Channel::SetGlobalPid(-1); // Set dummy PID for IPC. Is this the right place to put this call? It looks like this for !nonsfi, whereas in ipc_channel.h it seems like SetGlobalPid() is only available for OS_LINUX and NONSFI. I would think this wouldn't compile under SFI. https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc... ipc/ipc_channel_posix.cc:780: return global_pid_; Will global_pid_ ever be set to anything other than -1 for nonsfi? Could we simplify things by changing this to: #if defined(OS_NACL_NONSFI) return -1; #else #if defined(OS_LINUX) if (global_pid_) return global_pid_; #endif return base::GetCurrentProcId(); #endif and then you don't even need SetGlobalPid() for nonsfi? I might also suggest using 0 instead of -1 for the nonsfi PID (because Linux already uses 0 to indicate an unrepresentable PID), but not if it ends up introducing any other complications.
Mark, I see. So please feel free to hold on until the browser_tests for Non-SFI gets ready, but if you have any concerns in advance, please let me know. I'll run trybots again, and ping you, when ready. Thank you for comment, Matthew. PTAL. https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_helper_linux.cc:411: IPC::Channel::SetGlobalPid(-1); // Set dummy PID for IPC. On 2014/11/07 03:05:34, mdempsky (Tokyo until Nov 14) wrote: > Is this the right place to put this call? It looks like this for !nonsfi, > whereas in ipc_channel.h it seems like SetGlobalPid() is only available for > OS_LINUX and NONSFI. I would think this wouldn't compile under SFI. Yes, this is the right place. This is not only for nonsfi but also for sfi. Note that nacl_helper for sfi mode is build by host toolchain (i.e. under OS_LINUX rather than OS_NACL). We won't compile this file to SFI nexe binary. https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc... ipc/ipc_channel_posix.cc:780: return global_pid_; On 2014/11/07 03:05:34, mdempsky (Tokyo until Nov 14) wrote: > Will global_pid_ ever be set to anything other than -1 for nonsfi? Could we > simplify things by changing this to: > > #if defined(OS_NACL_NONSFI) > return -1; > #else > #if defined(OS_LINUX) > if (global_pid_) > return global_pid_; > #endif > return base::GetCurrentProcId(); > #endif > > and then you don't even need SetGlobalPid() for nonsfi? > > I might also suggest using 0 instead of -1 for the nonsfi PID (because Linux > already uses 0 to indicate an unrepresentable PID), but not if it ends up > introducing any other complications. This is both for SFI (under OS_LINUX) and Non-SFI (under OS_NACL_NONSFI), so it may be better to be shared for consistency. WDYT? Also, 0 does not work. It is used as "Not yet initialized" in IPC. Actually, we'd hit a real issue at IPC::ChannelProxy::Context::OnAddFilter, if we use 0 instead.
Can we resume this review? browser_tests are enabled on x86-32 bots (linux_rel_precise32).
On 2014/12/02 05:35:58, hidehiko wrote: > Can we resume this review? browser_tests are enabled on > x86-32 bots (linux_rel_precise32). Sure. Your commit message says: "This is short term workaround of PID use case for IPC channel." Is this still a short term workaround, or is this intended to be the long-term solution? https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_helper_linux.cc:408: // Now do nothing in main for nacl_helper_nonsfi. Can you rebase this change, so that I can see what the current context is? https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc... ipc/ipc_channel_posix.cc:780: return global_pid_; On 2014/11/07 05:43:21, hidehiko wrote: > On 2014/11/07 03:05:34, mdempsky (Tokyo until Nov 14) wrote: > > Will global_pid_ ever be set to anything other than -1 for nonsfi? Could we > > simplify things by changing this to: > > > > #if defined(OS_NACL_NONSFI) > > return -1; > > #else > > #if defined(OS_LINUX) > > if (global_pid_) > > return global_pid_; > > #endif > > return base::GetCurrentProcId(); > > #endif > > > > and then you don't even need SetGlobalPid() for nonsfi? > > > > I might also suggest using 0 instead of -1 for the nonsfi PID (because Linux > > already uses 0 to indicate an unrepresentable PID), but not if it ends up > > introducing any other complications. > > This is both for SFI (under OS_LINUX) and Non-SFI (under OS_NACL_NONSFI), so it > may be better to be shared for consistency. WDYT? I agree with Matthew's comment about doing: #if defined(OS_NACL_NONSFI) return -1; #... Furthermore, why not do the following? #if defined(OS_NACL_NONSFI) || defined(OS_LINUX) return -1; #... AFAICT, the PID is not required on Linux. My understanding is that the PID was originally required for Windows, and this usage leaked across onto other platforms.
Thank you for review. PTAL. https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_helper_linux.cc (right): https://codereview.chromium.org/695353005/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_helper_linux.cc:408: // Now do nothing in main for nacl_helper_nonsfi. On 2014/12/03 01:54:00, Mark Seaborn wrote: > Can you rebase this change, so that I can see what the current context is? Done. https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/20001/ipc/ipc_channel_posix.cc... ipc/ipc_channel_posix.cc:780: return global_pid_; On 2014/12/03 01:54:00, Mark Seaborn wrote: > On 2014/11/07 05:43:21, hidehiko wrote: > > On 2014/11/07 03:05:34, mdempsky (Tokyo until Nov 14) wrote: > > > Will global_pid_ ever be set to anything other than -1 for nonsfi? Could we > > > simplify things by changing this to: > > > > > > #if defined(OS_NACL_NONSFI) > > > return -1; > > > #else > > > #if defined(OS_LINUX) > > > if (global_pid_) > > > return global_pid_; > > > #endif > > > return base::GetCurrentProcId(); > > > #endif > > > > > > and then you don't even need SetGlobalPid() for nonsfi? > > > > > > I might also suggest using 0 instead of -1 for the nonsfi PID (because Linux > > > already uses 0 to indicate an unrepresentable PID), but not if it ends up > > > introducing any other complications. > > > > This is both for SFI (under OS_LINUX) and Non-SFI (under OS_NACL_NONSFI), so > it > > may be better to be shared for consistency. WDYT? > > I agree with Matthew's comment about doing: > #if defined(OS_NACL_NONSFI) > return -1; > #... > > Furthermore, why not do the following? > #if defined(OS_NACL_NONSFI) || defined(OS_LINUX) > return -1; > #... > > AFAICT, the PID is not required on Linux. My understanding is that the PID was > originally required for Windows, and this usage leaked across onto other > platforms. Ok, done, in mdempsky@'s way. My focus is Non-SFI. Under OS_LINUX, some other code, like zygote, uses the feature, which is out of our focus. Also, I have no way to confirm if it is ok to do, or there is some problems. So, I defer to mdempsky@ and jln@ for the decision. If it could be safely removable, please let me know. Then, I'd remove global_pid_ itself.
jln@chromium.org changed reviewers: + jschuh@chromium.org
On 2014/12/03 17:03:19, hidehiko wrote: > > Furthermore, why not do the following? > > #if defined(OS_NACL_NONSFI) || defined(OS_LINUX) > > return -1; > > #... > > > > AFAICT, the PID is not required on Linux. My understanding is that the PID > was > > originally required for Windows, and this usage leaked across onto other > > platforms. > > Ok, done, in mdempsky@'s way. > My focus is Non-SFI. Under OS_LINUX, some other code, like zygote, uses the > feature, which is out of our focus. Also, I have no way to confirm if it is ok > to do, or there is some problems. So, I defer to mdempsky@ and jln@ for the > decision. If it could be safely removable, please let me know. Then, I'd remove > global_pid_ itself. jschuh@ confirmed that the PID should not play any security role on Linux. I think you could remove it, my only fear is that the feature is abused somewhere for something unrelated. I would say that for this CL, forcing PIDs to -1 (or 0) is good enough, removing all notions of PID in Linux IPC can happen as a separate effort (I could take a look).
hidehiko@chromium.org changed reviewers: + jam@chromium.org
PTAL. On 2014/12/04 19:50:48, jln wrote: > On 2014/12/03 17:03:19, hidehiko wrote: > > > > Furthermore, why not do the following? > > > #if defined(OS_NACL_NONSFI) || defined(OS_LINUX) > > > return -1; > > > #... > > > > > > AFAICT, the PID is not required on Linux. My understanding is that the PID > > was > > > originally required for Windows, and this usage leaked across onto other > > > platforms. > > > > Ok, done, in mdempsky@'s way. > > My focus is Non-SFI. Under OS_LINUX, some other code, like zygote, uses the > > feature, which is out of our focus. Also, I have no way to confirm if it is ok > > to do, or there is some problems. So, I defer to mdempsky@ and jln@ for the > > decision. If it could be safely removable, please let me know. Then, I'd > remove > > global_pid_ itself. > > jschuh@ confirmed that the PID should not play any security role on Linux. I > think you could remove it, my only fear is that the feature is abused somewhere > for something unrelated. I would say that for this CL, forcing PIDs to -1 (or 0) > is good enough, removing all notions of PID in Linux IPC can happen as a > separate effort (I could take a look). Thank you for confirmation. Done for linux, too. By this CL, on Linux, IPC::ChannelProxy::GetPeerPID starts to return -1. To be honest, I'm not sure if it is OK or not, but all tests pass, so maybe ok? Added jam@ to R=. Could you kindly review this CL as an OWNER of IPC especially for linux changes? Thanks, - hidehiko
lgtm
The commit message says "This is short term workaround of PID use case for IPC channel." Shouldn't that be removed? LGTM otherwise. https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_channel_posix.c... ipc/ipc_channel_posix.cc:766: // allowed and would cause a SIGSYS crashing because of the seccomp sandbox. "crashing" -> "crash" https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_sync_channel_un... File ipc/ipc_sync_channel_unittest.cc (left): https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_sync_channel_un... ipc/ipc_sync_channel_unittest.cc:1725: ASSERT_EQ(channel()->GetPeerPID(), base::GetCurrentProcId()); Should this check stay for non-Linux platforms? Should it assert that GetPeerPID() returns a dummy value on Linux? https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_sync_channel_un... ipc/ipc_sync_channel_unittest.cc:1754: ASSERT_EQ(channel()->GetPeerPID(), base::GetCurrentProcId()); Same for this one.
Thank you for review, jam@, mseaborn@. Julien, would you mind to take a look? Thanks, - hidehiko https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_channel_posix.c... ipc/ipc_channel_posix.cc:766: // allowed and would cause a SIGSYS crashing because of the seccomp sandbox. On 2014/12/08 21:40:36, Mark Seaborn wrote: > "crashing" -> "crash" Done. https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_sync_channel_un... File ipc/ipc_sync_channel_unittest.cc (left): https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_sync_channel_un... ipc/ipc_sync_channel_unittest.cc:1725: ASSERT_EQ(channel()->GetPeerPID(), base::GetCurrentProcId()); On 2014/12/08 21:40:36, Mark Seaborn wrote: > Should this check stay for non-Linux platforms? > > Should it assert that GetPeerPID() returns a dummy value on Linux? Done. https://codereview.chromium.org/695353005/diff/120001/ipc/ipc_sync_channel_un... ipc/ipc_sync_channel_unittest.cc:1754: ASSERT_EQ(channel()->GetPeerPID(), base::GetCurrentProcId()); On 2014/12/08 21:40:36, Mark Seaborn wrote: > Same for this one. Done.
On 8 December 2014 at 13:40, <mseaborn@chromium.org> wrote: > The commit message says "This is short term workaround of PID use case for > IPC > channel." Shouldn't that be removed? Ping on the above -- you didn't address that comment. Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/09 16:43:27, Mark Seaborn wrote: > On 8 December 2014 at 13:40, <mailto:mseaborn@chromium.org> wrote: > > > The commit message says "This is short term workaround of PID use case for > > IPC > > channel." Shouldn't that be removed? > > > Ping on the above -- you didn't address that comment. Oops. I've just updated. PTAL. > > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
lgtm LGTM for content/zygote/OWNERS
On 2014/12/10 01:38:34, mdempsky wrote: > lgtm > > LGTM for content/zygote/OWNERS Thank you for review. So, all OWNERs say ok. jln@, do you have any comments? If not, I'll submit. Thanks, - hidehiko
Forgot to lgtm, sorry.
On 2014/12/11 01:46:10, jln wrote: > Forgot to lgtm, sorry. Thank you, all. Submitting.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/695353005/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/20a9a3ad484caea1f97b9c8824a79a5c8bf8a958 Cr-Commit-Position: refs/heads/master@{#307853} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
