It seems for me that the patches to random/ was written in old days.

• Now, we have getentropy in libc
  • This is most reliable one
  • better than urandom, because it may block when kernel is not yet seeded
  • better than random, because it never blocks once kernel is seeded
• So, the real path in rndlinux.c is actually, call to getentropy
• No access to /dev/random or /dev/urandom any more, in fact
• Although old code remains, non-touched
  • like use of syscall when getentropy function is not available

I'd like to know the purpose or original problems to solve by the patches to random/, to merge the work.

Can we focus on non-random/ parts at first, then come back to random/?
In this case, the problem to solve looks like:

• global_init and call of _gcry_fips_run_selftests under no_secure_memory=1

Fair enough. Unfortunately, the separation is not completely clear from the dist git history, so please, excuse any inaccuracies I will provide here. I will try to reference particular bugs so we can get back to them if needed:

• https://bugzilla.redhat.com/show_bug.cgi?id=1762741 -- this was RHEL 8.3 issue only when we did not call the selftests from constructor so this is not relevant, but one of the dead-end paths we took
• https://bugzilla.redhat.com/show_bug.cgi?id=1815083 (private) -- fips requirement to really call the self-tests from constructor (instead of delaying the selftests to the first use which tried to address the issue of blocking getrandom in constructor). No other useful information
• https://bugzilla.redhat.com/show_bug.cgi?id=1816291 (private) -- just systematic use of the /proc/sys/crypto/fips_enabled instead of /etc/system-fips
• https://bugzilla.redhat.com/show_bug.cgi?id=1706920 -- continuous random test (not covered in this changeset): Opened a new issue T5669
• https://bugzilla.redhat.com/show_bug.cgi?id=1172223 (private) -- prevent initialization of secure memory in constructor self-tests. This works partially right now because the random tests initialize the secure memory anyway.
• https://bugzilla.redhat.com/show_bug.cgi?id=1195850 -- The same for Fedora
• https://bugzilla.redhat.com/show_bug.cgi?id=1011566 (private) -- no useful information. Requirement to use NIST 800-90a RNG instead of X9.31. Handles also RNG initialization before self-tests
• https://bugzilla.redhat.com/show_bug.cgi?id=1038209 (private) -- need to run FIPS selftests in constructor. No much useful information
• https://bugzilla.redhat.com/show_bug.cgi?id=1380866 -- use getrandom if available and do not try to open /dev/urandom

Unfortunately, this does not cover all the thing that are done in the above patch. But if I ignore the the random changes, its really just moving the selftests from _gcry_initialize_fips_mode() to __constructor() because of the FIPS requirements. For that, the in_constructor part is not needed.

For the random stuff, I agree that some patches might no longer be relevant. Let me give an update on this later.

The most of the stuff about boot blocking was discussed in the bug https://bugzilla.redhat.com/show_bug.cgi?id=1569393 (private). There were some bugs in our patches, but also some issue in the kernel that locked the boot process (in FIPS mode).

Applying the part of the patch without random should already do most of the work. I broke down the patches to more self-contained parts:

• Running under no_secure_memory=1 does not help because the get_no_secure_memory() never returns 1 in fips mode so I reworked that part to be ignored in the constructor.
• when libgcrypt is used early in the boot (ex. from systemd), the constructor is called before filesystem is completely set up and the libgcrypt initialization fails because it can not find /dev/random and /dev/urandom (even though it does not need these as we have getrandom/getentropy syscall). This needs to be handled on several places as the random/rndlinux.c requires opening both of them anyway before going into checking syscalls. The same is probably a reason for changing defaults to urandom only if no configuration file is available.
• I changed the references to FIPS 140-3 as we are working on its support here and 140-2 version is no more relevant.
• Updating secmem test to not count with secure memory allocations from selftests.
• Deinitialization of random subystem.

Please, let me know if you will need some more clarifications either here or on the meeting next week.

Firstly, applied uncontroversial part in rC976673425784: doc: Reference the new FIPS 140-3

Let me clean up rndlinux.c for current use case, at first.

I think that it's better (for maintenance) to fix _gcry_rndlinux_gather_random to have two independent implementations (selected at build time):
(1) Use of getentropy (in libc, or system call by getrandom)
(2) Use of device

I think that fallback from (1) to (2) is now questionable and can be removed.

Then, things will be much cleaner.

We will have rnd-getentropy.c

I just wanted to add one more note that i just found out that the tests --disable-hwf or gcry_control GCRYCTL_DISABLE_HWF have no effect in case the global_init() is called from constructor.

In the documentation, I found:

• GCRYCTL_ENABLE_M_GUARD
• GCRYCTL_SET_PREFERRED_RNG_TYPE
• GCRYCTL_DISABLE_HWF

should/must be used before calling gcry_check_version. This doesn't good for the constructor doing automatic initialization.

Part 1 was applied. Part 3, Part 4, and Part 7 are irrelevant now, because we now have rndgetentropy which doesn't use device.

Thus, now, the patches in question are Part 2, Part 5 and Part 6.

Let us focus on Part 5, then, Part 2 and Part 6 are the ones for the constructor.

Current idea of mine is that, applying part 5, and merging those deinit routines into _gcry_random_close_fds. That will extend the semantics of GCRYCTL_CLOSE_RANDOM_DEVICE.

The intended use case of GCRYCTL_CLOSE_RANDOM_DEVICE is things after fork.
So, I think that the semantics change is natural, and actually a bit better (less sharing between parent and child).

Thank you. Extending the semantics of GCRYCTL_CLOSE_RANDOM_DEVICE sounds good to me. I think the deinit functions were created initially especially not to change the semantics of existing code using GCRYCTL_CLOSE_RANDOM_DEVICE, but I agree that it will probably not be an issue.

