Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210407113920.3735505-1-liuhangbin@gmail.com>
 <CAHmME9p40M5oHDZXnFDXfO4-JuJ7bUB5BnsccGV1pksguz73sg@mail.gmail.com> <c47d99b9d0efeea4e6cd238c2affc0fbe296b53c.camel@redhat.com>
In-Reply-To: <c47d99b9d0efeea4e6cd238c2affc0fbe296b53c.camel@redhat.com>
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
Date:   Thu, 8 Apr 2021 15:55:59 -0600
Message-ID: <CAHmME9pRSOANrdvegLm9x8VTNWKcMtoymYrgStuSx+nsu=jpwA@mail.gmail.com>
Subject: Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
To:     Simo Sorce <simo@redhat.com>
Cc:     Hangbin Liu <liuhangbin@gmail.com>,
        Netdev <netdev@vger.kernel.org>,
        =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Ondrej Mosnacek <omosnace@redhat.com>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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