From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mimi Zohar Date: Thu, 26 Oct 2017 23:57:36 +0000 Subject: Re: [PATCH] KEYS: trusted: fix writing past end of buffer in trusted_read() Message-Id: <1509062256.5886.147.camel@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit List-Id: References: <20171026205744.105566-1-ebiggers3@gmail.com> In-Reply-To: <20171026205744.105566-1-ebiggers3@gmail.com> To: linux-security-module@vger.kernel.org On Thu, 2017-10-26 at 13:57 -0700, Eric Biggers wrote: > From: Eric Biggers > > When calling keyctl_read() on a key of type "trusted", if the > user-supplied buffer was too small, the kernel ignored the buffer length > and just wrote past the end of the buffer, potentially corrupting > userspace memory. Fix it by instead returning the size required, as per > the documentation for keyctl_read(). > > We also don't even fill the buffer at all in this case, as this is > slightly easier to implement than doing a short read, and either > behavior appears to be permitted. It also makes it match the behavior > of the "encrypted" key type. > > Fixes: d00a1c72f7f4 ("keys: add new trusted key-type") > Reported-by: Ben Hutchings > Cc: # v2.6.38+ > Signed-off-by: Eric Biggers Thanks! Reviewed-by: Mimi Zohar > --- > security/keys/trusted.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/security/keys/trusted.c b/security/keys/trusted.c > index bd85315cbfeb..98aa89ff7bfd 100644 > --- a/security/keys/trusted.c > +++ b/security/keys/trusted.c > @@ -1147,20 +1147,21 @@ static long trusted_read(const struct key *key, char __user *buffer, > p = dereference_key_locked(key); > if (!p) > return -EINVAL; > - if (!buffer || buflen <= 0) > - return 2 * p->blob_len; > - ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); > - if (!ascii_buf) > - return -ENOMEM; > > - bufp = ascii_buf; > - for (i = 0; i < p->blob_len; i++) > - bufp = hex_byte_pack(bufp, p->blob[i]); > - if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) { > + if (buffer && buflen >= 2 * p->blob_len) { > + ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); > + if (!ascii_buf) > + return -ENOMEM; > + > + bufp = ascii_buf; > + for (i = 0; i < p->blob_len; i++) > + bufp = hex_byte_pack(bufp, p->blob[i]); > + if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) { > + kzfree(ascii_buf); > + return -EFAULT; > + } > kzfree(ascii_buf); > - return -EFAULT; > } > - kzfree(ascii_buf); > return 2 * p->blob_len; > } > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:41250 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751553AbdJZX5q (ORCPT ); Thu, 26 Oct 2017 19:57:46 -0400 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v9QNvYN3064574 for ; Thu, 26 Oct 2017 19:57:46 -0400 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0a-001b2d01.pphosted.com with ESMTP id 2dur20ccgm-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 26 Oct 2017 19:57:45 -0400 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 27 Oct 2017 00:57:43 +0100 Subject: Re: [PATCH] KEYS: trusted: fix writing past end of buffer in trusted_read() From: Mimi Zohar To: Eric Biggers , keyrings@vger.kernel.org Cc: David Howells , Ben Hutchings , Xiao Yang , linux-security-module@vger.kernel.org, Eric Biggers , stable@vger.kernel.org Date: Thu, 26 Oct 2017 19:57:36 -0400 In-Reply-To: <20171026205744.105566-1-ebiggers3@gmail.com> References: <20171026205744.105566-1-ebiggers3@gmail.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Message-Id: <1509062256.5886.147.camel@linux.vnet.ibm.com> Sender: stable-owner@vger.kernel.org List-ID: On Thu, 2017-10-26 at 13:57 -0700, Eric Biggers wrote: > From: Eric Biggers > > When calling keyctl_read() on a key of type "trusted", if the > user-supplied buffer was too small, the kernel ignored the buffer length > and just wrote past the end of the buffer, potentially corrupting > userspace memory. Fix it by instead returning the size required, as per > the documentation for keyctl_read(). > > We also don't even fill the buffer at all in this case, as this is > slightly easier to implement than doing a short read, and either > behavior appears to be permitted. It also makes it match the behavior > of the "encrypted" key type. > > Fixes: d00a1c72f7f4 ("keys: add new trusted key-type") > Reported-by: Ben Hutchings > Cc: # v2.6.38+ > Signed-off-by: Eric Biggers Thanks! Reviewed-by: Mimi Zohar > --- > security/keys/trusted.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/security/keys/trusted.c b/security/keys/trusted.c > index bd85315cbfeb..98aa89ff7bfd 100644 > --- a/security/keys/trusted.c > +++ b/security/keys/trusted.c > @@ -1147,20 +1147,21 @@ static long trusted_read(const struct key *key, char __user *buffer, > p = dereference_key_locked(key); > if (!p) > return -EINVAL; > - if (!buffer || buflen <= 0) > - return 2 * p->blob_len; > - ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); > - if (!ascii_buf) > - return -ENOMEM; > > - bufp = ascii_buf; > - for (i = 0; i < p->blob_len; i++) > - bufp = hex_byte_pack(bufp, p->blob[i]); > - if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) { > + if (buffer && buflen >= 2 * p->blob_len) { > + ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); > + if (!ascii_buf) > + return -ENOMEM; > + > + bufp = ascii_buf; > + for (i = 0; i < p->blob_len; i++) > + bufp = hex_byte_pack(bufp, p->blob[i]); > + if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) { > + kzfree(ascii_buf); > + return -EFAULT; > + } > kzfree(ascii_buf); > - return -EFAULT; > } > - kzfree(ascii_buf); > return 2 * p->blob_len; > } > From mboxrd@z Thu Jan 1 00:00:00 1970 From: zohar@linux.vnet.ibm.com (Mimi Zohar) Date: Thu, 26 Oct 2017 19:57:36 -0400 Subject: [PATCH] KEYS: trusted: fix writing past end of buffer in trusted_read() In-Reply-To: <20171026205744.105566-1-ebiggers3@gmail.com> References: <20171026205744.105566-1-ebiggers3@gmail.com> Message-ID: <1509062256.5886.147.camel@linux.vnet.ibm.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Thu, 2017-10-26 at 13:57 -0700, Eric Biggers wrote: > From: Eric Biggers > > When calling keyctl_read() on a key of type "trusted", if the > user-supplied buffer was too small, the kernel ignored the buffer length > and just wrote past the end of the buffer, potentially corrupting > userspace memory. Fix it by instead returning the size required, as per > the documentation for keyctl_read(). > > We also don't even fill the buffer at all in this case, as this is > slightly easier to implement than doing a short read, and either > behavior appears to be permitted. It also makes it match the behavior > of the "encrypted" key type. > > Fixes: d00a1c72f7f4 ("keys: add new trusted key-type") > Reported-by: Ben Hutchings > Cc: # v2.6.38+ > Signed-off-by: Eric Biggers Thanks! Reviewed-by: Mimi Zohar > --- > security/keys/trusted.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/security/keys/trusted.c b/security/keys/trusted.c > index bd85315cbfeb..98aa89ff7bfd 100644 > --- a/security/keys/trusted.c > +++ b/security/keys/trusted.c > @@ -1147,20 +1147,21 @@ static long trusted_read(const struct key *key, char __user *buffer, > p = dereference_key_locked(key); > if (!p) > return -EINVAL; > - if (!buffer || buflen <= 0) > - return 2 * p->blob_len; > - ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); > - if (!ascii_buf) > - return -ENOMEM; > > - bufp = ascii_buf; > - for (i = 0; i < p->blob_len; i++) > - bufp = hex_byte_pack(bufp, p->blob[i]); > - if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) { > + if (buffer && buflen >= 2 * p->blob_len) { > + ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); > + if (!ascii_buf) > + return -ENOMEM; > + > + bufp = ascii_buf; > + for (i = 0; i < p->blob_len; i++) > + bufp = hex_byte_pack(bufp, p->blob[i]); > + if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) { > + kzfree(ascii_buf); > + return -EFAULT; > + } > kzfree(ascii_buf); > - return -EFAULT; > } > - kzfree(ascii_buf); > return 2 * p->blob_len; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html