I did go through a bit more testing too and the selftests still initialize and use the secure memory (and the t-secmem fails in FIPS mode if we invoke selftests from constructor). Now from run_random_selftests() -> _gcry_random_selftest() -> drbg_healthcheck() -> _gcry_rngdrbg_healthcheck_one(). So this means that we either need to de-initialize secure memory after the constructor selftests or prevent its initialization as I suggested in some of the previous comments.

Thank you for testing.

I thing that we are getting close to the last step(s) of disentangling things around the constructor.

Let me proceed two things:
(1) factoring the system/process check for FIPS (the environment variable and file), so that evaluating the change will be easier
(2) Fix DRBG implementation to release forgotten resource release

IIUC, one of the causes for the failure of secmem was resource release of DRBG memory.

@Jakuje , please rebase the constructor patch by rC751fcadd34ed: random: Release memory in DRBG.

And please let me know the change rC751fcadd34ed: random: Release memory in DRBG. affects t-secmem failure.

Thank you. My local tests (in emulated fips mode and normal mode) do not show any errors with current master branch.

The only remaining relevant change that I have in my branch now is the following, but I believe this one is no longer needed on linux as we are using the getentropy provider.

commit 58ede58a6fcd5933efbd0fa9503e70818721d40c
Author: Jakub Jelen <jjelen@redhat.com>
Date:   Tue Nov 2 19:44:53 2021 +0100

    random: If no configuration file is available use urandom only
    
    * random/random.c (_gcry_random_read_conf): Use urandom only by default.
    --
    
    Signed-off-by: Jakub Jelen <jjelen@redhat.com>

diff --git a/random/random.c b/random/random.c
index 9aab7893..e98fe791 100644
--- a/random/random.c
+++ b/random/random.c
@@ -110,8 +110,8 @@ _gcry_random_read_conf (void)
   unsigned int result = 0;
 
   fp = fopen (fname, "r");
-  if (!fp)
-    return result;
+  if (!fp) /* We make only_urandom the default. */
+    return RANDOM_CONF_ONLY_URANDOM;
 
   for (;;)
     {

Before rebasing, I pushed a change to simplify access to no_secure_memory variable by rC209d98dcf66b: Simplify the logic for no_secure_memory..

Here is the change remained:

diff --git a/src/fips.c b/src/fips.c
index bcadc5f2..5499aee8 100644
--- a/src/fips.c
+++ b/src/fips.c
@@ -82,6 +82,12 @@ static void fips_new_state (enum module_states new_state);
 
 
 
+/*
+ * Returns 1 if the FIPS mode is to be activated based on the
+ * environment variable LIBGCRYPT_FORCE_FIPS_MODE, the file defined by
+ * FIPS_FORCE_FILE, or /proc/sys/crypto/fips_enabled.
+ * This function aborts on misconfigured filesystems.
+ */
 static int
 check_fips_system_setting (void)
 {
@@ -136,6 +142,17 @@ check_fips_system_setting (void)
   return 0;
 }
 
+/*
+ * Initial check if the FIPS mode should be activated on startup.
+ * Called by the constructor at the initialization of the library.
+ */
+int
+_gcry_fips_to_activate (void)
+{
+  return check_fips_system_setting ();
+}
+
+
 /* Check whether the OS is in FIPS mode and record that in a module
    local variable.  If FORCE is passed as true, fips mode will be
    enabled anyway. Note: This function is not thread-safe and should
diff --git a/src/g10lib.h b/src/g10lib.h
index d2e718a5..c12cfef3 100644
--- a/src/g10lib.h
+++ b/src/g10lib.h
@@ -425,6 +425,7 @@ gpg_err_code_t _gcry_sexp_vextract_param (gcry_sexp_t sexp, const char *path,
 extern int _gcry_no_fips_mode_required;
 
 void _gcry_initialize_fips_mode (int force);
+int _gcry_fips_to_activate (void);
 
 /* This macro returns true if fips mode is enabled.  This is
    independent of the fips required finite state machine and only used
diff --git a/src/global.c b/src/global.c
index 47dd80ec..ad2e95a4 100644
--- a/src/global.c
+++ b/src/global.c
@@ -140,6 +140,25 @@ global_init (void)
   BUG ();
 }
 
+#ifdef ENABLE_HMAC_BINARY_CHECK
+# if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 7 )
+# define GCC_ATTR_CONSTRUCTOR  __attribute__ ((__constructor__))
+
+static void GCC_ATTR_CONSTRUCTOR
+_gcry_global_constructor (void)
+{
+  force_fips_mode = _gcry_fips_to_activate ();
+  if (force_fips_mode)
+    {
+      no_secure_memory = 1;
+      global_init ();
+      _gcry_fips_run_selftests (0);
+      _gcry_random_close_fds ();
+      no_secure_memory = 0;
+    }
+}
+# endif
+#endif /* ENABLE_HMAC_BINARY_CHECK */
 
 /* This function is called by the macro fips_is_operational and makes
    sure that the minimal initialization has been done.  This is far

It is simplified (no in_constructor any more, just no_secure_memory).
The constructor is only enabled with ENABLE_HMAC_BINARY_CHECK, to be conservative (less surprise for other use cases than FIPS).

Thank you. Tested locally that it does what it is supposed to do and all tests passed for me as expected.

Thank you for your quick testing.

Applied and pushed the change.

I sorted out things in the original patch and remove unnecessary interactions by in_constructor and no_secure_memory, while keeping the exact same behavior (not using secure memory for the selftests from constructor). I believe that the minimum difference between FIPS mode and normal code path makes auditors happy.

Please note that the constructor (for C) is non-portable GCC feature (Clang also supports that, though).

I investigated the history of GCC, and I found that __constructor__ attribute feature was added in 2.7.0 release of GCC.