[PATCH net] esp: select CRYPTO_SEQIV

[PATCH net] esp: select CRYPTO_SEQIV

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Since CRYPTO_CTR no longer selects CRYPTO_SEQIV, it should be selected
by INET_ESP and INET6_ESP -- similar to CRYPTO_ECHAINIV.

Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV")
Cc: Corentin Labbe <clabbe@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 net/ipv4/Kconfig | 1 +
 net/ipv6/Kconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
index 23ba5045e3d3..d393e8132aa1 100644
--- a/net/ipv4/Kconfig
+++ b/net/ipv4/Kconfig
@@ -361,6 +361,7 @@ config INET_ESP
 	select CRYPTO_SHA1
 	select CRYPTO_DES
 	select CRYPTO_ECHAINIV
+	select CRYPTO_SEQIV
 	---help---
 	  Support for IPsec ESP.
 
diff --git a/net/ipv6/Kconfig b/net/ipv6/Kconfig
index 4f03aece2980..f2f4563c8dbf 100644
--- a/net/ipv6/Kconfig
+++ b/net/ipv6/Kconfig
@@ -70,6 +70,7 @@ config INET6_ESP
 	select CRYPTO_SHA1
 	select CRYPTO_DES
 	select CRYPTO_ECHAINIV
+	select CRYPTO_SEQIV
 	---help---
 	  Support for IPsec ESP.
 
-- 
2.27.0.278.ge193c7cf3a9-goog

Re: [PATCH net] esp: select CRYPTO_SEQIV

[Herbert Xu]

On Thu, Jun 04, 2020 at 12:23:22PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since CRYPTO_CTR no longer selects CRYPTO_SEQIV, it should be selected
> by INET_ESP and INET6_ESP -- similar to CRYPTO_ECHAINIV.
> 
> Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV")
> Cc: Corentin Labbe <clabbe@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  net/ipv4/Kconfig | 1 +
>  net/ipv6/Kconfig | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index 23ba5045e3d3..d393e8132aa1 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -361,6 +361,7 @@ config INET_ESP
>  	select CRYPTO_SHA1
>  	select CRYPTO_DES
>  	select CRYPTO_ECHAINIV
> +	select CRYPTO_SEQIV
>  	---help---
>  	  Support for IPsec ESP.

Hmm, the selection list doesn't include CTR so just adding SEQIV
per se makes no sense.  I'm not certain that we really want to
include every algorithm under the sun.  Steffen, what do you think?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] esp: select CRYPTO_SEQIV

[Herbert Xu]

On Fri, Jun 05, 2020 at 10:28:58AM +1000, Herbert Xu wrote:
>
> Hmm, the selection list doesn't include CTR so just adding SEQIV
> per se makes no sense.  I'm not certain that we really want to
> include every algorithm under the sun.  Steffen, what do you think?

Or how about

	select CRYPTO_SEQIV if CRYPTO_CTR

That would make more sense.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] esp: select CRYPTO_SEQIV

[Eric Biggers]

On Fri, Jun 05, 2020 at 10:29:56AM +1000, Herbert Xu wrote:
> On Fri, Jun 05, 2020 at 10:28:58AM +1000, Herbert Xu wrote:
> >
> > Hmm, the selection list doesn't include CTR so just adding SEQIV
> > per se makes no sense.  I'm not certain that we really want to
> > include every algorithm under the sun.  Steffen, what do you think?
> 
> Or how about
> 
> 	select CRYPTO_SEQIV if CRYPTO_CTR
> 
> That would make more sense.
> 
> Cheers,

There's also a case where "seqiv" is used without counter mode:

net/xfrm/xfrm_algo.c:

{
        .name = "rfc7539esp(chacha20,poly1305)",

        .uinfo = {
                .aead = {
                        .geniv = "seqiv",
                        .icv_truncbits = 128,
                }
        },

        .pfkey_supported = 0,
},


FWIW, we make CONFIG_FS_ENCRYPTION select only the algorithms that we consider
the "default", and any "non-default" algorithms need to be explicitly enabled.

Is something similar going on here with INET_ESP and INET_ESP6?  Should "seqiv"
be considered a "default" for IPsec?

- Eric

Re: [PATCH net] esp: select CRYPTO_SEQIV

[Herbert Xu]

