[PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output IV

[PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output IV

[Ard Biesheuvel]

Scott reports SUNRPC self-test failures regarding the output IV on arm64
when using the SIMD accelerated implementation of AES in CBC mode with
ciphertext stealing ("cts(cbc(aes))" in crypto API speak).

These failures are the result of the fact that, while RFC 3962 does
specify what the output IV should be and includes test vectors for it,
the general concept of an output IV is poorly defined, and generally,
not specified by the various algorithms implemented by the crypto API.
Only algorithms that support transparent chaining (e.g., CBC mode on a
block boundary) have requirements on the output IV, but ciphertext
stealing (CTS) is fundamentally about how to encapsulate CBC in a way
where the length of the entire message may not be an integral multiple
of the cipher block size, and the concept of an output IV does not exist
here because it has no defined purpose past the end of the message.

The generic CTS template takes advantage of this chaining capability of
the CBC implementations, and as a result, happens to return an output
IV, simply because it passes its IV buffer directly to the encapsulated
CBC implementation, which operates on full blocks only, and always
returns an IV. This output IV happens to match how RFC 3962 defines it,
even though the CTS template itself does not contain any output IV logic
whatsoever, and, for this reason, lacks any test vectors that exercise
this accidental output IV generation.

The arm64 SIMD implementation of cts(cbc(aes)) does not use the generic
CTS template at all, but instead, implements the CBC mode and ciphertext
stealing directly, and therefore does not encapsule a CBC implementation
that returns an output IV in the same way. The arm64 SIMD implementation
complies with the specification and passes all internal tests, but when
invoked by the SUNRPC code, fails to produce the expected output IV and
causes its selftests to fail.

Given that the output IV is defined as the penultimate block (where the
final block may smaller than the block size), we can quite easily derive
it in the caller by copying the appropriate slice of ciphertext after
encryption.

Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna@kernel.org>
Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Jeff Layton <jlayton@kernel.org>
Reported-by: Scott Mayhew <smayhew@redhat.com>
Tested-by: Scott Mayhew <smayhew@redhat.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 net/sunrpc/auth_gss/gss_krb5_crypto.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index 212c5d57465a1bf5..22dca4647ee66b3e 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher *cipher, struct xdr_buf *buf,
 
 	ret = write_bytes_to_xdr_buf(buf, offset, data, len);
 
+	/*
+	 * CBC-CTS does not define an output IV but RFC 3962 defines it as the
+	 * penultimate block of ciphertext, so copy that into the IV buffer
+	 * before returning.
+	 */
+	if (encrypt)
+		memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
 out:
 	kfree(data);
 	return ret;
-- 
2.39.2

Re: [PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output IV

[Chuck Lever III]

> On May 1, 2023, at 10:04 AM, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> Scott reports SUNRPC self-test failures regarding the output IV on arm64
> when using the SIMD accelerated implementation of AES in CBC mode with
> ciphertext stealing ("cts(cbc(aes))" in crypto API speak).
> 
> These failures are the result of the fact that, while RFC 3962 does
> specify what the output IV should be and includes test vectors for it,
> the general concept of an output IV is poorly defined, and generally,
> not specified by the various algorithms implemented by the crypto API.
> Only algorithms that support transparent chaining (e.g., CBC mode on a
> block boundary) have requirements on the output IV, but ciphertext
> stealing (CTS) is fundamentally about how to encapsulate CBC in a way
> where the length of the entire message may not be an integral multiple
> of the cipher block size, and the concept of an output IV does not exist
> here because it has no defined purpose past the end of the message.
> 
> The generic CTS template takes advantage of this chaining capability of
> the CBC implementations, and as a result, happens to return an output
> IV, simply because it passes its IV buffer directly to the encapsulated
> CBC implementation, which operates on full blocks only, and always
> returns an IV. This output IV happens to match how RFC 3962 defines it,
> even though the CTS template itself does not contain any output IV logic
> whatsoever, and, for this reason, lacks any test vectors that exercise
> this accidental output IV generation.
> 
> The arm64 SIMD implementation of cts(cbc(aes)) does not use the generic
> CTS template at all, but instead, implements the CBC mode and ciphertext
> stealing directly, and therefore does not encapsule a CBC implementation
> that returns an output IV in the same way. The arm64 SIMD implementation
> complies with the specification and passes all internal tests, but when
> invoked by the SUNRPC code, fails to produce the expected output IV and
> causes its selftests to fail.
> 
> Given that the output IV is defined as the penultimate block (where the
> final block may smaller than the block size), we can quite easily derive
> it in the caller by copying the appropriate slice of ciphertext after
> encryption.
> 
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna@kernel.org>
> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Reported-by: Scott Mayhew <smayhew@redhat.com>
> Tested-by: Scott Mayhew <smayhew@redhat.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Thanks Ard. I always like a patch description with lots
of context. I agree that looking at the output IV is a
GSS Kerberos idiosyncrasy.

Since I'm responsible for the GSS Kerberos Kunit tests,
I can take this one for v6.4-rc1.


> ---
> net/sunrpc/auth_gss/gss_krb5_crypto.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index 212c5d57465a1bf5..22dca4647ee66b3e 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher *cipher, struct xdr_buf *buf,
> 
> ret = write_bytes_to_xdr_buf(buf, offset, data, len);
> 
> + /*
> + * CBC-CTS does not define an output IV but RFC 3962 defines it as the
> + * penultimate block of ciphertext, so copy that into the IV buffer
> + * before returning.
> + */
> + if (encrypt)
> + memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
> out:
> kfree(data);
> return ret;
> -- 
> 2.39.2
> 

--
Chuck Lever

RE: [PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output IV

[Elliott, Robert (Servers)]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Monday, May 1, 2023 9:04 AM
> Subject: [PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output
> IV
> 
...
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher *cipher,
> struct xdr_buf *buf,
> 
>  	ret = write_bytes_to_xdr_buf(buf, offset, data, len);
> 
> +	/*
> +	 * CBC-CTS does not define an output IV but RFC 3962 defines it as the
> +	 * penultimate block of ciphertext, so copy that into the IV buffer
> +	 * before returning.
> +	 */
> +	if (encrypt)
> +		memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
>  out:
>  	kfree(data);
>  	return ret;
> --
> 2.39.2

What about the decrypt (encrypt == 0) case?

That function supports both encrypt and decrypt operations,
and both of its callers mention this IV expectation:

gss_krb5_aes_encrypt:
        /* Make sure IV carries forward from any CBC results. */
        err = gss_krb5_cts_crypt(cipher, buf,
                                 offset + GSS_KRB5_TOK_HDR_LEN + cbcbytes,
                                 desc.iv, pages, 1);
gss_krb5_aes_decrypt:
        /* Make sure IV carries forward from any CBC results. */
        ret = gss_krb5_cts_crypt(cipher, &subbuf, cbcbytes, desc.iv, NULL, 0);

Re: [PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output IV

[Ard Biesheuvel]

On Mon, 1 May 2023 at 16:40, Elliott, Robert (Servers) <elliott@hpe.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Monday, May 1, 2023 9:04 AM
> > Subject: [PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS output
> > IV
> >
> ...
> > +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> > @@ -639,6 +639,13 @@ gss_krb5_cts_crypt(struct crypto_sync_skcipher *cipher,
> > struct xdr_buf *buf,
> >
> >       ret = write_bytes_to_xdr_buf(buf, offset, data, len);
> >
> > +     /*
> > +      * CBC-CTS does not define an output IV but RFC 3962 defines it as the
> > +      * penultimate block of ciphertext, so copy that into the IV buffer
> > +      * before returning.
> > +      */
> > +     if (encrypt)
> > +             memcpy(iv, data, crypto_sync_skcipher_ivsize(cipher));
> >  out:
> >       kfree(data);
> >       return ret;
> > --
> > 2.39.2
>
> What about the decrypt (encrypt == 0) case?
>
> That function supports both encrypt and decrypt operations,
> and both of its callers mention this IV expectation:
>
> gss_krb5_aes_encrypt:
>         /* Make sure IV carries forward from any CBC results. */
>         err = gss_krb5_cts_crypt(cipher, buf,
>                                  offset + GSS_KRB5_TOK_HDR_LEN + cbcbytes,
>                                  desc.iv, pages, 1);
> gss_krb5_aes_decrypt:
>         /* Make sure IV carries forward from any CBC results. */
>         ret = gss_krb5_cts_crypt(cipher, &subbuf, cbcbytes, desc.iv, NULL, 0);
>

This is something different: for some reason, this code chooses to
split the input into two parts, and passes the first into plain CBC,
and then passes the rest (which needs ciphertext stealing *) into
CBC-CTS configured with the same key. The net result should be
precisely the same, so I'm not sure why this is implemented like this,
but the consequence of this is that the final CBC ciphertext block
should be passed to the CTS-CBC routine as its input IV, and our CBC
implementations happen to return this via the IV buffer (to support
transparent chaining inside the crypto API implementations)

This patch is about what gss_krb5_cts_crypt() /returns/ to its caller
via desc.iv, which, as I explained, has no semantic significance, but
is covered by RFC 3962 test vectors nonetheless.

* note that the CS3 ciphertext stealing scheme we use in the crypto
API always reorders the final two blocks, even if the input length is
an integral multiple of the block size, so we always need the CTS
logic to take effect.