WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(28.27 KB, patch)
2017-06-20 02:33 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(2.21 KB, patch)
2017-06-26 09:51 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2017-06-20 01:58:19 PDT
Created
attachment 313382
[details]
Patch
Zan Dobersek
Comment 2
2017-06-20 02:33:06 PDT
Created
attachment 313387
[details]
Patch
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
Created
attachment 313848
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug