RESOLVED FIXED 173589
[GCrypt] Properly initialize libgcrypt before using it
https://bugs.webkit.org/show_bug.cgi?id=173589
Summary [GCrypt] Properly initialize libgcrypt before using it
Zan Dobersek
Reported 2017-06-20 01:45:36 PDT
[GCrypt] Properly initialize libgcrypt before using it
Attachments
Patch (27.84 KB, patch)
2017-06-20 01:58 PDT, Zan Dobersek
no flags
Patch (28.27 KB, patch)
2017-06-20 02:33 PDT, Zan Dobersek
no flags
Patch (2.21 KB, patch)
2017-06-26 09:51 PDT, Zan Dobersek
no flags
Zan Dobersek
Comment 1 2017-06-20 01:58:19 PDT
Zan Dobersek
Comment 2 2017-06-20 02:33:06 PDT
Carlos Garcia Campos
Comment 3 2017-06-22 00:39:20 PDT
Wouldn't it be possible to initialize gcrypt once in the main function?
Michael Catanzaro
Comment 4 2017-06-23 17:14:41 PDT
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.
Zan Dobersek
Comment 5 2017-06-26 02:30:09 PDT
(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.
Fujii Hironori
Comment 6 2017-06-26 03:18:53 PDT
*** Bug 173829 has been marked as a duplicate of this bug. ***
Michael Catanzaro
Comment 7 2017-06-26 07:59:45 PDT
(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?
Zan Dobersek
Comment 8 2017-06-26 08:21:19 PDT
(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.
Michael Catanzaro
Comment 9 2017-06-26 09:14:07 PDT
I presume you've added initialization of secure memory, since your original patch did not do that?
Zan Dobersek
Comment 10 2017-06-26 09:25:59 PDT
(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().
Zan Dobersek
Comment 11 2017-06-26 09:43:27 PDT
(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.
Michael Catanzaro
Comment 12 2017-06-26 09:46:52 PDT
(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.
Zan Dobersek
Comment 13 2017-06-26 09:51:27 PDT
Michael Catanzaro
Comment 14 2017-06-26 10:30:34 PDT
Comment on attachment 313848 [details] Patch Much better.
Zan Dobersek
Comment 15 2017-06-26 11:01:24 PDT
Comment on attachment 313848 [details] Patch Clearing flags on attachment: 313848 Committed r218814: <http://trac.webkit.org/changeset/218814>
Zan Dobersek
Comment 16 2017-06-26 11:01:29 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.