On Thu, Jun 04, 2020 at 10:09:10PM -0700, Eric Biggers wrote:
>
> There's also a case where "seqiv" is used without counter mode:
> 
> net/xfrm/xfrm_algo.c:
> 
> {
>         .name = "rfc7539esp(chacha20,poly1305)",

So

	select CRYPTO_SEQIV if CRYPTO_CTR || CRYPTO_CHACHA20POLY1305

and if the list gets too long we could create another symbol that is
selected by the algorithms, say CRYPTO_SEQIV_ESP which could then
be used as the condition:

	config CRYPTO_SEQIV_ESP
		bool

	config CRYPTO_CTR
		select CRYPTO_SEQIV_ESP

	config INET_ESP
		select CRYPTO_SEQIV if CRYPTO_SEQIV_ESP

> FWIW, we make CONFIG_FS_ENCRYPTION select only the algorithms that we consider
> the "default", and any "non-default" algorithms need to be explicitly enabled.
> 
> Is something similar going on here with INET_ESP and INET_ESP6?  Should "seqiv"
> be considered a "default" for IPsec?

The default with IPsec is up to the user-space IPsec manager,
e.g., libreswan.  So the kernel has no idea what the default
is.  Also, unlike filesystems IPsec is about interoperability
so the default is actually a list of algorithms.

If you were using libreswan then top of the list is gcm(aes),
followed by aes(cbc)+sha256, and then aes(cbc)+sha1.

Incidentally we should probably prune the INET_ESP select list.
At least MD5/SHA1/DES shouldn't be on it.  We probably should
add AES, SHA256 and GCM to the list.

Another potential improvement is to merge the two select lists
between ESP and ESP6.  Perhaps move them to a new tristate say
XFRM_ESP that would then be selected by ESP and ESP6.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] esp: select CRYPTO_SEQIV

[Greg Kroah-Hartman]

On Thu, Jun 04, 2020 at 12:23:22PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Since CRYPTO_CTR no longer selects CRYPTO_SEQIV, it should be selected
> by INET_ESP and INET6_ESP -- similar to CRYPTO_ECHAINIV.
> 
> Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV")
> Cc: Corentin Labbe <clabbe@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>


Tested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH net v2] esp: select CRYPTO_SEQIV when useful

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

CRYPTO_CTR no longer selects CRYPTO_SEQIV, which breaks IPsec for users
who need any of the algorithms that use seqiv.  These users now would
need to explicitly enable CRYPTO_SEQIV.

There doesn't seem to be a clear rule on what algorithms the IPsec
options (INET_ESP and INET6_ESP) actually select, as apparently none is
*always* required.  They currently select just a particular subset,
along with CRYPTO_ECHAINIV which is the other IV generator template.

As a compromise between too many and too few selections, select
CRYPTO_SEQIV if either CRYPTO_CTR or CRYPTO_CHACHA20POLY1305 is enabled.
These are the algorithms that can use seqiv for IPsec.  (Note: GCM and
CCM can too, but those both use CTR.)

Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV")
Cc: Corentin Labbe <clabbe@baylibre.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---

v2: added the 'if' condition and updated commit message

 net/ipv4/Kconfig | 1 +
 net/ipv6/Kconfig | 1 +
 2 files changed, 2 insertions(+)

diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
index 23ba5045e3d3..6520b30883cf 100644
--- a/net/ipv4/Kconfig
+++ b/net/ipv4/Kconfig
@@ -361,6 +361,7 @@ config INET_ESP
 	select CRYPTO_SHA1
 	select CRYPTO_DES
 	select CRYPTO_ECHAINIV
+	select CRYPTO_SEQIV if CRYPTO_CTR || CRYPTO_CHACHA20POLY1305
 	---help---
 	  Support for IPsec ESP.
 
diff --git a/net/ipv6/Kconfig b/net/ipv6/Kconfig
index 4f03aece2980..c78adb0f5339 100644
--- a/net/ipv6/Kconfig
+++ b/net/ipv6/Kconfig
@@ -70,6 +70,7 @@ config INET6_ESP
 	select CRYPTO_SHA1
 	select CRYPTO_DES
 	select CRYPTO_ECHAINIV
+	select CRYPTO_SEQIV if CRYPTO_CTR || CRYPTO_CHACHA20POLY1305
 	---help---
 	  Support for IPsec ESP.
 
-- 
2.27.0.278.ge193c7cf3a9-goog

Re: [PATCH net v2] esp: select CRYPTO_SEQIV when useful

[Eric Biggers]

On Fri, Jun 05, 2020 at 10:39:31AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> CRYPTO_CTR no longer selects CRYPTO_SEQIV, which breaks IPsec for users
> who need any of the algorithms that use seqiv.  These users now would
> need to explicitly enable CRYPTO_SEQIV.
> 
> There doesn't seem to be a clear rule on what algorithms the IPsec
> options (INET_ESP and INET6_ESP) actually select, as apparently none is
> *always* required.  They currently select just a particular subset,
> along with CRYPTO_ECHAINIV which is the other IV generator template.
> 
> As a compromise between too many and too few selections, select
> CRYPTO_SEQIV if either CRYPTO_CTR or CRYPTO_CHACHA20POLY1305 is enabled.
> These are the algorithms that can use seqiv for IPsec.  (Note: GCM and
> CCM can too, but those both use CTR.)
> 
> Fixes: f23efcbcc523 ("crypto: ctr - no longer needs CRYPTO_SEQIV")
> Cc: Corentin Labbe <clabbe@baylibre.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> 
> v2: added the 'if' condition and updated commit message
> 
>  net/ipv4/Kconfig | 1 +
>  net/ipv6/Kconfig | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index 23ba5045e3d3..6520b30883cf 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -361,6 +361,7 @@ config INET_ESP
>  	select CRYPTO_SHA1
>  	select CRYPTO_DES
>  	select CRYPTO_ECHAINIV
> +	select CRYPTO_SEQIV if CRYPTO_CTR || CRYPTO_CHACHA20POLY1305
>  	---help---
>  	  Support for IPsec ESP.
>  

