Received-SPF: pass (google.com: domain of linux-nfs-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: <167951169462.22263.13708891630674211649.stgit@morisot.1015granger.net>
 <CAFX2JfnA=qk+=YGFjaE8nrmW=yUwthvWaeCN3X4k0065eXExuA@mail.gmail.com> <08CC5855-A1A1-4716-BC8A-2BC55E8B8B34@oracle.com>
In-Reply-To: <08CC5855-A1A1-4716-BC8A-2BC55E8B8B34@oracle.com>
From:   Anna Schumaker <schumaker.anna@gmail.com>
Date:   Wed, 22 Mar 2023 17:11:47 -0400
Message-ID: <CAFX2JfnPpSfSa5K1GnS+y98QPfyy+9iBDtuCiKShxbPTw7W6TQ@mail.gmail.com>
Subject: Re: [PATCH v1] SUNRPC: Fix a crash in gss_krb5_checksum()
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Chuck Lever <cel@kernel.org>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Mar 22, 2023 at 4:59=E2=80=AFPM Chuck Lever III <chuck.lever@oracle=
.com> wrote:
>
>
>
> > On Mar 22, 2023, at 4:55 PM, Anna Schumaker <schumaker.anna@gmail.com> =
wrote:
> >
> > On Wed, Mar 22, 2023 at 3:17=E2=80=AFPM Chuck Lever <cel@kernel.org> wr=
ote:
> >>
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >>
> >> Anna says:
> >>> KASAN reports [...] a slab-out-of-bounds in gss_krb5_checksum(),
> >>> and it can cause my client to panic when running cthon basic
> >>> tests with krb5p.
> >>
> >>> Running faddr2line gives me:
> >>>
> >>> gss_krb5_checksum+0x4b6/0x630:
> >>> ahash_request_free at
> >>> /home/anna/Programs/linux-nfs.git/./include/crypto/hash.h:619
> >>> (inlined by) gss_krb5_checksum at
> >>> /home/anna/Programs/linux-nfs.git/net/sunrpc/auth_gss/gss_krb5_crypto=
.c:358
> >>
> >> My diagnosis is that the memcpy() at the end of gss_krb5_checksum()
> >> reads past the end of the buffer containing the checksum data
> >> because the callers have ignored gss_krb5_checksum()'s API contract:
> >>
> >> * Caller provides the truncation length of the output token (h) in
> >> * cksumout.len.
> >>
> >> Instead they provide the fixed length of the hmac buffer. This
> >> length happens to be larger than the value returned by
> >> crypto_ahash_digestsize().
> >>
> >> Change these errant callers to work like krb5_etm_{en,de}crypt().
> >> As a defensive measure, bound the length of the byte copy at the
> >> end of gss_krb5_checksum().
> >>
> >> Kunit sez:
> >> Testing complete. Ran 68 tests: passed: 68
> >> Elapsed time: 81.680s total, 5.875s configuring, 75.610s building, 0.1=
03s running
> >>
> >> Reported-by: Anna Schumaker <schumaker.anna@gmail.com>
> >> Fixes: 8270dbfcebea ("SUNRPC: Obscure Kerberos integrity keys")
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >
> > This patch fixed the issue for me, thanks! Are you going to submit it
> > with a future 6.3-rc pull request, or should I?
>
> I'll queue it in nfsd-fixes.

Sounds good. Thanks!

Anna
>
>
> > Anna
> >
> >> ---
> >> net/sunrpc/auth_gss/gss_krb5_crypto.c |   10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_g=
ss/gss_krb5_crypto.c
> >> index 6c7c52eeed4f..212c5d57465a 100644
> >> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> >> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> >> @@ -353,7 +353,9 @@ gss_krb5_checksum(struct crypto_ahash *tfm, char *=
header, int hdrlen,
> >>        err =3D crypto_ahash_final(req);
> >>        if (err)
> >>                goto out_free_ahash;
> >> -       memcpy(cksumout->data, checksumdata, cksumout->len);
> >> +
> >> +       memcpy(cksumout->data, checksumdata,
> >> +              min_t(int, cksumout->len, crypto_ahash_digestsize(tfm))=
);
> >>
> >> out_free_ahash:
> >>        ahash_request_free(req);
> >> @@ -809,8 +811,7 @@ gss_krb5_aes_encrypt(struct krb5_ctx *kctx, u32 of=
fset,
> >>        buf->tail[0].iov_len +=3D GSS_KRB5_TOK_HDR_LEN;
> >>        buf->len +=3D GSS_KRB5_TOK_HDR_LEN;
> >>
> >> -       /* Do the HMAC */
> >> -       hmac.len =3D GSS_KRB5_MAX_CKSUM_LEN;
> >> +       hmac.len =3D kctx->gk5e->cksumlength;
> >>        hmac.data =3D buf->tail[0].iov_base + buf->tail[0].iov_len;
> >>
> >>        /*
> >> @@ -873,8 +874,7 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 of=
fset, u32 len,
> >>        if (ret)
> >>                goto out_err;
> >>
> >> -       /* Calculate our hmac over the plaintext data */
> >> -       our_hmac_obj.len =3D sizeof(our_hmac);
> >> +       our_hmac_obj.len =3D kctx->gk5e->cksumlength;
> >>        our_hmac_obj.data =3D our_hmac;
> >>        ret =3D gss_krb5_checksum(ahash, NULL, 0, &subbuf, 0, &our_hmac=
_obj);
> >>        if (ret)
>
> --
> Chuck Lever
>
>