Date:   Thu, 08 Apr 2021 18:16:00 -0400
From:   Simo Sorce <simo@...hat.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Hangbin Liu <liuhangbin@...il.com>,
        Netdev <netdev@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Ondrej Mosnacek <omosnace@...hat.com>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>
Subject: Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

On Thu, 2021-04-08 at 15:55 -0600, Jason A. Donenfeld wrote:
> On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce <simo@...hat.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.

I guess that moving the fips check down at the algorithms level is a
valid option. There are some cases that will be a little iffy though,
like when only certain key sizes cannot be accepted, but for the
wireguard case it would be clean.

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

For certification purposes wireguard is not part of the module boundary
(speaking only for my company in this case).

But that is not the issue.

There is an expectation by customers that, when the kernel is in fips
mode, non-approved cryptography is not used (given those customers are
required by law/regulation to use only approved/certified
cryptography).
So we still have a strong desire, where possible, to not allow the
kernel to use non-certified cryptography, regardless of what is the
crypto module boundary (we do the same in user space).

HTH,
Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Powered by blists - more mailing lists