Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce [off-list ref] wrote:

quoted

I'm not sure this makes so much sense to do _in wireguard_. If you
feel like the FIPS-allergic part is actually blake, 25519, chacha, and
poly1305, then wouldn't it make most sense to disable _those_ modules
instead? And then the various things that rely on those (such as
wireguard, but maybe there are other things too, like
security/keys/big_key.c) would be naturally disabled transitively?

The reason why it is better to disable the whole module is that it
provide much better feedback to users. Letting init go through and then
just fail operations once someone tries to use it is much harder to
deal with in terms of figuring out what went wrong.
Also wireguard seem to be poking directly into the algorithms
implementations and not use the crypto API, so disabling individual
algorithm via the regular fips_enabled mechanism at runtime doesn't
work.

What I'm suggesting _would_ work in basically the exact same way as
this patch. Namely, something like:

diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
index 288a62cd29b2..b794f49c291a 100644
--- a/lib/crypto/curve25519.c
+++ b/lib/crypto/curve25519.c

@@ -12,11 +12,15 @@
 #include <crypto/curve25519.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/fips.h>

 bool curve25519_selftest(void);

 static int __init mod_init(void)
 {
+ if (!fips_enabled)
+ return -EOPNOTSUPP;
+
  if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
      WARN_ON(!curve25519_selftest()))
  return -ENODEV;

Making the various lib/crypto/* modules return EOPNOTSUPP will in turn
mean that wireguard will refuse to load, due to !fips_enabled. It has
the positive effect that all other things that use it will also be
EOPNOTSUPP.

For example, what are you doing about big_key.c? Right now, I assume
nothing. But this way, you'd get all of the various effects for free.
Are you going to continuously audit all uses of non-FIPS crypto and
add `if (!fips_enabled)` to every new use case, always, everywhere,
from now into the boundless future? By adding `if (!fips_enabled)` to
wireguard, that's what you're signing yourself up for. Instead, by
restricting the lib/crypto/* modules to !fips_enabled, you can get all
of those transitive effects without having to do anything additional.

Alternatively, I agree with Eric - why not just consider this outside
your boundary?

Jason