Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11411013:
Initialize NSS in the PPAPI process for ClearKey CDM. (Closed)
|
Created:
8 years, 1 month ago by Jorge Lucangeli Obes Modified:
8 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jln (very slow on Chromium) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionInitialize NSS in the PPAPI process for ClearKey CDM.
BUG=156864
TEST=content_browsertests passes on linux_rel_precise.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168372
Patch Set 1 #Patch Set 2 : Upload something that actually compiles. #
Total comments: 2
Patch Set 3 : Add DisableNSSForkCheck to wrapper for future use. #
Total comments: 4
Patch Set 4 : Address nits in comments. #
Total comments: 2
Patch Set 5 : Rebase and address Brett's comment. #
Total comments: 3
Messages
Total messages: 21 (0 generated)
Taking over this one (https://codereview.chromium.org/11231021) since Julien is busy with other stuff. There's another NSS init in: https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... What makes the most sense is add crypto::DisableNSSForkCheck() to the wrapper and use the wrapper in both places. If people are happy with the wrapper, I'll add that to the CL.
https://chromiumcodereview.appspot.com/11411013/diff/3001/content/ppapi_plugi... File content/ppapi_plugin/ppapi_plugin_main.cc (right): https://chromiumcodereview.appspot.com/11411013/diff/3001/content/ppapi_plugi... content/ppapi_plugin/ppapi_plugin_main.cc:88: InitializeSandbox(); Just showing my own ignorance here: This call initializes the seccomp-bpf sandbox, correct? The SUID sandbox is already in place by the time the process gets here, by virtue of having forked off the zygote, right?
https://chromiumcodereview.appspot.com/11411013/diff/3001/content/ppapi_plugi... File content/ppapi_plugin/ppapi_plugin_main.cc (right): https://chromiumcodereview.appspot.com/11411013/diff/3001/content/ppapi_plugi... content/ppapi_plugin/ppapi_plugin_main.cc:88: InitializeSandbox(); On 2012/11/15 19:48:02, Ryan Sleevi wrote: > Just showing my own ignorance here: > > This call initializes the seccomp-bpf sandbox, correct? The SUID sandbox is > already in place by the time the process gets here, by virtue of having forked > off the zygote, right? Correct. There's no "initializing" the SUID sandbox per-se, just the Zygote telling the helper binary to perform the chrooting, which happens in ZygoteMain.
So this change LGTM. "It would be nice" to have NSS fully run in the seccomp-bpf sandbox - that is, I would not be surprised if there are other code paths, particularly within NSPR, that may trigger seccomp-bpf's unhandled syscall logic. If we could whitelist those - having them fail, but without crashing - it would be nice. That said, I understand the motivation for having the disallowed calls crash, and it's probably better to have NSS initialized in all OOP PPAPI processes than to have syscalls silently fail for all OOP PPAPI processes, which is why I'm OK with this. Wan-Teh may wish to double check though.
On 2012/11/15 19:57:34, Ryan Sleevi wrote: > So this change LGTM. "It would be nice" to have NSS fully run in the seccomp-bpf > sandbox - that is, I would not be surprised if there are other code paths, > particularly within NSPR, that may trigger seccomp-bpf's unhandled syscall > logic. If we could whitelist those - having them fail, but without crashing - it > would be nice. > > That said, I understand the motivation for having the disallowed calls crash, > and it's probably better to have NSS initialized in all OOP PPAPI processes than > to have syscalls silently fail for all OOP PPAPI processes, which is why I'm OK > with this. > > Wan-Teh may wish to double check though. Do you think it's worth it to change https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... as well? I stopped seeing the crashing syscalls in my local checkout even before this NSS change. I was seeing the plugin timeout, but no segfaults. We are always looking for crashes, and we would definitely whitelist or gracefully block anything that we find, as long as the graceful block does not compromise security.
On 2012/11/15 19:40:28, Jorge Lucangeli Obes wrote: > Taking over this one (https://codereview.chromium.org/11231021) since Julien is > busy with other stuff. > > There's another NSS init in: > https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... > > What makes the most sense is add crypto::DisableNSSForkCheck() to the wrapper > and use the wrapper in both places. If people are happy with the wrapper, I'll > add that to the CL. That SGTM
On 2012/11/15 20:27:37, Ryan Sleevi wrote: > On 2012/11/15 19:40:28, Jorge Lucangeli Obes wrote: > > Taking over this one (https://codereview.chromium.org/11231021) since Julien > is > > busy with other stuff. > > > > There's another NSS init in: > > > https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... > > > > What makes the most sense is add crypto::DisableNSSForkCheck() to the wrapper > > and use the wrapper in both places. If people are happy with the wrapper, I'll > > add that to the CL. > > That SGTM Antoine, mind taking a look for OWNERS, or suggesting an appropriate OWNER?
On 2012/11/15 22:09:45, Jorge Lucangeli Obes wrote: > On 2012/11/15 20:27:37, Ryan Sleevi wrote: > > On 2012/11/15 19:40:28, Jorge Lucangeli Obes wrote: > > > Taking over this one (https://codereview.chromium.org/11231021) since Julien > > is > > > busy with other stuff. > > > > > > There's another NSS init in: > > > > > > https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... > > > > > > What makes the most sense is add crypto::DisableNSSForkCheck() to the > wrapper > > > and use the wrapper in both places. If people are happy with the wrapper, > I'll > > > add that to the CL. > > > > That SGTM > > Antoine, mind taking a look for OWNERS, or suggesting an appropriate OWNER? Antoine is out this week, John, would you mind taking a look? This CL explicitly initializes NSS before enabling the seccomp-bpf sandbox on Linux and Chrome OS.
LGTM, with paranoia-nit https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.cc File crypto/nss_util.cc (right): https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.cc#... crypto/nss_util.cc:657: // but it's more correct to tell NSS to not do it. I would remove the reference to the setuid sandbox. Now that this is in crypto/, we really don't know if this will run sandboxed. Probably a comment in the interface file would be better suited. https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.h File crypto/nss_util.h (right): https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.h#n... crypto/nss_util.h:41: // sandboxed environment. I would maybe add a comment here along the lines of: "As a security in depth measure, this should be called in a sandboxed environment to make sure that NSS will not load security modules that could expose private data and keys. Please make sure to get a lgtm from security if you use this." In any case, it's more appropriate to mention sandboxing here than in the implementation.
https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.cc File crypto/nss_util.cc (right): https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.cc#... crypto/nss_util.cc:657: // but it's more correct to tell NSS to not do it. On 2012/11/15 23:22:18, Julien Tinnes wrote: > I would remove the reference to the setuid sandbox. Now that this is in crypto/, > we really don't know if this will run sandboxed. > > Probably a comment in the interface file would be better suited. Done. https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.h File crypto/nss_util.h (right): https://chromiumcodereview.appspot.com/11411013/diff/8002/crypto/nss_util.h#n... crypto/nss_util.h:41: // sandboxed environment. On 2012/11/15 23:22:18, Julien Tinnes wrote: > I would maybe add a comment here along the lines of: > > "As a security in depth measure, this should be called in a sandboxed > environment to make sure that NSS will not load security modules that could > expose private data and keys. Please make sure to get a lgtm from security if > you use this." > > In any case, it's more appropriate to mention sandboxing here than in the > implementation. Done.
On 2012/11/15 23:17:12, Jorge Lucangeli Obes wrote: > On 2012/11/15 22:09:45, Jorge Lucangeli Obes wrote: > > On 2012/11/15 20:27:37, Ryan Sleevi wrote: > > > On 2012/11/15 19:40:28, Jorge Lucangeli Obes wrote: > > > > Taking over this one (https://codereview.chromium.org/11231021) since > Julien > > > is > > > > busy with other stuff. > > > > > > > > There's another NSS init in: > > > > > > > > > > https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... > > > > > > > > What makes the most sense is add crypto::DisableNSSForkCheck() to the > > wrapper > > > > and use the wrapper in both places. If people are happy with the wrapper, > > I'll > > > > add that to the CL. > > > > > > That SGTM > > > > Antoine, mind taking a look for OWNERS, or suggesting an appropriate OWNER? > > Antoine is out this week, John, would you mind taking a look? This CL explicitly > initializes NSS before enabling the seccomp-bpf sandbox on Linux and Chrome OS. jam@: and of course by the above I meant OWNERS rubberstamp.
On 2012/11/15 23:58:35, Jorge Lucangeli Obes wrote: > On 2012/11/15 23:17:12, Jorge Lucangeli Obes wrote: > > On 2012/11/15 22:09:45, Jorge Lucangeli Obes wrote: > > > On 2012/11/15 20:27:37, Ryan Sleevi wrote: > > > > On 2012/11/15 19:40:28, Jorge Lucangeli Obes wrote: > > > > > Taking over this one (https://codereview.chromium.org/11231021) since > > Julien > > > > is > > > > > busy with other stuff. > > > > > > > > > > There's another NSS init in: > > > > > > > > > > > > > > > https://cs.corp.google.com/#chrome/src/chrome/renderer/chrome_render_process_... > > > > > > > > > > What makes the most sense is add crypto::DisableNSSForkCheck() to the > > > wrapper > > > > > and use the wrapper in both places. If people are happy with the > wrapper, > > > I'll > > > > > add that to the CL. > > > > > > > > That SGTM > > > > > > Antoine, mind taking a look for OWNERS, or suggesting an appropriate OWNER? > > > > Antoine is out this week, John, would you mind taking a look? This CL > explicitly > > initializes NSS before enabling the seccomp-bpf sandbox on Linux and Chrome > OS. > > jam@: and of course by the above I meant OWNERS rubberstamp. Friendly ping for OWNERS rubberstamp.
redirecting to brett
On 2012/11/16 17:42:53, John Abd-El-Malek wrote: > redirecting to brett Thanks! Brett, this CL explicitly initializes NSS before enabling the Seccomp-BPF sandbox on Linux and Chrome OS, so that NSS is initialized correctly and we don't risk having issues with entropy.
ppapi lgtm with comment. https://codereview.chromium.org/11411013/diff/8/content/ppapi_plugin/ppapi_pl... File content/ppapi_plugin/ppapi_plugin_main.cc (right): https://codereview.chromium.org/11411013/diff/8/content/ppapi_plugin/ppapi_pl... content/ppapi_plugin/ppapi_plugin_main.cc:83: // Some out-of-process PPAPI plugins need access to NSS. Can this comment explicitly say that this needs to be called before sandbox initialization?
https://codereview.chromium.org/11411013/diff/8/content/ppapi_plugin/ppapi_pl... File content/ppapi_plugin/ppapi_plugin_main.cc (right): https://codereview.chromium.org/11411013/diff/8/content/ppapi_plugin/ppapi_pl... content/ppapi_plugin/ppapi_plugin_main.cc:83: // Some out-of-process PPAPI plugins need access to NSS. On 2012/11/16 19:09:59, brettw wrote: > Can this comment explicitly say that this needs to be called before sandbox > initialization? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/11411013/11003
Retried try job too often for step(s) interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/11411013/11003
Change committed as 168372
Patch set 5 LGTM. My complaints are all on the function name "WarmUpNSSSafely" and the header file comments that specify what it does. https://chromiumcodereview.appspot.com/11411013/diff/11003/crypto/nss_util.cc File crypto/nss_util.cc (right): https://chromiumcodereview.appspot.com/11411013/diff/11003/crypto/nss_util.cc... crypto/nss_util.cc:661: crypto::EnsureNSSInit(); Remove the crypto:: prefixes because this is inside the crypto namespace. https://chromiumcodereview.appspot.com/11411013/diff/11003/crypto/nss_util.h File crypto/nss_util.h (right): https://chromiumcodereview.appspot.com/11411013/diff/11003/crypto/nss_util.h#... crypto/nss_util.h:41: // sandboxed environment. Why do you call this "WarmUp" as opposed to "Init"? It's not clear what you meant by "safely". The second paragraph seems to say this means not loading any security modules, but still doesn't give a direct definition of "safely". I only fully understand what "safely" means after reading the .cc file. https://chromiumcodereview.appspot.com/11411013/diff/11003/crypto/nss_util.h#... crypto/nss_util.h:46: // if you use this. "Make sure to get an LGTM from Security if you use this" isn't clear. I guess "Security" means the Chrome Security Team? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
