ima: avoid appraise error for hash calc interrupt
Subsystems (4)
PATCH 1/1 ima: avoid appraise error for hash calc interrupt
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().
--- security/integrity/ima/ima_crypto.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
security/integrity/ima/ima_crypto.c +3 -1
@@ -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);
Signature · 1 line
2.8.3
security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_tfm(struct file *file, · line 369
Quoted 6 lines · Patrick Callaghan
security/integrity/ima/ima_crypto.c +3
-1-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;Quoted 1 line
rc = crypto_shash_update(shash, rbuf, rbuf_len);
-lakshmi
Mimi Zohar
maintainerQuoted 12 lines · Lakshmi Ramasubramanian
security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_tfm(struct file *file, · line 369
Quoted 6 lines · Patrick Callaghan
security/integrity/ima/ima_crypto.c +3
-1-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 thani_sizebefore callingcypto_shash_update(sincerbuf_lenis one of the parameters for this call)?
The "while" statement enforces that.
Mimi
Quoted 11 lines
if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) { rc = -EINVAL; break; } offset += rbuf_len;Quoted 1 line
rc = crypto_shash_update(shash, rbuf, rbuf_len);-lakshmi
Quoted 14 lines · Mimi Zohar
Quoted 12 lines · Lakshmi Ramasubramanian
security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_tfm(struct file *file, · line 369
Quoted 6 lines · Patrick Callaghan
security/integrity/ima/ima_crypto.c +3
-1-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 thani_sizebefore callingcypto_shash_update(sincerbuf_lenis one of the parameters for this call)?
Quoted 3 lines
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
Quoted 9 lines
Quoted 9 lines
if ((rbuf_len == 0) || (offset + rbuf_len >= i_size)) { rc = -EINVAL; break; } offset += rbuf_len;Quoted 1 line
rc = crypto_shash_update(shash, rbuf, rbuf_len);
Mimi Zohar
maintainerQuoted 20 lines · Lakshmi Ramasubramanian
Quoted 14 lines · Mimi Zohar
Quoted 12 lines · Lakshmi Ramasubramanian
security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_tfm(struct file *file, · line 369
Quoted 6 lines · Patrick Callaghan
security/integrity/ima/ima_crypto.c +3
-1-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 thani_sizebefore callingcypto_shash_update(sincerbuf_lenis one of the parameters for this call)?Quoted 3 lines
The "while" statement enforces that.
Mimi
Quoted 8 lines
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
Quoted 33 lines · Mimi Zohar
Quoted 32 lines · Lakshmi Ramasubramanian
Quoted 16 lines · Mimi Zohar
Quoted 14 lines · Lakshmi Ramasubramanian
security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_tfm(struct file *file, · line 369
Quoted 6 lines · Patrick Callaghan
security/integrity/ima/ima_crypto.c +3
-1-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 thani_sizebefore callingcypto_shash_update(sincerbuf_lenis
one of the parameters for this call)?Quoted 3 lines
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.
Quoted 8 lines
Quoted 1 line
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,
Quoted 6 lines
Hello Laks,
You suggested that the if statement of the patch change to the following:if ((
rbuf_len== 0) || (offset +rbuf_len>=i_size)) {
Quoted 8 lines
Unless the file size changed between the time that
i_sizewas set inima_calc_file_hash_tfm()and ani_size_read()call was subsequently issued in a function downstream of theintegrity_kernel_read()call, therbuf_lenreturned on theintegrity_kernel_read()call will not be more thani_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
Quoted 45 lines · Lakshmi Ramasubramanian
On 11/14/19 5:55 AM, Patrick Callaghan wrote:
Hi Patrick,
Quoted 6 lines
Hello Laks,
You suggested that the if statement of the patch change to the following:if ((
rbuf_len== 0) || (offset +rbuf_len>=i_size)) {Quoted 15 lines
Unless the file size changed between the time that
i_sizewas set inima_calc_file_hash_tfm()and ani_size_read()call was subsequently issued in a function downstream of theintegrity_kernel_read()call,
therbuf_lenreturned on theintegrity_kernel_read()call will not be
more thani_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, butrbuf_len, returned byintegrity_kernel_read()is passed as buffer size tocrypto_shash_update()without any validation (rbuf_len<=PAGE_SIZE)
Quoted 8 lines
It is assumed here that
integrity_kernel_read()would not return a length greater than rbuf size and hencecrypto_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.
Quoted 9 lines · Patrick Callaghan
Hello Laks,
Agreed. The assumption is thatintegrity_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
Quoted 22 lines · Patrick Callaghan
The
integrity_kernel_read()call inima_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 inima_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(-)security/integrity/ima/ima_crypto.c +0
-0
security/integrity/ima/ima_crypto.c static int ima_calc_file_hash_tfm(struct file *file, · line 368
Quoted 8 lines
security/integrity/ima/ima_crypto.c +3
-1@@ -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:
Sascha
Signature · 4 lines
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 |