Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230501140408.2648535-1-ardb@kernel.org> <MW5PR84MB18421BA9A3772DC313D06E5BAB6E9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
In-Reply-To: <MW5PR84MB18421BA9A3772DC313D06E5BAB6E9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Mon, 1 May 2023 16:55:02 +0200
Message-ID: <CAMj1kXFBrPR9geZRszsWsZshuyJE351Zi9a+vGm0LBdZZxQQgw@mail.gmail.com>
Subject: Re: [PATCH] SUNRPC: Avoid relying on crypto API to derive CBC-CTS
 output IV
To:     "Elliott, Robert (Servers)" <elliott@hpe.com>
Cc:     "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        "ebiggers@kernel.org" <ebiggers@kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Chuck Lever <chuck.lever@oracle.com>,
        Jeff Layton <jlayton@kernel.org>,
        Scott Mayhew <smayhew@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.