Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1210973003:
Use crypto::InitNSSSafely to initialize NSS for remoting. (Closed)
DescriptionUse crypto::InitNSSSafely to initialize NSS for remoting.
It was doing it manually and missed DisableNSSForkCheck. This was working
because ppapi_plugin_main.cc contains a redundant call. This instance and
ppapi_plugin_main.cc's instance will be removed at different times (when
we can remove the internal remoting plugin and when we can assume the
chimera, respectively), so decouple the two to help set up the dominoes
for the future removal.
BUG=506323
Committed: https://crrev.com/0d6bcbaecce3d7b6607ce36127de3a683c428294
Cr-Commit-Position: refs/heads/master@{#337890}
Patch Set 1 #
Total comments: 3
Dependent Patchsets: Messages
Total messages: 19 (6 generated)
davidben@chromium.org changed reviewers: + jln@chromium.org
I believe this should be like this? We can't remove it until remoting has shipped the PNaCl version, sadly, but this probably needs to be fixed before the ppapi_plugin_main.cc copy is disabled in the USE_OPENSSL build.
lgtm because I think it's more clear to call InitNSSSafely() rather than call ForceNSSNoDBInit() and EnsureNSSInit() manually. However, I don't think either is necessary. But it may still be a good idea to do it, in case this regresses in PluginMain(). I would prefer for Ryan to also take a look if possible. https://chromiumcodereview.appspot.com/1210973003/diff/1/chrome/plugin/chrome... File chrome/plugin/chrome_content_plugin_client.cc (right): https://chromiumcodereview.appspot.com/1210973003/diff/1/chrome/plugin/chrome... chrome/plugin/chrome_content_plugin_client.cc:38: crypto::InitNSSSafely(); It looks like this shouldn't be needed because content/ppapi_plugin/ppapi_plugin_main.cc calls crypto::InitNSSSafely() before calling the plugin's PreSandboxInitialization(). But it shouldn't hurt to call it twice? By the time this runs, the first sandbox (setuid sandbox) should be engaged. Also the comment above is wrong. The NSS libraries are already loaded because ZygotePreSandboxInit() calls crypto::LoadNSSLibraries() before this runs. (Note: I'm generally quite confused about the state of affairs of NSS initialization, Ryan is the expert).
https://chromiumcodereview.appspot.com/1210973003/diff/1/chrome/plugin/chrome... File chrome/plugin/chrome_content_plugin_client.cc (right): https://chromiumcodereview.appspot.com/1210973003/diff/1/chrome/plugin/chrome... chrome/plugin/chrome_content_plugin_client.cc:38: crypto::InitNSSSafely(); On 2015/07/07 17:45:36, jln wrote: > It looks like this shouldn't be needed because > content/ppapi_plugin/ppapi_plugin_main.cc calls crypto::InitNSSSafely() before > calling the plugin's PreSandboxInitialization(). > But it shouldn't hurt to call it twice? Yeah, it's a bit of a mess. I should probably have squashed these commits. This one's keyed on ENABLE_REMOTING. The ppapi_plugin one was added for ClearKey and so it only needs !USE_OPENSSL, not USE_NSS_CERTS, while remoting actually needs USE_NSS_CERTS but is conditioned on ENABLE_REMOTING. https://chromiumcodereview.appspot.com/1222103005/ This codepath also does something crazy with crypt32.dll for similar reasons, so I thought it would be better to keep remoting's InitNSSSafely together with it. I also think it makes sense to keep them separate so that they can be removed independently from each other. The end state I want to be at is: - LoadNSSLibraries is !USE_OPENSSL || USE_NSS_CERTS, because of this code. - This is USE_NSS_CERTS because it actually needs it. - Everything else is !USE_OPENSSL. When remoting's internal plugin is deleted in favor of PNaCl, ENABLE_REMOTING code should be gone and we can remove this code. When the chimera sticks, we can remove all !USE_OPENSSL code that isn't built on iOS, so the rest of the InitNSSSafely's can go away. When both events have happened, LoadNSSLibraries and all its dependencies will be gone. How about I squash all the changes into one CL rather than several, so it's clearer what all is going on? > By the time this runs, the first sandbox (setuid sandbox) should be engaged. > > Also the comment above is wrong. The NSS libraries are already loaded because > ZygotePreSandboxInit() calls crypto::LoadNSSLibraries() before this runs. My understanding is that LoadNSSLibraries needs to run before the first sandbox is engaged while this needs to run before the second is? > (Note: I'm generally quite confused about the state of affairs of NSS > initialization, Ryan is the expert).
davidben@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1210973003/diff/1/chrome/plugin/chrome_conten... File chrome/plugin/chrome_content_plugin_client.cc (right): https://codereview.chromium.org/1210973003/diff/1/chrome/plugin/chrome_conten... chrome/plugin/chrome_content_plugin_client.cc:38: crypto::InitNSSSafely(); On 2015/07/07 18:14:48, David Benjamin wrote: > > Also the comment above is wrong. The NSS libraries are already loaded because > > ZygotePreSandboxInit() calls crypto::LoadNSSLibraries() before this runs. > > My understanding is that LoadNSSLibraries needs to run before the first sandbox > is engaged while this needs to run before the second is? Right, I think we actually want to delete this code, because it suggests we can still end up loading files off disk (c.f. 41 / 47, new), which we explicitly don't want. Line 38 (original) was our one defense of "Don't mess up" crypto::LoadNSSLibraries() needs to be called while we still have access to the filesystem (which would appear to still include this section, before either sandbox?), but should nothing other than dlopen the bits. That seems most aligned with what this code is "also" doing, although like jln@ mentioned, it should have been called before even that. I think this call exists because we initially wanted to limit the NSS loading to just being the PPAPI plugin process (for Chromoting), but then as we expanded to include the GPU process, the CDM process, and [much later] WebCrypto, we ended up deciding to load it for all processes (hence in the zygote) crypto::InitNSSSafely() should be called after both sandboxes have been closed, and before any crypto::EnsureNSSInit() [which would no-op]. So I think that should be deferred to PPAPI, but *after* the sandbox has closed. That should be fine, provided the libc hacks are in. TL;DR: Pretty sure we want to delete this.
Bah, LGTM, because we still consider Linux systems that haven't received security updates for 2 years to be "Supported" ;(
Chatted with Ryan out-of-band. Here's my understanding of the state of the world, for posterity: We have two sandboxes, the setuid sandbox and the seccomp sandbox. The setuid sandbox is entered early in the zygote before forking. The seccomp sandbox is entered after fork in the individual types of child processes. For simplicity, I'm going to use "pre-sandbox" to mean before the setuid sandbox is entered, "post-sandbox" to mean after the seccomp sandbox is entered, and "mid-sandbox" to mean in between the two. For NSS to be usable post-sandbox, LoadNSSLibraries must be called pre-sandbox. As long as any child needs NSS post-sandbox, we can't remove that line. Right now, a lot of code on Linux needs NSS post-sandbox because all crypto code goes through NSS (e.g. WebCrypto). But with the chimera, much of that will be BoringSSL. Thus we hope to get rid of all of this imminently. The chimera isn't enough though. There is one consumer, remoting, which needs NSS in the sandbox because it uses X509Certificate. That's this code. Remoting's use is also short-lived as they will soon be a PNaCl plugin and we can delete the internal plugin. Whether this removal will happen before or after we can delete non-chimera Linux code, I'm not sure. There's a few weird things about remoting's code here: 1. Remoting's code is also redundant with PpapiPluginMain, here: https://codereview.chromium.org/1222103005/ However, this code fails to do all three sets of InitNSSSafely, unlike the PpapiPluginMain. Hence this CL. The PpapiPluginMain main instance was added for ClearKey which no longer needs NSS post-chimera. Rather than fold the two together, I opted to decouple the two and make this only for remoting and the other only for ClearKey. This is because I don't know whether we will remove !USE_OPENSSL or ENABLE_REMOTING code first. 2. This code runs mid-sandbox. Ryan tells me that InitNSSSafely need only be called post-sandbox, so we probably should move this and the PpapiPluginMain code. Except that really old versions of NSS will have problems with the seccomp sandbox. Though, if I'm reading it right, chrome_render_process_observer.cc's instance runs post-sandbox, not mid-sandbox, so one would think it already has problems already? Anyway. The conclusion is: This CL address #1 to help set up dominos for this code's future removal. This CL doesn't make #2 any worse than it is now. Because this code is to be removed later anyway, it does not seem worth investigating #2.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210973003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
davidben@chromium.org changed reviewers: + jhawkins@chromium.org
+jhawkins for OWNERS
lgtm
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210973003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0d6bcbaecce3d7b6607ce36127de3a683c428294 Cr-Commit-Position: refs/heads/master@{#337890} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
