Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, 2021-04-09 at 14:56 -0400, Simo Sorce wrote: > Hi Jason, > I can't speak for Hangbin, we do not work for the same company and I > was not aware of his efforts until this patch landed. Turns out I and Hangbin do work for the same company after all. Left hand is meeting right hand internally now. :-D The comments still stand of course. Simo. > For my part we were already looking at big_key, wireguard and other > areas internally, but were not thinking of sending upstream patches > like these w/o first a good assessment with our teams and lab that they > were proper and sufficient. > > > So > > I think either you should send an exhaustive patch series that forbids > > all use of non-FIPS crypto anywhere in the kernel (another example: > > net/core/secure_seq.c) in addition to all tunneling modules that don't > > use FIPS-certified crypto, or figure out how to disable the lib/crypto > > primitives that you want to be disabled in "fips mode". With a > > coherent patchset for either of these, we can then evaluate it. > > Yes a cohesive approach would be ideal, but I do not know if pushing > substantially the same checks we have in the Crypto API down to > lib/crypto is the right way to go, I am not oppose but I guess Herbert > would have to chime in here. > -- Simo Sorce RHEL Crypto Team Red Hat, Inc
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, Apr 09, 2021 at 12:29:42PM -0600, Jason A. Donenfeld wrote: > On Fri, Apr 9, 2021 at 2:08 AM Hangbin Liu wrote: > > After offline discussion with Herbert, here is > > what he said: > > > > """ > > This is not a problem in RHEL8 because the Crypto API RNG replaces > > /dev/random > > in FIPS mode. > > """ > > So far as I can see, this isn't the case in the kernel sources I'm > reading? Maybe you're doing some userspace hack with CUSE? But at > least get_random_bytes doesn't behave this way... > > I'm not familiar with this code, not sure how upstream handle this. Hi Jason, As I said, I'm not familiar with this part of code. If upstream does not handle this correctly, sure this is an issue and need to be fixed. And as Simo said, he is also working on this part. I will talk with him and Herbert and see if we can have a more proper fix. Feel free to drop this patch. Thanks Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, 2021-04-09 at 12:36 -0600, Jason A. Donenfeld wrote: > On Fri, Apr 9, 2021 at 6:47 AM Simo Sorce wrote: > > > depends on m || !CRYPTO_FIPS > > > > > > but I am a bit concerned that the rather intricate kconfig > > > dependencies between the generic and arch-optimized versions of those > > > drivers get complicated even further. > > > > Actually this is the opposite direction we are planning to go for > > future fips certifications. > > > > Due to requirements about crypto module naming and versioning in the > > new FIPS-140-3 standard we are planning to always build all the CRYPTO > > as bultin (and maybe even forbid loading additional crypto modules in > > FIPS mode). This is clearly just a vendor choice and has no bearing on > > what upstream ultimately will do, but just throwing it here as a data > > point. > > I'm wondering: do you intend to apply similar patches to all the other > uses of "non-FIPS-certified" crypto in the kernel? I've already > brought up big_key.c, for example. Also if you're intent on adding > this check to WireGuard, because it tunnels packets without using > FIPS-certified crypto primitives, do you also plan on adding this > check to other network tunnels that don't tunnel packets using > FIPS-certified crypto primitives? For example, GRE, VXLAN, GENEVE? I'd > be inclined to take this patch more seriously if it was exhaustive and > coherent for your use case. The targeted hit on WireGuard seems > incoherent as a standalone patch, making it hard to even evaluate. Hi Jason, I can't speak for Hangbin, we do not work for the same company and I was not aware of his efforts until this patch landed. For my part we were already looking at big_key, wireguard and other areas internally, but were not thinking of sending upstream patches like these w/o first a good assessment with our teams and lab that they were proper and sufficient. > So > I think either you should send an exhaustive patch series that forbids > all use of non-FIPS crypto anywhere in the kernel (another example: > net/core/secure_seq.c) in addition to all tunneling modules that don't > use FIPS-certified crypto, or figure out how to disable the lib/crypto > primitives that you want to be disabled in "fips mode". With a > coherent patchset for either of these, we can then evaluate it. Yes a cohesive approach would be ideal, but I do not know if pushing substantially the same checks we have in the Crypto API down to lib/crypto is the right way to go, I am not oppose but I guess Herbert would have to chime in here. -- Simo Sorce RHEL Crypto Team Red Hat, Inc
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, Apr 9, 2021 at 6:47 AM Simo Sorce wrote: > > depends on m || !CRYPTO_FIPS > > > > but I am a bit concerned that the rather intricate kconfig > > dependencies between the generic and arch-optimized versions of those > > drivers get complicated even further. > > Actually this is the opposite direction we are planning to go for > future fips certifications. > > Due to requirements about crypto module naming and versioning in the > new FIPS-140-3 standard we are planning to always build all the CRYPTO > as bultin (and maybe even forbid loading additional crypto modules in > FIPS mode). This is clearly just a vendor choice and has no bearing on > what upstream ultimately will do, but just throwing it here as a data > point. I'm wondering: do you intend to apply similar patches to all the other uses of "non-FIPS-certified" crypto in the kernel? I've already brought up big_key.c, for example. Also if you're intent on adding this check to WireGuard, because it tunnels packets without using FIPS-certified crypto primitives, do you also plan on adding this check to other network tunnels that don't tunnel packets using FIPS-certified crypto primitives? For example, GRE, VXLAN, GENEVE? I'd be inclined to take this patch more seriously if it was exhaustive and coherent for your use case. The targeted hit on WireGuard seems incoherent as a standalone patch, making it hard to even evaluate. So I think either you should send an exhaustive patch series that forbids all use of non-FIPS crypto anywhere in the kernel (another example: net/core/secure_seq.c) in addition to all tunneling modules that don't use FIPS-certified crypto, or figure out how to disable the lib/crypto primitives that you want to be disabled in "fips mode". With a coherent patchset for either of these, we can then evaluate it. Jason
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, Apr 9, 2021 at 2:08 AM Hangbin Liu wrote: > After offline discussion with Herbert, here is > what he said: > > """ > This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random > in FIPS mode. > """ So far as I can see, this isn't the case in the kernel sources I'm reading? Maybe you're doing some userspace hack with CUSE? But at least get_random_bytes doesn't behave this way...
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, 2021-04-09 at 16:08 +0800, Hangbin Liu wrote: > On Fri, Apr 09, 2021 at 09:08:20AM +0200, Stephan Mueller wrote: > > > > > > > And how do you handle all the other places in the kernel that use > > > > > > > ChaCha20 and > > > > > > > SipHash? For example, drivers/char/random.c? > > > > > > > > > > > > Good question, I will check it and reply to you later. > > > > > > > > > > I just read the code. The drivers/char/random.c do has some fips > > > > > specific > > > > > parts(seems not related to crypto). After commit e192be9d9a30 > > > > > ("random: > > > > > replace > > > > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha > > > > > code to > > > > > lib/chacha20.c and make that code out of control. > > > > > > > > > So you are saying that you removed drivers/char/random.c and > > > > lib/chacha20.c from > > > > your FIPS module boundary? Why not do the same for WireGuard? > > > > > > No, I mean this looks like a bug (using not allowed crypto in FIPS mode) > > > and > > > we should fix it. > > > > The entirety of random.c is not compliant to FIPS rules. ChaCha20 is the > > least > > of the problems. SP800-90B is the challenge. This is one of the motivation > > of > > the design and architecture of the LRNG allowing different types of crypto > > and > > have a different approach to post-process the data. > > > > https://github.com/smuellerDD/lrng > > Thanks Stephan for this info. After offline discussion with Herbert, here is > what he said: > > """ > This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random > in FIPS mode. > """ > > I'm not familiar with this code, not sure how upstream handle this. It is an open problem upstream. Simo. -- Simo Sorce RHEL Crypto Team Red Hat, Inc
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, 2021-04-09 at 08:02 +0200, Ard Biesheuvel wrote: > On Fri, 9 Apr 2021 at 05:03, Jason A. Donenfeld wrote: > > On Fri, Apr 09, 2021 at 10:49:07AM +0800, Hangbin Liu wrote: > > > On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote: > > > > Since it's just a normal module library, you can simply do this in the > > > > module_init function, rather than deep within registration > > > > abstractions. > > > > > > I did a try but looks it's not that simple. Not sure if it's because > > > wireguard > > > calls the library directly. Need to check more... > > > > Something like the below should work... > > > > The below only works if all the code is modular. initcall return > values are ignored for builtin code, and so the library functions will > happily work regardless of fips_enabled, and there is generally no > guarantee that no library calls can be made before the initcall() is > invoked. > > For ordinary crypto API client code, the algorithm in question may be > an a priori unknown, and so the only sensible place to put this check > is where the algorithms are registered or instantiated. > > For code such as WireGuard that is hardwired to use a single set of > (forbidden! :-)) algorithms via library calls, the simplest way to do > this securely is to disable the whole thing, even though I agree it is > not the most elegant solution. > > If we go with Jason's approach, we would need to mandate each of these > drivers can only be built as a module if the kernel is built with > FIPS-200 support. This is rather trivial by itself, i.e., > > depends on m || !CRYPTO_FIPS > > but I am a bit concerned that the rather intricate kconfig > dependencies between the generic and arch-optimized versions of those > drivers get complicated even further. Actually this is the opposite direction we are planning to go for future fips certifications. Due to requirements about crypto module naming and versioning in the new FIPS-140-3 standard we are planning to always build all the CRYPTO as bultin (and maybe even forbid loading additional crypto modules in FIPS mode). This is clearly just a vendor choice and has no bearing on what upstream ultimately will do, but just throwing it here as a data point. Plus, as you note, it would overly complicate the interfaces. As much as the check in wireguard is inelegant, it is much simpler to understand and is not invasive. Simo. -- Simo Sorce RHEL Crypto Team Red Hat, Inc
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, Apr 09, 2021 at 09:08:20AM +0200, Stephan Mueller wrote: > > > > > > And how do you handle all the other places in the kernel that use > > > > > > ChaCha20 and > > > > > > SipHash? For example, drivers/char/random.c? > > > > > > > > > > Good question, I will check it and reply to you later. > > > > > > > > I just read the code. The drivers/char/random.c do has some fips > > > > specific > > > > parts(seems not related to crypto). After commit e192be9d9a30 ("random: > > > > replace > > > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha > > > > code to > > > > lib/chacha20.c and make that code out of control. > > > > > > > So you are saying that you removed drivers/char/random.c and > > > lib/chacha20.c from > > > your FIPS module boundary? Why not do the same for WireGuard? > > > > No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and > > we should fix it. > > The entirety of random.c is not compliant to FIPS rules. ChaCha20 is the least > of the problems. SP800-90B is the challenge. This is one of the motivation of > the design and architecture of the LRNG allowing different types of crypto and > have a different approach to post-process the data. > > https://github.com/smuellerDD/lrng Thanks Stephan for this info. After offline discussion with Herbert, here is what he said: """ This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random in FIPS mode. """ I'm not familiar with this code, not sure how upstream handle this. Thanks Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
Am Freitag, dem 09.04.2021 um 10:11 +0800 schrieb Hangbin Liu: > On Thu, Apr 08, 2021 at 08:11:34AM -0700, Eric Biggers wrote: > > On Thu, Apr 08, 2021 at 07:58:08PM +0800, Hangbin Liu wrote: > > > On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote: > > > > > Also, couldn't you just consider WireGuard to be outside your FIPS > > > > > module > > > > > boundary, which would remove it from the scope of the certification? > > > > > > > > > > And how do you handle all the other places in the kernel that use > > > > > ChaCha20 and > > > > > SipHash? For example, drivers/char/random.c? > > > > > > > > Good question, I will check it and reply to you later. > > > > > > I just read the code. The drivers/char/random.c do has some fips > > > specific > > > parts(seems not related to crypto). After commit e192be9d9a30 ("random: > > > replace > > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha > > > code to > > > lib/chacha20.c and make that code out of control. > > > > > So you are saying that you removed drivers/char/random.c and > > lib/chacha20.c from > > your FIPS module boundary? Why not do the same for WireGuard? > > No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and > we should fix it. The entirety of random.c is not compliant to FIPS rules. ChaCha20 is the least of the problems. SP800-90B is the challenge. This is one of the motivation of the design and architecture of the LRNG allowing different types of crypto and have a different approach to post-process the data. https://github.com/smuellerDD/lrng Ciao Stephan > > Thanks > Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, 9 Apr 2021 at 05:03, Jason A. Donenfeld wrote: > > On Fri, Apr 09, 2021 at 10:49:07AM +0800, Hangbin Liu wrote: > > On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote: > > > Since it's just a normal module library, you can simply do this in the > > > module_init function, rather than deep within registration > > > abstractions. > > > > I did a try but looks it's not that simple. Not sure if it's because > > wireguard > > calls the library directly. Need to check more... > > Something like the below should work... > The below only works if all the code is modular. initcall return values are ignored for builtin code, and so the library functions will happily work regardless of fips_enabled, and there is generally no guarantee that no library calls can be made before the initcall() is invoked. For ordinary crypto API client code, the algorithm in question may be an a priori unknown, and so the only sensible place to put this check is where the algorithms are registered or instantiated. For code such as WireGuard that is hardwired to use a single set of (forbidden! :-)) algorithms via library calls, the simplest way to do this securely is to disable the whole thing, even though I agree it is not the most elegant solution. If we go with Jason's approach, we would need to mandate each of these drivers can only be built as a module if the kernel is built with FIPS-200 support. This is rather trivial by itself, i.e., depends on m || !CRYPTO_FIPS but I am a bit concerned that the rather intricate kconfig dependencies between the generic and arch-optimized versions of those drivers get complicated even further. -- Ard. > diff --git a/arch/arm/crypto/chacha-glue.c b/arch/arm/crypto/chacha-glue.c > index a408f4bcfd62..47212f9421c1 100644 > --- a/arch/arm/crypto/chacha-glue.c > +++ b/arch/arm/crypto/chacha-glue.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -297,6 +298,9 @@ static int __init chacha_simd_mod_init(void) > { > int err = 0; > > + if (fips_enabled) > + return -EOPNOTSUPP; > + > if (IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER)) { > err = crypto_register_skciphers(arm_algs, > ARRAY_SIZE(arm_algs)); > if (err) > diff --git a/arch/arm/crypto/curve25519-glue.c > b/arch/arm/crypto/curve25519-glue.c > index 31eb75b6002f..d03f810fdaf3 100644 > --- a/arch/arm/crypto/curve25519-glue.c > +++ b/arch/arm/crypto/curve25519-glue.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -114,6 +115,9 @@ static struct kpp_alg curve25519_alg = { > > static int __init mod_init(void) > { > + if (fips_enabled) > + return -EOPNOTSUPP; > + > if (elf_hwcap & HWCAP_NEON) { > static_branch_enable(&have_neon); > return IS_REACHABLE(CONFIG_CRYPTO_KPP) ? > diff --git a/arch/arm/crypto/poly1305-glue.c b/arch/arm/crypto/poly1305-glue.c > index 3023c1acfa19..30d6c6de7a27 100644 > --- a/arch/arm/crypto/poly1305-glue.c > +++ b/arch/arm/crypto/poly1305-glue.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > void poly1305_init_arm(void *state, const u8 *key); > void poly1305_blocks_arm(void *state, const u8 *src, u32 len, u32 hibit); > @@ -240,6 +241,9 @@ static struct shash_alg arm_poly1305_algs[] = {{ > > static int __init arm_poly1305_mod_init(void) > { > + if (fips_enabled) > + return -EOPNOTSUPP; > + > if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && > (elf_hwcap & HWCAP_NEON)) > static_branch_enable(&have_neon); > diff --git a/arch/arm64/crypto/chacha-neon-glue.c > b/arch/arm64/crypto/chacha-neon-glue.c > index 1d9824c4ae43..1696993326b5 100644 > --- a/arch/arm64/crypto/chacha-neon-glue.c > +++ b/arch/arm64/crypto/chacha-neon-glue.c > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -214,6 +215,9 @@ static struct skcipher_alg algs[] = { > > static int __init chacha_simd_mod_init(void) > { > + if (fips_enabled) > + return -EOPNOTSUPP; > + > if (!cpu_have_named_feature(ASIMD)) > return 0; > > diff --git a/arch/arm64/crypto/poly1305-glue.c > b/arch/arm64/crypto/poly1305-glue.c > index f33ada70c4ed..ac257a52be4d 100644 > --- a/arch/arm64/crypto/poly1305-glue.c > +++ b/arch/arm64/crypto/poly1305-glue.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > asmlinkage void poly1305_init_arm64(void *state, const u8 *key); > asmlinkage void poly1305_blocks(void *state, const u8 *src, u32 len, u32 > hibit); > @@ -208,6 +209,9 @@ static struct shash_alg neon_poly1305_alg = { > > static int __init neon_poly1305_mod_init(void) > { > + if (fips_enabled) > + return -EOPNOTSUPP; > + > if (!cpu_have_named_feature(ASIMD)) >
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Fri, Apr 09, 2021 at 10:49:07AM +0800, Hangbin Liu wrote: > On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote: > > Since it's just a normal module library, you can simply do this in the > > module_init function, rather than deep within registration > > abstractions. > > I did a try but looks it's not that simple. Not sure if it's because wireguard > calls the library directly. Need to check more... Something like the below should work... diff --git a/arch/arm/crypto/chacha-glue.c b/arch/arm/crypto/chacha-glue.c index a408f4bcfd62..47212f9421c1 100644 --- a/arch/arm/crypto/chacha-glue.c +++ b/arch/arm/crypto/chacha-glue.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -297,6 +298,9 @@ static int __init chacha_simd_mod_init(void) { int err = 0; + if (fips_enabled) + return -EOPNOTSUPP; + if (IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER)) { err = crypto_register_skciphers(arm_algs, ARRAY_SIZE(arm_algs)); if (err) diff --git a/arch/arm/crypto/curve25519-glue.c b/arch/arm/crypto/curve25519-glue.c index 31eb75b6002f..d03f810fdaf3 100644 --- a/arch/arm/crypto/curve25519-glue.c +++ b/arch/arm/crypto/curve25519-glue.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -114,6 +115,9 @@ static struct kpp_alg curve25519_alg = { static int __init mod_init(void) { + if (fips_enabled) + return -EOPNOTSUPP; + if (elf_hwcap & HWCAP_NEON) { static_branch_enable(&have_neon); return IS_REACHABLE(CONFIG_CRYPTO_KPP) ? diff --git a/arch/arm/crypto/poly1305-glue.c b/arch/arm/crypto/poly1305-glue.c index 3023c1acfa19..30d6c6de7a27 100644 --- a/arch/arm/crypto/poly1305-glue.c +++ b/arch/arm/crypto/poly1305-glue.c @@ -17,6 +17,7 @@ #include #include #include +#include void poly1305_init_arm(void *state, const u8 *key); void poly1305_blocks_arm(void *state, const u8 *src, u32 len, u32 hibit); @@ -240,6 +241,9 @@ static struct shash_alg arm_poly1305_algs[] = {{ static int __init arm_poly1305_mod_init(void) { + if (fips_enabled) + return -EOPNOTSUPP; + if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && (elf_hwcap & HWCAP_NEON)) static_branch_enable(&have_neon); diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c index 1d9824c4ae43..1696993326b5 100644 --- a/arch/arm64/crypto/chacha-neon-glue.c +++ b/arch/arm64/crypto/chacha-neon-glue.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -214,6 +215,9 @@ static struct skcipher_alg algs[] = { static int __init chacha_simd_mod_init(void) { + if (fips_enabled) + return -EOPNOTSUPP; + if (!cpu_have_named_feature(ASIMD)) return 0; diff --git a/arch/arm64/crypto/poly1305-glue.c b/arch/arm64/crypto/poly1305-glue.c index f33ada70c4ed..ac257a52be4d 100644 --- a/arch/arm64/crypto/poly1305-glue.c +++ b/arch/arm64/crypto/poly1305-glue.c @@ -17,6 +17,7 @@ #include #include #include +#include asmlinkage void poly1305_init_arm64(void *state, const u8 *key); asmlinkage void poly1305_blocks(void *state, const u8 *src, u32 len, u32 hibit); @@ -208,6 +209,9 @@ static struct shash_alg neon_poly1305_alg = { static int __init neon_poly1305_mod_init(void) { + if (fips_enabled) + return -EOPNOTSUPP; + if (!cpu_have_named_feature(ASIMD)) return 0; diff --git a/arch/mips/crypto/chacha-glue.c b/arch/mips/crypto/chacha-glue.c index 90896029d0cd..31f8294f2a31 100644 --- a/arch/mips/crypto/chacha-glue.c +++ b/arch/mips/crypto/chacha-glue.c @@ -12,6 +12,7 @@ #include #include #include +#include asmlinkage void chacha_crypt_arch(u32 *state, u8 *dst, const u8 *src, unsigned int bytes, int nrounds); @@ -128,6 +129,9 @@ static struct skcipher_alg algs[] = { static int __init chacha_simd_mod_init(void) { + if (fips_enabled) + return -EOPNOTSUPP; + return IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER) ? crypto_register_skciphers(algs, ARRAY_SIZE(algs)) : 0; } diff --git a/arch/mips/crypto/poly1305-glue.c b/arch/mips/crypto/poly1305-glue.c index fc881b46d911..f5edec10cef8 100644 --- a/arch/mips/crypto/poly1305-glue.c +++ b/arch/mips/crypto/poly1305-glue.c @@ -12,6 +12,7 @@ #include #include #include +#include asmlinkage void poly1305_init_mips(void *state, const u8 *key); asmlinkage void poly1305_blocks_mips(void *state, const u8 *src, u32 len, u32 hibit); @@ -173,6 +174,9 @@ static struct shash_alg mips_poly1305_alg = { static int __init mips_poly1305_mod_init(void) { + if (fips_enabled) + return -EOPNOTSUPP; + return IS_REACHABLE(CONFIG_CRYPTO_HASH) ? crypto_register_shash(&mips_poly1305_alg) : 0; } diff --git a/
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote: > Since it's just a normal module library, you can simply do this in the > module_init function, rather than deep within registration > abstractions. I did a try but looks it's not that simple. Not sure if it's because wireguard calls the library directly. Need to check more... Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
Hi Hangbin, On Thu, Apr 8, 2021 at 8:41 PM Hangbin Liu wrote: > I agree that the best way is to disable the crypto modules in FIPS mode. > But the code in lib/crypto looks not the same with crypto/. For modules > in crypto, there is an alg_test() to check if the crytpo is FIPS allowed > when do register. > > - crypto_register_alg() > - crypto_wait_for_test() > - crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult) > - cryptomgr_schedule_test() > - cryptomgr_test() > - alg_test() > > But in lib/crypto the code are more like a library. We can call it anytime > and there is no register. Maybe we should add a similar check in lib/crypto. > But I'm not familiar with crypto code... Not sure if anyone in linux-crypto@ > would like help do that. Since it's just a normal module library, you can simply do this in the module_init function, rather than deep within registration abstractions. > > 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 > > #include > > #include > > +#include > > > > bool curve25519_selftest(void); > > > > static int __init mod_init(void) > > { > > + if (!fips_enabled) > > + return -EOPNOTSUPP; > > Question here, why it is !fips_enabled? Shouldn't we return error when > fips_enabled? Er, just not thinking straight today. `if (fips_enabled)` is probably what you want indeed. Jason
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 08, 2021 at 03:55:59PM -0600, Jason A. Donenfeld wrote: > On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce 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: Hi Jason, I agree that the best way is to disable the crypto modules in FIPS mode. But the code in lib/crypto looks not the same with crypto/. For modules in crypto, there is an alg_test() to check if the crytpo is FIPS allowed when do register. - crypto_register_alg() - crypto_wait_for_test() - crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult) - cryptomgr_schedule_test() - cryptomgr_test() - alg_test() But in lib/crypto the code are more like a library. We can call it anytime and there is no register. Maybe we should add a similar check in lib/crypto. But I'm not familiar with crypto code... Not sure if anyone in linux-crypto@ would like help do that. > > 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 > #include > #include > +#include > > bool curve25519_selftest(void); > > static int __init mod_init(void) > { > + if (!fips_enabled) > + return -EOPNOTSUPP; Question here, why it is !fips_enabled? Shouldn't we return error when fips_enabled? Thanks Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 08, 2021 at 08:11:34AM -0700, Eric Biggers wrote: > On Thu, Apr 08, 2021 at 07:58:08PM +0800, Hangbin Liu wrote: > > On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote: > > > > Also, couldn't you just consider WireGuard to be outside your FIPS > > > > module > > > > boundary, which would remove it from the scope of the certification? > > > > > > > > And how do you handle all the other places in the kernel that use > > > > ChaCha20 and > > > > SipHash? For example, drivers/char/random.c? > > > > > > Good question, I will check it and reply to you later. > > > > I just read the code. The drivers/char/random.c do has some fips specific > > parts(seems not related to crypto). After commit e192be9d9a30 ("random: > > replace > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha code > > to > > lib/chacha20.c and make that code out of control. > > > So you are saying that you removed drivers/char/random.c and lib/chacha20.c > from > your FIPS module boundary? Why not do the same for WireGuard? No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and we should fix it. Thanks Hangbin
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 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 > #include > #include > +#include > > 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
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce 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 #include #include +#include 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
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 08, 2021 at 07:58:08PM +0800, Hangbin Liu wrote: > On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote: > > > Also, couldn't you just consider WireGuard to be outside your FIPS module > > > boundary, which would remove it from the scope of the certification? > > > > > > And how do you handle all the other places in the kernel that use > > > ChaCha20 and > > > SipHash? For example, drivers/char/random.c? > > > > Good question, I will check it and reply to you later. > > I just read the code. The drivers/char/random.c do has some fips specific > parts(seems not related to crypto). After commit e192be9d9a30 ("random: > replace > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha code to > lib/chacha20.c and make that code out of control. > So you are saying that you removed drivers/char/random.c and lib/chacha20.c from your FIPS module boundary? Why not do the same for WireGuard? - Eric
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Wed, 2021-04-07 at 15:15 -0600, Jason A. Donenfeld wrote: > Hi Hangbin, > > On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu wrote: > > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not > > FIPS certified, the WireGuard module should be disabled in FIPS mode. > > 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. > [As an aside, I don't think any of this fips-flag-in-the-kernel makes > much sense at all for anything, but that seems like a different > discussion, maybe?] It makes sense, because vendors provide a single kernel that can be used by both people that are required to be FIPS compliant and people that don't. For people that are required to be FIPS complaint vendors want to provide the ability to use a single knob (fips=1 at boot) to turn off everything that is not FIPS compliant. Disabling algorithms at compile time would not work for people that do not care about FIPS compliance. HTH, Simo. -- Simo Sorce RHEL Crypto Team Red Hat, Inc
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote: > > Also, couldn't you just consider WireGuard to be outside your FIPS module > > boundary, which would remove it from the scope of the certification? > > > > And how do you handle all the other places in the kernel that use ChaCha20 > > and > > SipHash? For example, drivers/char/random.c? > > Good question, I will check it and reply to you later. I just read the code. The drivers/char/random.c do has some fips specific parts(seems not related to crypto). After commit e192be9d9a30 ("random: replace non-blocking pool with a Chacha20-based CRNG") we moved part of chacha code to lib/chacha20.c and make that code out of control. Thanks Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Thu, Apr 8, 2021 at 8:52 AM Hangbin Liu wrote: > On Wed, Apr 07, 2021 at 03:15:51PM -0600, Jason A. Donenfeld wrote: > > Hi Hangbin, > > > > On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu wrote: > > > > > > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not > > > FIPS certified, the WireGuard module should be disabled in FIPS mode. > > > > 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? > > Hi Jason, > > I'm not familiar with the crypto code. From wg_noise_init() it looks the init > part is in header file. So I just disabled wireguard directly. > > For disabling the modules. Hi Ondrej, do you know if there is any FIPS policy > in crypto part? There seems no handler when load not allowed crypto modules > in FIPS mode. If I understand your question correctly, yes, there is a mechanism that disables not-FIPS-approved algorithms/drivers in FIPS mode (not kernel modules themselves, AFAIK). So if any part of the kernel tries to use e.g. chacha20 via the Crypto API (the bits in crypto/...), it will fail. I'm not sure about the direct library interface (the bits in lib/crypto/...) though... That's relatively new and I haven't been following the upstream development in this area that closely for some time now... > > BTW, I also has a question, apart from the different RFC standard, what's the > relation/difference between crypto/chacha20poly1305.c and > lib/crypto/chacha20poly1305.c? > > Thanks > Hangbin > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Wed, Apr 07, 2021 at 03:15:51PM -0600, Jason A. Donenfeld wrote: > Hi Hangbin, > > On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu wrote: > > > > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not > > FIPS certified, the WireGuard module should be disabled in FIPS mode. > > 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? Hi Jason, I'm not familiar with the crypto code. From wg_noise_init() it looks the init part is in header file. So I just disabled wireguard directly. For disabling the modules. Hi Ondrej, do you know if there is any FIPS policy in crypto part? There seems no handler when load not allowed crypto modules in FIPS mode. BTW, I also has a question, apart from the different RFC standard, what's the relation/difference between crypto/chacha20poly1305.c and lib/crypto/chacha20poly1305.c? Thanks Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Wed, Apr 07, 2021 at 02:12:27PM -0700, Eric Biggers wrote: > On Wed, Apr 07, 2021 at 07:39:20PM +0800, Hangbin Liu wrote: > > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not > > FIPS certified, the WireGuard module should be disabled in FIPS mode. > > > > Signed-off-by: Hangbin Liu > > I think you mean "FIPS allowed", not "FIPS certified"? Even if it used FIPS > allowed algorithms like AES, the Linux kernel doesn't come with any sort of > FIPS > certification out of the box. Yes, you are right. > > Also, couldn't you just consider WireGuard to be outside your FIPS module > boundary, which would remove it from the scope of the certification? > > And how do you handle all the other places in the kernel that use ChaCha20 and > SipHash? For example, drivers/char/random.c? Good question, I will check it and reply to you later. Thanks Hangbin
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
Hi Hangbin, On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu wrote: > > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not > FIPS certified, the WireGuard module should be disabled in FIPS mode. 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? [As an aside, I don't think any of this fips-flag-in-the-kernel makes much sense at all for anything, but that seems like a different discussion, maybe?] Jason
Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode
On Wed, Apr 07, 2021 at 07:39:20PM +0800, Hangbin Liu wrote: > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not > FIPS certified, the WireGuard module should be disabled in FIPS mode. > > Signed-off-by: Hangbin Liu I think you mean "FIPS allowed", not "FIPS certified"? Even if it used FIPS allowed algorithms like AES, the Linux kernel doesn't come with any sort of FIPS certification out of the box. Also, couldn't you just consider WireGuard to be outside your FIPS module boundary, which would remove it from the scope of the certification? And how do you handle all the other places in the kernel that use ChaCha20 and SipHash? For example, drivers/char/random.c? - Eric