| Message ID | 167951169462.22263.13708891630674211649.stgit@morisot.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] SUNRPC: Fix a crash in gss_krb5_checksum() | expand |
On Wed, Mar 22, 2023 at 3:17 PM Chuck Lever <cel@kernel.org> wrote: > > 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.103s 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? 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_gss/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 = 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 offset, > buf->tail[0].iov_len += GSS_KRB5_TOK_HDR_LEN; > buf->len += GSS_KRB5_TOK_HDR_LEN; > > - /* Do the HMAC */ > - hmac.len = GSS_KRB5_MAX_CKSUM_LEN; > + hmac.len = kctx->gk5e->cksumlength; > hmac.data = buf->tail[0].iov_base + buf->tail[0].iov_len; > > /* > @@ -873,8 +874,7 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, u32 len, > if (ret) > goto out_err; > > - /* Calculate our hmac over the plaintext data */ > - our_hmac_obj.len = sizeof(our_hmac); > + our_hmac_obj.len = kctx->gk5e->cksumlength; > our_hmac_obj.data = our_hmac; > ret = gss_krb5_checksum(ahash, NULL, 0, &subbuf, 0, &our_hmac_obj); > if (ret) > >
> On Mar 22, 2023, at 4:55 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote: > > On Wed, Mar 22, 2023 at 3:17 PM Chuck Lever <cel@kernel.org> wrote: >> >> 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.103s 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. > 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_gss/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 = 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 offset, >> buf->tail[0].iov_len += GSS_KRB5_TOK_HDR_LEN; >> buf->len += GSS_KRB5_TOK_HDR_LEN; >> >> - /* Do the HMAC */ >> - hmac.len = GSS_KRB5_MAX_CKSUM_LEN; >> + hmac.len = kctx->gk5e->cksumlength; >> hmac.data = buf->tail[0].iov_base + buf->tail[0].iov_len; >> >> /* >> @@ -873,8 +874,7 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, u32 len, >> if (ret) >> goto out_err; >> >> - /* Calculate our hmac over the plaintext data */ >> - our_hmac_obj.len = sizeof(our_hmac); >> + our_hmac_obj.len = kctx->gk5e->cksumlength; >> our_hmac_obj.data = our_hmac; >> ret = gss_krb5_checksum(ahash, NULL, 0, &subbuf, 0, &our_hmac_obj); >> if (ret) -- Chuck Lever
On Wed, Mar 22, 2023 at 4:59 PM 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 PM Chuck Lever <cel@kernel.org> wrote: > >> > >> 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.103s 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_gss/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 = 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 offset, > >> buf->tail[0].iov_len += GSS_KRB5_TOK_HDR_LEN; > >> buf->len += GSS_KRB5_TOK_HDR_LEN; > >> > >> - /* Do the HMAC */ > >> - hmac.len = GSS_KRB5_MAX_CKSUM_LEN; > >> + hmac.len = kctx->gk5e->cksumlength; > >> hmac.data = buf->tail[0].iov_base + buf->tail[0].iov_len; > >> > >> /* > >> @@ -873,8 +874,7 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, u32 len, > >> if (ret) > >> goto out_err; > >> > >> - /* Calculate our hmac over the plaintext data */ > >> - our_hmac_obj.len = sizeof(our_hmac); > >> + our_hmac_obj.len = kctx->gk5e->cksumlength; > >> our_hmac_obj.data = our_hmac; > >> ret = gss_krb5_checksum(ahash, NULL, 0, &subbuf, 0, &our_hmac_obj); > >> if (ret) > > -- > Chuck Lever > >
diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/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 = 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 offset, buf->tail[0].iov_len += GSS_KRB5_TOK_HDR_LEN; buf->len += GSS_KRB5_TOK_HDR_LEN; - /* Do the HMAC */ - hmac.len = GSS_KRB5_MAX_CKSUM_LEN; + hmac.len = kctx->gk5e->cksumlength; hmac.data = buf->tail[0].iov_base + buf->tail[0].iov_len; /* @@ -873,8 +874,7 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, u32 len, if (ret) goto out_err; - /* Calculate our hmac over the plaintext data */ - our_hmac_obj.len = sizeof(our_hmac); + our_hmac_obj.len = kctx->gk5e->cksumlength; our_hmac_obj.data = our_hmac; ret = gss_krb5_checksum(ahash, NULL, 0, &subbuf, 0, &our_hmac_obj); if (ret)