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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce <simo@redhat.com> wrote:
> > 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