Oops, this doesn't actually work:

scripts/kconfig/conf  --olddefconfig Kconfig
crypto/Kconfig:1799:error: recursive dependency detected!
crypto/Kconfig:1799:	symbol CRYPTO_DRBG_MENU is selected by CRYPTO_RNG_DEFAULT
crypto/Kconfig:83:	symbol CRYPTO_RNG_DEFAULT is selected by CRYPTO_SEQIV
crypto/Kconfig:330:	symbol CRYPTO_SEQIV is selected by CRYPTO_CTR
crypto/Kconfig:370:	symbol CRYPTO_CTR is selected by CRYPTO_DRBG_CTR
crypto/Kconfig:1819:	symbol CRYPTO_DRBG_CTR depends on CRYPTO_DRBG_MENU
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"


I guess we need to go with v1 (which just had 'select CRYPTO_SEQIV'),
or else make users explicitly select CRYPTO_SEQIV?

- Eric

Re: [PATCH net v2] esp: select CRYPTO_SEQIV when useful

[Steffen Klassert]

On Fri, Jun 05, 2020 at 11:00:23AM -0700, Eric Biggers wrote:
> On Fri, Jun 05, 2020 at 10:39:31AM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> > index 23ba5045e3d3..6520b30883cf 100644
> > --- a/net/ipv4/Kconfig
> > +++ b/net/ipv4/Kconfig
> > @@ -361,6 +361,7 @@ config INET_ESP
> >  	select CRYPTO_SHA1
> >  	select CRYPTO_DES
> >  	select CRYPTO_ECHAINIV
> > +	select CRYPTO_SEQIV if CRYPTO_CTR || CRYPTO_CHACHA20POLY1305
> >  	---help---
> >  	  Support for IPsec ESP.
> >  
> 
> Oops, this doesn't actually work:
> 
> scripts/kconfig/conf  --olddefconfig Kconfig
> crypto/Kconfig:1799:error: recursive dependency detected!
> crypto/Kconfig:1799:	symbol CRYPTO_DRBG_MENU is selected by CRYPTO_RNG_DEFAULT
> crypto/Kconfig:83:	symbol CRYPTO_RNG_DEFAULT is selected by CRYPTO_SEQIV
> crypto/Kconfig:330:	symbol CRYPTO_SEQIV is selected by CRYPTO_CTR
> crypto/Kconfig:370:	symbol CRYPTO_CTR is selected by CRYPTO_DRBG_CTR
> crypto/Kconfig:1819:	symbol CRYPTO_DRBG_CTR depends on CRYPTO_DRBG_MENU
> For a resolution refer to Documentation/kbuild/kconfig-language.rst
> subsection "Kconfig recursive dependency limitations"
> 
> 
> I guess we need to go with v1 (which just had 'select CRYPTO_SEQIV'),
> or else make users explicitly select CRYPTO_SEQIV?

I think we should make INET_ESP to select everything that is
needed to instantiate the ciphers marked as 'MUST' in RFC 
8221 and let the users explicitly select everything else.

Re: [PATCH net v2] esp: select CRYPTO_SEQIV when useful

[Herbert Xu]

On Fri, Jun 05, 2020 at 11:00:23AM -0700, Eric Biggers wrote:
>
> Oops, this doesn't actually work:
> 
> scripts/kconfig/conf  --olddefconfig Kconfig
> crypto/Kconfig:1799:error: recursive dependency detected!
> crypto/Kconfig:1799:	symbol CRYPTO_DRBG_MENU is selected by CRYPTO_RNG_DEFAULT
> crypto/Kconfig:83:	symbol CRYPTO_RNG_DEFAULT is selected by CRYPTO_SEQIV
> crypto/Kconfig:330:	symbol CRYPTO_SEQIV is selected by CRYPTO_CTR
> crypto/Kconfig:370:	symbol CRYPTO_CTR is selected by CRYPTO_DRBG_CTR
> crypto/Kconfig:1819:	symbol CRYPTO_DRBG_CTR depends on CRYPTO_DRBG_MENU
> For a resolution refer to Documentation/kbuild/kconfig-language.rst
> subsection "Kconfig recursive dependency limitations"
> 
> I guess we need to go with v1 (which just had 'select CRYPTO_SEQIV'),
> or else make users explicitly select CRYPTO_SEQIV?

OK, let's just go with the unconditional select on SEQIV since
Steffen recommended RFC8221 which lists GCM and CBC as MUST and
GCM requires SEQIV to work.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt