The integrity_kernel_read() call in ima_calc_file_hash_tfm() can return
a value of 0 before all bytes of the file are read. A value of 0 would
normally indicate an EOF. This has been observed if a user process is
causing a file appraisal and is terminated with a SIGTERM signal. The
most common occurrence of seeing the problem is if a shutdown or systemd
reload is initiated while files are being appraised.
The problem is similar to commit <f5e1040196db> (ima: always return
negative code for error) that fixed the problem in
ima_calc_file_hash_atfm().
Suggested-by: Mimi Zohar <[email protected]>
Signed-off-by: Patrick Callaghan <[email protected]>
---
security/integrity/ima/ima_crypto.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 73044fc..7967a69 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -362,8 +362,10 @@ static int ima_calc_file_hash_tfm(struct file *file,
rc = rbuf_len;
break;
}
- if (rbuf_len == 0)
+ if (rbuf_len == 0) { /* unexpected EOF */
+ rc = -EINVAL;
break;
+ }
offset += rbuf_len;
rc = crypto_shash_update(shash, rbuf, rbuf_len);
--
2.8.3
On 11/11/19 11:23 AM, Patrick Callaghan wrote:
> - if (rbuf_len == 0)
> + if (rbuf_len == 0) { /* unexpected EOF */
> + rc = -EINVAL;
> break;
> + }
> offset += rbuf_len;
Should there be an additional check to validate that (offset + rbuf_len)
is less than i_size before calling cypto_shash_update (since rbuf_len is
one of the parameters for this call)?
if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
rc = -EINVAL;
break;
}
offset += rbuf_len;
> rc = crypto_shash_update(shash, rbuf, rbuf_len);
-lakshmi
On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
> On 11/11/19 11:23 AM, Patrick Callaghan wrote:
>
> > - if (rbuf_len == 0)
> > + if (rbuf_len == 0) { /* unexpected EOF */
> > + rc = -EINVAL;
> > break;
> > + }
> > offset += rbuf_len;
>
> Should there be an additional check to validate that (offset + rbuf_len)
> is less than i_size before calling cypto_shash_update (since rbuf_len is
> one of the parameters for this call)?
The "while" statement enforces that.
Mimi
>
> if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
> rc = -EINVAL;
> break;
> }
> offset += rbuf_len;
>
> > rc = crypto_shash_update(shash, rbuf, rbuf_len);
>
> -lakshmi
>
On 11/12/2019 9:14 AM, Mimi Zohar wrote:
> On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
>> On 11/11/19 11:23 AM, Patrick Callaghan wrote:
>>
>>> - if (rbuf_len == 0)
>>> + if (rbuf_len == 0) { /* unexpected EOF */
>>> + rc = -EINVAL;
>>> break;
>>> + }
>>> offset += rbuf_len;
>>
>> Should there be an additional check to validate that (offset + rbuf_len)
>> is less than i_size before calling cypto_shash_update (since rbuf_len is
>> one of the parameters for this call)?
>
> The "while" statement enforces that.
>
> Mimi
Yes - but that check happens after the call to crypto_shash_update().
Perhaps integrity_kernel_read() will never return (rbuf_len) that will
=> violate the check in the "while" statement.
=> number of bytes read that is greater than the memory allocated for
rbuf even in error conditions.
Just making sure.
thanks,
-lakshmi
>
>>
>> if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
>> rc = -EINVAL;
>> break;
>> }
>> offset += rbuf_len;
>>
>>> rc = crypto_shash_update(shash, rbuf, rbuf_len);
On Tue, 2019-11-12 at 09:33 -0800, Lakshmi Ramasubramanian wrote:
> On 11/12/2019 9:14 AM, Mimi Zohar wrote:
>
> > On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
> >> On 11/11/19 11:23 AM, Patrick Callaghan wrote:
> >>
> >>> - if (rbuf_len == 0)
> >>> + if (rbuf_len == 0) { /* unexpected EOF */
> >>> + rc = -EINVAL;
> >>> break;
> >>> + }
> >>> offset += rbuf_len;
> >>
> >> Should there be an additional check to validate that (offset + rbuf_len)
> >> is less than i_size before calling cypto_shash_update (since rbuf_len is
> >> one of the parameters for this call)?
> >
> > The "while" statement enforces that.
> >
> > Mimi
>
> Yes - but that check happens after the call to crypto_shash_update().
>
> Perhaps integrity_kernel_read() will never return (rbuf_len) that will
> => violate the check in the "while" statement.
> => number of bytes read that is greater than the memory allocated for
> rbuf even in error conditions.
>
> Just making sure.
integrity_kernel_read() returns an error (< 0) or the number of bytes
read. The while statement ensures that there is more data to read, so
returning 0 is always an error.
Mimi
On Mon, Nov 11, 2019 at 02:23:48PM -0500, Patrick Callaghan wrote:
> The integrity_kernel_read() call in ima_calc_file_hash_tfm() can return
> a value of 0 before all bytes of the file are read. A value of 0 would
> normally indicate an EOF. This has been observed if a user process is
> causing a file appraisal and is terminated with a SIGTERM signal. The
> most common occurrence of seeing the problem is if a shutdown or systemd
> reload is initiated while files are being appraised.
>
> The problem is similar to commit <f5e1040196db> (ima: always return
> negative code for error) that fixed the problem in
> ima_calc_file_hash_atfm().
>
> Suggested-by: Mimi Zohar <[email protected]>
> Signed-off-by: Patrick Callaghan <[email protected]>
> ---
> security/integrity/ima/ima_crypto.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 73044fc..7967a69 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -362,8 +362,10 @@ static int ima_calc_file_hash_tfm(struct file *file,
> rc = rbuf_len;
> break;
> }
> - if (rbuf_len == 0)
> + if (rbuf_len == 0) { /* unexpected EOF */
> + rc = -EINVAL;
> break;
> + }
There's no point in calling crypto_shash_final() on incomplete data, so
setting rc to an error to avoid that seems the right thing to do to me,
so:
Reviewed-by: Sascha Hauer <[email protected]>
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, 2019-11-12 at 13:12 -0500, Mimi Zohar wrote:
> On Tue, 2019-11-12 at 09:33 -0800, Lakshmi Ramasubramanian wrote:
> > On 11/12/2019 9:14 AM, Mimi Zohar wrote:
> >
> > > On Mon, 2019-11-11 at 14:29 -0800, Lakshmi Ramasubramanian wrote:
> > > > On 11/11/19 11:23 AM, Patrick Callaghan wrote:
> > > >
> > > > > - if (rbuf_len == 0)
> > > > > + if (rbuf_len == 0) { /* unexpected EOF */
> > > > > + rc = -EINVAL;
> > > > > break;
> > > > > + }
> > > > > offset += rbuf_len;
> > > >
> > > > Should there be an additional check to validate that (offset +
> > > > rbuf_len)
> > > > is less than i_size before calling cypto_shash_update (since
> > > > rbuf_len is
> > > > one of the parameters for this call)?
> > >
> > > The "while" statement enforces that.
> > >
> > > Mimi
> >
> > Yes - but that check happens after the call to
> > crypto_shash_update().
> >
> > Perhaps integrity_kernel_read() will never return (rbuf_len) that
> > will
> > => violate the check in the "while" statement.
> > => number of bytes read that is greater than the memory allocated
> > for
> > rbuf even in error conditions.
> >
> > Just making sure.
>
> integrity_kernel_read() returns an error (< 0) or the number of bytes
> read. The while statement ensures that there is more data to read,
> so
> returning 0 is always an error.
>
> Mimi
Hello Laks,
You suggested that the if statement of the patch change to the
following:
if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
Unless the file size changed between the time that i_size was set in
ima_calc_file_hash_tfm() and an i_size_read() call was subsequently
issued in a function downstream of the integrity_kernel_read() call,
the rbuf_len returned on the integrity_kernel_read() call will not be
more than i_size - offset. I do not think that it is possible for the
file size to change during this window but nonetheless, if it can, this
would be a different problem and I would not want to include this in my
patch. That said, I do appreciate you taking time to review this patch.
On 11/14/19 5:55 AM, Patrick Callaghan wrote:
Hi Patrick,
> Hello Laks,
> You suggested that the if statement of the patch change to the
> following:
>
> if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
>
> Unless the file size changed between the time that i_size was set in
> ima_calc_file_hash_tfm() and an i_size_read() call was subsequently
> issued in a function downstream of the integrity_kernel_read() call,
> the rbuf_len returned on the integrity_kernel_read() call will not be
> more than i_size - offset. I do not think that it is possible for the
> file size to change during this window but nonetheless, if it can, this
> would be a different problem and I would not want to include this in my
> patch. That said, I do appreciate you taking time to review this patch.
You are right - unless the file size changes between the calls this
problem would not occur. I agree - that issue, even if it can occur,
should be addressed separately.
Another one (again - am not saying this needs to be addressed in this
patch, but just wanted to point out)
rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
...
rbuf_len = integrity_kernel_read(file, offset, rbuf, PAGE_SIZE);
...
rc = crypto_shash_update(shash, rbuf, rbuf_len);
rbuf is of size PAGE_SIZE, but rbuf_len, returned by
integrity_kernel_read() is passed as buffer size to
crypto_shash_update() without any validation (rbuf_len <= PAGE_SIZE)
It is assumed here that integrity_kernel_read() would not return a
length greater than rbuf size and hence crypto_shash_update() would
never access beyond the given buffer.
thanks,
-lakshmi
On Thu, 2019-11-14 at 10:45 -0800, Lakshmi Ramasubramanian wrote:
> On 11/14/19 5:55 AM, Patrick Callaghan wrote:
>
> Hi Patrick,
>
> > Hello Laks,
> > You suggested that the if statement of the patch change to the
> > following:
> >
> > if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) {
> >
> > Unless the file size changed between the time that i_size was set
> > in
> > ima_calc_file_hash_tfm() and an i_size_read() call was subsequently
> > issued in a function downstream of the integrity_kernel_read()
> > call,
> > the rbuf_len returned on the integrity_kernel_read() call will not
> > be
> > more than i_size - offset. I do not think that it is possible for
> > the
> > file size to change during this window but nonetheless, if it can,
> > this
> > would be a different problem and I would not want to include this
> > in my
> > patch. That said, I do appreciate you taking time to review this
> > patch.
>
> You are right - unless the file size changes between the calls this
> problem would not occur. I agree - that issue, even if it can occur,
> should be addressed separately.
>
> Another one (again - am not saying this needs to be addressed in
> this
> patch, but just wanted to point out)
>
> rbuf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> ...
> rbuf_len = integrity_kernel_read(file, offset, rbuf,
> PAGE_SIZE);
> ...
> rc = crypto_shash_update(shash, rbuf, rbuf_len);
>
> rbuf is of size PAGE_SIZE, but rbuf_len, returned by
> integrity_kernel_read() is passed as buffer size to
> crypto_shash_update() without any validation (rbuf_len <= PAGE_SIZE)
>
> It is assumed here that integrity_kernel_read() would not return a
> length greater than rbuf size and hence crypto_shash_update() would
> never access beyond the given buffer.
>
> thanks,
> -lakshmi
>
>
Hello Laks,
Agreed. The assumption is that integrity_kernel_read() function does
not return a value greater than the fourth parameter passed to it (i.e.
does not read more bytes from the file than the size of the buffer
passed to it). I tried to validate that this assumption was true by
following the code but felt I could not prove it with my current
knowledge of the code. If this assumption is not true then I believe
that any code change for this problem should go into a different
patch.
Thank you.
On 11/15/19 7:25 AM, Patrick Callaghan wrote:
> Hello Laks,
> Agreed. The assumption is that integrity_kernel_read() function does
> not return a value greater than the fourth parameter passed to it (i.e.
> does not read more bytes from the file than the size of the buffer
> passed to it). I tried to validate that this assumption was true by
> following the code but felt I could not prove it with my current
> knowledge of the code. If this assumption is not true then I believe
> that any code change for this problem should go into a different
> patch.
I agree Patrick - not a blocker for this patch set.
thanks,
-lakshmi