Bug 173589

Created attachment 313382 [details] Patch

Created attachment 313387 [details] Patch

Wouldn't it be possible to initialize gcrypt once in the main function?

Comment on attachment 313387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313387&action=review Yeah, I agree with Carlos Garcia, there's got to be a better way to do this. If it has to be done in WebKit2 entry points, that would be unfortunate layering, but even that would seem preferable to adding such a huge number of initialization calls. There's another problem here: what about applications that use Libgcrypt themselves? [1] recommends ensuring that applications have not already initialized Libgcrypt before doing so. It also recommends enabling secure memory before initialization, to ensure crypto keys don't get swapped to disk. So I think PAL::GCrypt::initialize should do something like this: // Inside std::call_once and !gcry_check_version if (!gcry_control (GCRYCTL_INITIALIZATION_FINISHED_P)) { gcry_control (GCRYCTL_INIT_SECMEM, 16384, nullptr); gcry_control (GCRYCTL_INITIALIZATION_FINISHED, nullptr); } [1] https://www.gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html > Source/WebCore/PAL/pal/crypto/gcrypt/Utilities.cpp:43 > + if (!gcry_check_version("1.7.0")) This is going to be lost and forgotten whenever we update our min Libgcrypt version. I first thought we should remove the runtime check, but apparently Libgcrypt really wants this to happen first. So I would instead ensure it gets defined somehow by CMake (probably in config.cmake) so that the min version can be reused here instead of hardcoded.

(In reply to Michael Catanzaro from comment #4) > Comment on attachment 313387 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313387&action=review > > Yeah, I agree with Carlos Garcia, there's got to be a better way to do this. > If it has to be done in WebKit2 entry points, that would be unfortunate > layering, but even that would seem preferable to adding such a huge number > of initialization calls. > > There's another problem here: what about applications that use Libgcrypt > themselves? [1] recommends ensuring that applications have not already > initialized Libgcrypt before doing so. It also recommends enabling secure > memory before initialization, to ensure crypto keys don't get swapped to > disk. So I think PAL::GCrypt::initialize should do something like this: > This initialization is only needed in the WebProcess. Problem here would be injected bundles that use libgcrypt, but we'd initialize libgcrypt for ourselves before those libraries are loaded. Another aspect is any potential memory consumption penalty we'd have to pay when initializing libgcrypt early, even if it wouldn't be utilized at any point during the WebProcess lifetime. It might not be much, but I'll try to measure it. > // Inside std::call_once and !gcry_check_version > if (!gcry_control (GCRYCTL_INITIALIZATION_FINISHED_P)) { > gcry_control (GCRYCTL_INIT_SECMEM, 16384, nullptr); > gcry_control (GCRYCTL_INITIALIZATION_FINISHED, nullptr); > } > > [1] > https://www.gnupg.org/documentation/manuals/gcrypt/Initializing-the-library. > html > > > Source/WebCore/PAL/pal/crypto/gcrypt/Utilities.cpp:43 > > + if (!gcry_check_version("1.7.0")) > > This is going to be lost and forgotten whenever we update our min Libgcrypt > version. I first thought we should remove the runtime check, but apparently > Libgcrypt really wants this to happen first. So I would instead ensure it > gets defined somehow by CMake (probably in config.cmake) so that the min > version can be reused here instead of hardcoded.

*** Bug 173829 has been marked as a duplicate of this bug. ***

(In reply to Zan Dobersek from comment #5) > This initialization is only needed in the WebProcess. Problem here would be > injected bundles that use libgcrypt, but we'd initialize libgcrypt for > ourselves before those libraries are loaded. Good point. > Another aspect is any potential memory consumption penalty we'd have to pay > when initializing libgcrypt early, even if it wouldn't be utilized at any > point during the WebProcess lifetime. It might not be much, but I'll try to > measure it. Oh, that's true... still, surely there's a better way to handle this than putting tons of calls in lots of different places. I'd rather use a bit more memory than that. Do we have some IDL mechanism to call some sort of initialization function when a new subsystem is used? (Probably not?) Maybe someone on webkit-dev@ would have an idea?

(In reply to Michael Catanzaro from comment #7) > > Another aspect is any potential memory consumption penalty we'd have to pay > > when initializing libgcrypt early, even if it wouldn't be utilized at any > > point during the WebProcess lifetime. It might not be much, but I'll try to > > measure it. > > Oh, that's true... still, surely there's a better way to handle this than > putting tons of calls in lots of different places. I'd rather use a bit more > memory than that. Do we have some IDL mechanism to call some sort of > initialization function when a new subsystem is used? (Probably not?) Maybe > someone on webkit-dev@ would have an idea? Increased memory usage is probably negligible -- in whole about 108kB is used after the initialization. 16kB are added to the heap (when the secure memory is allocated, probably) and the remaining increase is due to additional libgcrypt.so section data getting mapped.

I presume you've added initialization of secure memory, since your original patch did not do that?

(In reply to Michael Catanzaro from comment #9) > I presume you've added initialization of secure memory, since your original > patch did not do that? Yes, and that's where likely the 16kB allocated on the heap come from. FWIW this was tested on a barebones program that only compiles main().

(In reply to Michael Catanzaro from comment #4) > > Source/WebCore/PAL/pal/crypto/gcrypt/Utilities.cpp:43 > > + if (!gcry_check_version("1.7.0")) > > This is going to be lost and forgotten whenever we update our min Libgcrypt > version. I first thought we should remove the runtime check, but apparently > Libgcrypt really wants this to happen first. So I would instead ensure it > gets defined somehow by CMake (probably in config.cmake) so that the min > version can be reused here instead of hardcoded. Using CMake is a bit difficult here, at this time. Macro definitions listed in cmakeconfig.h can only posses a value of 0 or 1. There's the GCRYPT_VERSION macro in gcrypt.h, but the comment there suggests it shouldn't be used by programs. But it seems the simplest way to ensure that the same version of library is used both during compilation and at runtime. But maybe we're just complicating things at this point. We should expect the runtime to match the configuration, where libgcrypt 1.7.0 is already guaranteed. So we could simply pass a nullptr value to the gcry_check_version() call and ignore the return value.

(In reply to Zan Dobersek from comment #11) > But maybe we're just complicating things at this point. We should expect the > runtime to match the configuration, where libgcrypt 1.7.0 is already > guaranteed. So we could simply pass a nullptr value to the > gcry_check_version() call and ignore the return value. OK, if that works it seems best.

Created attachment 313848 [details] Patch

Comment on attachment 313848 [details] Patch Much better.

Comment on attachment 313848 [details] Patch Clearing flags on attachment: 313848 Committed r218814: <http://trac.webkit.org/changeset/218814>

All reviewed patches have been landed. Closing bug.