Move OpenSSL random under USE_OPENSSL_RANDOM
The USE_OPENSSL_RANDOM macro is defined when OpenSSL is used as a randomness
provider, but the implementation of strong randomness is guarded by USE_OPENSSL
in most places. This is technically the same thing today, but it seems
hygienic to use the appropriate macro in case we ever want to allow OS
randomness together with OpenSSL or something similar (or just make git grep
easier which is my itch to scratch with this).
The attached moves all invocations under the correct guards. RAND_poll() in
fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
check for both.
cheers ./daniel
Attachments:
openssl_random_macros.patchapplication/octet-stream; name=openssl_random_macros.patch; x-unix-mode=0644Download+3-4
On Tue, Aug 25, 2020 at 03:52:14PM +0200, Daniel Gustafsson wrote:
The USE_OPENSSL_RANDOM macro is defined when OpenSSL is used as a randomness
provider, but the implementation of strong randomness is guarded by USE_OPENSSL
in most places. This is technically the same thing today, but it seems
hygienic to use the appropriate macro in case we ever want to allow OS
randomness together with OpenSSL or something similar (or just make git grep
easier which is my itch to scratch with this).
@@ -24,7 +24,7 @@
#include <unistd.h>
#include <sys/time.h>
-#ifdef USE_OPENSSL
+#ifdef USE_OPENSSL_RANDOM
#include <openssl/rand.h>
#endif
I agree that this makes the header declarations more consistent with
WIN32.
The attached moves all invocations under the correct guards. RAND_poll() in
fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
check for both.
Yeah, it could be possible that somebody still calls RAND_bytes() or
similar without going through pg_strong_random(), so we still need to
use USE_OPENSSL after forking. Per this argument, I am not sure I see
the point of the change in fork_process.c as it seems to me that
USE_OPENSSL_RANDOM should only be tied to pg_strong_random.c, and
you'd still get a compilation failure if trying to use
USE_OPENSSL_RANDOM without --with-openssl.
--
Michael
On 26 Aug 2020, at 09:56, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 25, 2020 at 03:52:14PM +0200, Daniel Gustafsson wrote:
The attached moves all invocations under the correct guards. RAND_poll() in
fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
check for both.Yeah, it could be possible that somebody still calls RAND_bytes() or
similar without going through pg_strong_random(), so we still need to
use USE_OPENSSL after forking. Per this argument, I am not sure I see
the point of the change in fork_process.c as it seems to me that
USE_OPENSSL_RANDOM should only be tied to pg_strong_random.c, and
you'd still get a compilation failure if trying to use
USE_OPENSSL_RANDOM without --with-openssl.
That's certainly true. The intention though is to make the code easier to
follow (more explicit/discoverable) for anyone trying to implement support for
TLS backends. It's a very git grep intense process already as it is.
cheers ./daniel
On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 26 Aug 2020, at 09:56, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 25, 2020 at 03:52:14PM +0200, Daniel Gustafsson wrote:The attached moves all invocations under the correct guards. RAND_poll() in
fork_process.c needs to happen for both OpenSSL and OpenSSL random, thus the
check for both.Yeah, it could be possible that somebody still calls RAND_bytes() or
similar without going through pg_strong_random(), so we still need to
use USE_OPENSSL after forking. Per this argument, I am not sure I see
the point of the change in fork_process.c as it seems to me that
USE_OPENSSL_RANDOM should only be tied to pg_strong_random.c, and
you'd still get a compilation failure if trying to use
USE_OPENSSL_RANDOM without --with-openssl.That's certainly true. The intention though is to make the code easier to
follow (more explicit/discoverable) for anyone trying to implement support for
Is it really a reasonable usecase to use RAND_bytes() outside of both
pg_stroing_random() *and' outside of the openssl-specific files (like
be-secure-openssl.c)? Because it would only be those cases that would
have this case, right?
If anything, perhaps the call to RAND_poll() in fork_process.c should
actually be a call to a strong_random_initialize() or something which
would have an implementation in pg_strong_random.c, thereby isolating
the openssl specific code in there? (And with a void implementation
without openssl)
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Tue, Nov 03, 2020 at 10:15:48AM +0100, Magnus Hagander wrote:
On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
That's certainly true. The intention though is to make the code easier to
follow (more explicit/discoverable) for anyone trying to implement support forIs it really a reasonable usecase to use RAND_bytes() outside of both
pg_stroing_random() *and' outside of the openssl-specific files (like
be-secure-openssl.c)? Because it would only be those cases that would
have this case, right?
It does not sound that strange to me to assume if some out-of-core
code makes use of that to fetch a random set of bytes. Now I don't
know of any code doing that. Who knows.
If anything, perhaps the call to RAND_poll() in fork_process.c should
actually be a call to a strong_random_initialize() or something which
would have an implementation in pg_strong_random.c, thereby isolating
the openssl specific code in there? (And with a void implementation
without openssl)
I don't think that we have any need to go to such extent just for this
case, as RAND_poll() after forking a process is irrelevant in 1.1.1.
We are still many years away from removing its support though.
No idea if other SSL implementations would require such a thing.
Daniel, what about NSS?
--
Michael
On 3 Nov 2020, at 11:35, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Nov 03, 2020 at 10:15:48AM +0100, Magnus Hagander wrote:
On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
That's certainly true. The intention though is to make the code easier to
follow (more explicit/discoverable) for anyone trying to implement support forIs it really a reasonable usecase to use RAND_bytes() outside of both
pg_stroing_random() *and' outside of the openssl-specific files (like
be-secure-openssl.c)? Because it would only be those cases that would
have this case, right?It does not sound that strange to me to assume if some out-of-core
code makes use of that to fetch a random set of bytes. Now I don't
know of any code doing that. Who knows.
Even if there are, I doubt such codepaths will be stumped by using
USE_OPENSSL_RANDOM for pg_strong_random code as opposed to USE_OPENSSL.
If anything, perhaps the call to RAND_poll() in fork_process.c should
actually be a call to a strong_random_initialize() or something which
would have an implementation in pg_strong_random.c, thereby isolating
the openssl specific code in there? (And with a void implementation
without openssl)I don't think that we have any need to go to such extent just for this
case, as RAND_poll() after forking a process is irrelevant in 1.1.1.
We are still many years away from removing its support though.
Agreed, I doubt we'll be able to retire our <1.1.1 suppport any time soon.
No idea if other SSL implementations would require such a thing.
Daniel, what about NSS?
PK11_GenerateRandom in NSS requires an NSSContext to be set up after fork,
which could be performed in such an _initialize function. The PRNG in NSPR has
a similar requirement (which may be the one the NSS patch end up using, not
sure yet).
I kind of like the idea of continuing to abstract this functionality, not
pulling in OpenSSL headers in fork_process.c is a neat bonus. I'd say it's
worth implementing to see what it would imply, and am happy to do unless
someone beats me to it.
cheers ./daniel
On Tue, Nov 3, 2020 at 1:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 3 Nov 2020, at 11:35, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Nov 03, 2020 at 10:15:48AM +0100, Magnus Hagander wrote:
On Wed, Aug 26, 2020 at 2:19 PM Daniel Gustafsson <daniel@yesql.se> wrote:
That's certainly true. The intention though is to make the code easier to
follow (more explicit/discoverable) for anyone trying to implement support forIs it really a reasonable usecase to use RAND_bytes() outside of both
pg_stroing_random() *and' outside of the openssl-specific files (like
be-secure-openssl.c)? Because it would only be those cases that would
have this case, right?It does not sound that strange to me to assume if some out-of-core
code makes use of that to fetch a random set of bytes. Now I don't
know of any code doing that. Who knows.Even if there are, I doubt such codepaths will be stumped by using
USE_OPENSSL_RANDOM for pg_strong_random code as opposed to USE_OPENSSL.If anything, perhaps the call to RAND_poll() in fork_process.c should
actually be a call to a strong_random_initialize() or something which
would have an implementation in pg_strong_random.c, thereby isolating
the openssl specific code in there? (And with a void implementation
without openssl)I don't think that we have any need to go to such extent just for this
case, as RAND_poll() after forking a process is irrelevant in 1.1.1.
We are still many years away from removing its support though.Agreed, I doubt we'll be able to retire our <1.1.1 suppport any time soon.
No idea if other SSL implementations would require such a thing.
Daniel, what about NSS?PK11_GenerateRandom in NSS requires an NSSContext to be set up after fork,
which could be performed in such an _initialize function. The PRNG in NSPR has
a similar requirement (which may be the one the NSS patch end up using, not
sure yet).I kind of like the idea of continuing to abstract this functionality, not
pulling in OpenSSL headers in fork_process.c is a neat bonus. I'd say it's
worth implementing to see what it would imply, and am happy to do unless
someone beats me to it.
Yeah, if it's likely to be usable in the other implementations, then I
think we should definitely explore exactly what that kind of an
abstraction would imply. Anything isolating the dependency on OpenSSL
would likely have to be done at that time anyway in that case, so
better have it ready.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Tue, Nov 03, 2020 at 01:46:38PM +0100, Magnus Hagander wrote:
On Tue, Nov 3, 2020 at 1:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
I kind of like the idea of continuing to abstract this functionality, not
pulling in OpenSSL headers in fork_process.c is a neat bonus. I'd say it's
worth implementing to see what it would imply, and am happy to do unless
someone beats me to it.Yeah, if it's likely to be usable in the other implementations, then I
think we should definitely explore exactly what that kind of an
abstraction would imply. Anything isolating the dependency on OpenSSL
would likely have to be done at that time anyway in that case, so
better have it ready.
With the NSS argument, agreed. Documenting when this initialization
routine is used is important. And I think that we should force to
look at this code when adding a new SSL implementation to make sure
that we never see CVE-2013-1900 again, say:
void
pg_strong_random_init(void)
{
#ifdef USE_SSL
#ifdef USE_OPENSSL
RAND_poll();
#elif USE_NSS
/* do the NSS initialization */
#else
Hey, you are missing something here.
#endif
#endif
}
--
Michael
On Wed, Nov 4, 2020 at 2:01 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Nov 03, 2020 at 01:46:38PM +0100, Magnus Hagander wrote:
On Tue, Nov 3, 2020 at 1:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
I kind of like the idea of continuing to abstract this functionality, not
pulling in OpenSSL headers in fork_process.c is a neat bonus. I'd say it's
worth implementing to see what it would imply, and am happy to do unless
someone beats me to it.Yeah, if it's likely to be usable in the other implementations, then I
think we should definitely explore exactly what that kind of an
abstraction would imply. Anything isolating the dependency on OpenSSL
would likely have to be done at that time anyway in that case, so
better have it ready.With the NSS argument, agreed. Documenting when this initialization
routine is used is important. And I think that we should force to
look at this code when adding a new SSL implementation to make sure
that we never see CVE-2013-1900 again, say:
void
pg_strong_random_init(void)
{
#ifdef USE_SSL
#ifdef USE_OPENSSL
RAND_poll();
#elif USE_NSS
/* do the NSS initialization */
#else
Hey, you are missing something here.
#endif
#endif
}
Yes, we should absolutely do that. We already do this for
pg_strong_random() itself, and we should definitely repeat the pattern
in the init function.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Wed, Nov 04, 2020 at 10:05:48AM +0100, Magnus Hagander wrote:
Yes, we should absolutely do that. We already do this for
pg_strong_random() itself, and we should definitely repeat the pattern
in the init function.
This poked at my curiosity, so I looked at it. The result looks
indeed like an improvement to me, while taking care of the point of
upthread to make the implementation stuff controlled only by
USE_OPENSSL_RANDOM. Per se the attached.
This could make random number generation predictible when an extension
calls directly RAND_bytes() if USE_OPENSSL_RANDOM is not used while
building with OpenSSL, but perhaps I am just too much of a pessimistic
nature.
--
Michael
Attachments:
openssl_random_macros_v2.patchtext/x-diff; charset=us-asciiDownload+37-12
On 5 Nov 2020, at 04:44, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 04, 2020 at 10:05:48AM +0100, Magnus Hagander wrote:
Yes, we should absolutely do that. We already do this for
pg_strong_random() itself, and we should definitely repeat the pattern
in the init function.This poked at my curiosity, so I looked at it. The result looks
indeed like an improvement to me, while taking care of the point of
upthread to make the implementation stuff controlled only by
USE_OPENSSL_RANDOM. Per se the attached.
This must check for USE_OPENSSL as well as per my original patch, since we'd
otherwise fail to perform post-fork initialization in case one use OpenSSL with
anothe PRNG for pg_strong_random. That might be theoretical at this point, but
if we ever support that and miss updating this it would be problematic.
+#if defined(USE_OPENSSL_RANDOM)
I'm not sure this comment adds any value, we currently have two non-TLS library
PRNGs in pg_strong_random, so even if we add NSS it will at best be 50%:
+ * Note that this applies normally to SSL implementations, so when
+ * implementing a new one, be careful to consider this initialization
+ * step.This could make random number generation predictible when an extension
calls directly RAND_bytes() if USE_OPENSSL_RANDOM is not used while
building with OpenSSL, but perhaps I am just too much of a pessimistic
nature.
Any extension blindly calling RAND_bytes and expecting there to automagically
be an OpenSSL library available seems questionable.
cheers ./daniel
On Thu, Nov 05, 2020 at 10:49:45AM +0100, Daniel Gustafsson wrote:
This must check for USE_OPENSSL as well as per my original patch, since we'd
otherwise fail to perform post-fork initialization in case one use OpenSSL with
anothe PRNG for pg_strong_random. That might be theoretical at this point, but
if we ever support that and miss updating this it would be problematic.
That's actually the same point I tried to make at the end of my last
email, but worded differently, isn't it? In short we have
USE_OPENSSL, but !USE_OPENSSL_RANDOM and we still need an
initialization. We could just do something like the following:
#ifdef USE_OPENSSL
RAND_poll();
#endif
#if defined(USE_OPENSSL_RANDOM)
/* OpenSSL is done above, because blah.. */
#elif etc..
[...]
#error missing an init, pal.
#endif
Or do you jave something else in mind?
+#if defined(USE_OPENSSL_RANDOM)
I'm not sure this comment adds any value, we currently have two non-TLS library
PRNGs in pg_strong_random, so even if we add NSS it will at best be 50%:
I don't mind removing this part, the compilation hint may be enough,
indeed.
--
Michael
On 5 Nov 2020, at 13:12, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 05, 2020 at 10:49:45AM +0100, Daniel Gustafsson wrote:
This must check for USE_OPENSSL as well as per my original patch, since we'd
otherwise fail to perform post-fork initialization in case one use OpenSSL with
anothe PRNG for pg_strong_random. That might be theoretical at this point, but
if we ever support that and miss updating this it would be problematic.That's actually the same point I tried to make at the end of my last
email, but worded differently, isn't it?
Ah, ok, then I failed to parse it that way. At least we are in agreement then
which is good.
In short we have
USE_OPENSSL, but !USE_OPENSSL_RANDOM and we still need an
initialization. We could just do something like the following:
#ifdef USE_OPENSSL
RAND_poll();
#endif
#if defined(USE_OPENSSL_RANDOM)
/* OpenSSL is done above, because blah.. */
#elif etc..
[...]
#error missing an init, pal.
#endifOr do you jave something else in mind?
What about the (hypothetical) situation where USE_OPENSSL_RANDOM is used
without USE_OPENSSL? Wouldn't the below make sure we cover all bases?
#if defined(USE_OPENSSL) || defined(USE_OPENSSL_RANDOM)
cheers ./daniel
On Thu, Nov 05, 2020 at 01:18:15PM +0100, Daniel Gustafsson wrote:
What about the (hypothetical) situation where USE_OPENSSL_RANDOM is used
without USE_OPENSSL? Wouldn't the below make sure we cover all bases?
You can actually try that combination, because it is possible today to
compile without --with-openssl but try to enforce USE_OPENSSL_RANDOM.
This leads to a compilation failure. I think that it is important to
have the #if/#elif business in the init function match the conditions
of the main function.
#if defined(USE_OPENSSL) || defined(USE_OPENSSL_RANDOM)
It seems to me that this one would become incorrect if compiling with
OpenSSL but select a random source that requires an initialization, as
it would enforce only OpenSSL initialization all the time.
Theoretical point now, of course, because such combination does not
exist yet in the code.
--
Michael
On Thu, Nov 5, 2020 at 1:28 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 05, 2020 at 01:18:15PM +0100, Daniel Gustafsson wrote:
What about the (hypothetical) situation where USE_OPENSSL_RANDOM is used
without USE_OPENSSL? Wouldn't the below make sure we cover all bases?You can actually try that combination, because it is possible today to
compile without --with-openssl but try to enforce USE_OPENSSL_RANDOM.
This leads to a compilation failure. I think that it is important to
have the #if/#elif business in the init function match the conditions
of the main function.
+1 -- whatever those are, they should be the same.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On 5 Nov 2020, at 13:28, Michael Paquier <michael@paquier.xyz> wrote:
It seems to me that this one would become incorrect if compiling with
OpenSSL but select a random source that requires an initialization, as
it would enforce only OpenSSL initialization all the time.
Right, how about something like the attached (untested) diff?
Theoretical point now, of course, because such combination does not
exist yet in the code.
Not yet, and potentially never will. Given the consequences of a PRNG which
hasn't been properly initialized I think it's ok to be defensive in this
codepath however.
cheers ./daniel
Attachments:
openssl_random_macros-v3.patchapplication/octet-stream; name=openssl_random_macros-v3.patch; x-unix-mode=0644Download+48-12
On Thu, Nov 05, 2020 at 01:59:11PM +0100, Daniel Gustafsson wrote:
Not yet, and potentially never will. Given the consequences of a PRNG which
hasn't been properly initialized I think it's ok to be defensive in this
codepath however.
+ /*
+ * In case the backend is using the PRNG from OpenSSL without being built
+ * with support for OpenSSL, make sure to perform post-fork initialization.
+ * If the backend is using OpenSSL then we have already performed this
+ * step. The same version caveat as discussed in the comment above applies
+ * here as well.
+ */
+#ifndef USE_OPENSSL
+ RAND_poll();
+#endifI still don't see the point of this extra complexity, as
USE_OPENSSL_RANDOM implies USE_OPENSSL, and we also call RAND_poll() a
couple of lines down in the main function under USE_OPENSSL_RANDOM.
So I would just remove this whole block, and replace the comment by a
simple "initialization already done above".
--
Michael
On 6 Nov 2020, at 00:36, Michael Paquier <michael@paquier.xyz> wrote:
I still don't see the point of this extra complexity, as
USE_OPENSSL_RANDOM implies USE_OPENSSL,
As long as we're sure that we'll remember to fix this when that assumption no
longer holds (intentional or unintentional) then it's fine to skip and instead
be defensive in documentation rather than code.
cheers ./daniel
On Fri, Nov 6, 2020 at 12:08 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 6 Nov 2020, at 00:36, Michael Paquier <michael@paquier.xyz> wrote:
I still don't see the point of this extra complexity, as
USE_OPENSSL_RANDOM implies USE_OPENSSL,As long as we're sure that we'll remember to fix this when that assumption no
longer holds (intentional or unintentional) then it's fine to skip and instead
be defensive in documentation rather than code.
I think the defensive-in-code instead of defensive-in-docs is a really
strong argument, so I have pushed it as such.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
