[PATCH 0/2] ima: limit both open-writers and ToMToU violations

[PATCH 0/2] ima: limit both open-writers and ToMToU violations

[Mimi Zohar]

Each time a file in policy, that is already opened for write, is opened
for read an open-writers integrity violation audit message is emitted
and a violation record is added to the IMA measurement list, even if an
open-writers violation has already been recorded.

Similalry each time a file in policy, that is already opened for read,
is opened for write a Time-of-Measure-Time-of-Use (ToMToU) integrity
violation audit message is emitted and a violation record is added to
the IMA measurement list, even if a ToMToU violation has already been
recorded.

Minimize the violations in the audit log and the IMA measurement list.


Mimi Zohar (2):
  ima: limit the number of open-writers integrity violations
  ima: limit the number of ToMToU integrity violations

 security/integrity/ima/ima.h      |  1 +
 security/integrity/ima/ima_main.c | 16 ++++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

--
2.48.1

[PATCH 1/2] ima: limit the number of open-writers integrity violations

[Mimi Zohar]

Each time a file in policy, that is already opened for write, is opened
for read an open-writers integrity violation audit message is emitted
and a violation record is added to the IMA measurement list, even if an
open-writers violation has already been recorded.

Limit the number of open-writers integrity violations for an existing
file open for write to one.  After the existing file open for write
closes (__fput), subsequent open-writers integrity violations may occur.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
Change log v1:
- Basesd on Stefan's RFC comments, updated the patch description and code.

 security/integrity/ima/ima.h      |  1 +
 security/integrity/ima/ima_main.c | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a4f284bd846c..7f21568544dd 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -182,6 +182,7 @@ struct ima_kexec_hdr {
 #define IMA_CHANGE_ATTR		2
 #define IMA_DIGSIG		3
 #define IMA_MUST_MEASURE	4
+#define IMA_LIMIT_VIOLATIONS	5

 /* IMA integrity metadata associated with an inode */
 struct ima_iint_cache {
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 28b8b0db6f9b..cde3ae55d654 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -137,8 +137,13 @@ static void ima_rdwr_violation_check(struct file *file,
 	} else {
 		if (must_measure)
 			set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
-		if (inode_is_open_for_write(inode) && must_measure)
-			send_writers = true;
+
+		/* Limit number of open_writers violations */
+		if (inode_is_open_for_write(inode) && must_measure) {
+			if (!test_and_set_bit(IMA_LIMIT_VIOLATIONS,
+					      &iint->atomic_flags))
+				send_writers = true;
+		}
 	}

 	if (!send_tomtou && !send_writers)
@@ -167,6 +172,8 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
 	if (atomic_read(&inode->i_writecount) == 1) {
 		struct kstat stat;

+		clear_bit(IMA_LIMIT_VIOLATIONS, &iint->atomic_flags);
+
 		update = test_and_clear_bit(IMA_UPDATE_XATTR,
 					    &iint->atomic_flags);
 		if ((iint->flags & IMA_NEW_FILE) ||
--
2.48.1

[PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Mimi Zohar]

Each time a file in policy, that is already opened for read, is opened
for write a Time-of-Measure-Time-of-Use (ToMToU) integrity violation
audit message is emitted and a violation record is added to the IMA
measurement list, even if a ToMToU violation has already been recorded.

Limit the number of ToMToU integrity violations for an existing file
open for read.

Note: The IMA_MUST_MEASURE atomic flag must be set from the reader side
based on policy.  This may result in a per open reader additional ToMToU
violation.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cde3ae55d654..f1671799a11b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -129,9 +129,10 @@ static void ima_rdwr_violation_check(struct file *file,
 		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
 			if (!iint)
 				iint = ima_iint_find(inode);
+
 			/* IMA_MEASURE is set from reader side */
-			if (iint && test_bit(IMA_MUST_MEASURE,
-						&iint->atomic_flags))
+			if (iint && test_and_clear_bit(IMA_MUST_MEASURE,
+						       &iint->atomic_flags))
 				send_tomtou = true;
 		}
 	} else {
-- 
2.48.1

Re: [PATCH 1/2] ima: limit the number of open-writers integrity violations

[Stefan Berger]

On 2/19/25 11:21 AM, Mimi Zohar wrote:
> Each time a file in policy, that is already opened for write, is opened
> for read an open-writers integrity violation audit message is emitted
> and a violation record is added to the IMA measurement list, even if an
> open-writers violation has already been recorded.
> 
> Limit the number of open-writers integrity violations for an existing
> file open for write to one.  After the existing file open for write
> closes (__fput), subsequent open-writers integrity violations may occur.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
> Change log v1:
> - Basesd on Stefan's RFC comments, updated the patch description and code.
> 
>   security/integrity/ima/ima.h      |  1 +
>   security/integrity/ima/ima_main.c | 11 +++++++++--
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index a4f284bd846c..7f21568544dd 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -182,6 +182,7 @@ struct ima_kexec_hdr {
>   #define IMA_CHANGE_ATTR		2
>   #define IMA_DIGSIG		3
>   #define IMA_MUST_MEASURE	4
> +#define IMA_LIMIT_VIOLATIONS	5
> 
>   /* IMA integrity metadata associated with an inode */
>   struct ima_iint_cache {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 28b8b0db6f9b..cde3ae55d654 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -137,8 +137,13 @@ static void ima_rdwr_violation_check(struct file *file,
>   	} else {
>   		if (must_measure)
>   			set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
> -		if (inode_is_open_for_write(inode) && must_measure)
> -			send_writers = true;
> +
> +		/* Limit number of open_writers violations */
> +		if (inode_is_open_for_write(inode) && must_measure) {
> +			if (!test_and_set_bit(IMA_LIMIT_VIOLATIONS,
> +					      &iint->atomic_flags))
> +				send_writers = true;
> +		}
>   	}
> 
>   	if (!send_tomtou && !send_writers)
> @@ -167,6 +172,8 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
>   	if (atomic_read(&inode->i_writecount) == 1) {
>   		struct kstat stat;
> 
> +		clear_bit(IMA_LIMIT_VIOLATIONS, &iint->atomic_flags);
> +
>   		update = test_and_clear_bit(IMA_UPDATE_XATTR,
>   					    &iint->atomic_flags);
>   		if ((iint->flags & IMA_NEW_FILE) ||
> --
> 2.48.1
> 

Re: [PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Stefan Berger]

On 2/19/25 11:21 AM, Mimi Zohar wrote:
> Each time a file in policy, that is already opened for read, is opened
> for write a Time-of-Measure-Time-of-Use (ToMToU) integrity violation
> audit message is emitted and a violation record is added to the IMA
> measurement list, even if a ToMToU violation has already been recorded.
> 
> Limit the number of ToMToU integrity violations for an existing file
> open for read.
> 
> Note: The IMA_MUST_MEASURE atomic flag must be set from the reader side
> based on policy.  This may result in a per open reader additional ToMToU
> violation.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   security/integrity/ima/ima_main.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index cde3ae55d654..f1671799a11b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -129,9 +129,10 @@ static void ima_rdwr_violation_check(struct file *file,
>   		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
>   			if (!iint)
>   				iint = ima_iint_find(inode);
> +
>   			/* IMA_MEASURE is set from reader side */
> -			if (iint && test_bit(IMA_MUST_MEASURE,
> -						&iint->atomic_flags))
> +			if (iint && test_and_clear_bit(IMA_MUST_MEASURE,
> +						       &iint->atomic_flags))
>   				send_tomtou = true;
>   		}
>   	} else {

Re: [PATCH 1/2] ima: limit the number of open-writers integrity violations

[Petr Vorel]

Hi Mimi,

> Each time a file in policy, that is already opened for write, is opened
> for read an open-writers integrity violation audit message is emitted
> and a violation record is added to the IMA measurement list, even if an
> open-writers violation has already been recorded.

> Limit the number of open-writers integrity violations for an existing
> file open for write to one.  After the existing file open for write
> closes (__fput), subsequent open-writers integrity violations may occur.

LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

I also did a regression testing on LTP IMA tests on x86_64, aarch64, ppc64le.
(not testing the feature itself, just really a very basic regression testing,
therefore I do not dare to add my TBT).

Kind regards,
Petr

Re: [PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Petr Vorel]

Hi Mimi,

LGTM.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

Re: [PATCH 1/2] ima: limit the number of open-writers integrity violations

[Petr Vorel]

Hi Mimi,

Tested-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

Re: [PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Petr Vorel]

Hi Mimi,

Tested-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

Re: [PATCH 0/2] ima: limit both open-writers and ToMToU violations

[Roberto Sassu]

On Wed, 2025-02-19 at 11:21 -0500, Mimi Zohar wrote:

Hi Mimi

> Each time a file in policy, that is already opened for write, is opened
> for read an open-writers integrity violation audit message is emitted

I would put a comma after 'for read' and remove the previous ones.

> and a violation record is added to the IMA measurement list, even if an

I would stop the sentence before 'even' and start a new sentence.

IMA does not track previous violations, and emits a new one of the same
kind, even if there was one before, resulting in redundant information
being produced.

The information might not be redundant though, if process-based
credentials are added to the measurement list. In that case, more
information about the process causing the violation would be shown.

> open-writers violation has already been recorded.
> 
> Similalry each time a file in policy, that is already opened for read,

Typo.

> is opened for write a Time-of-Measure-Time-of-Use (ToMToU) integrity
> violation audit message is emitted and a violation record is added to
> the IMA measurement list, even if a ToMToU violation has already been
> recorded.
> 
> Minimize the violations in the audit log and the IMA measurement list.

I would describe more precisely how you are trying to minimize them.

Thanks

Roberto

> Mimi Zohar (2):
>   ima: limit the number of open-writers integrity violations
>   ima: limit the number of ToMToU integrity violations
> 
>  security/integrity/ima/ima.h      |  1 +
>  security/integrity/ima/ima_main.c | 16 ++++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> --
> 2.48.1
> 

Re: [PATCH 1/2] ima: limit the number of open-writers integrity violations

[Roberto Sassu]

On Wed, 2025-02-19 at 11:21 -0500, Mimi Zohar wrote:
> Each time a file in policy, that is already opened for write, is opened
> for read an open-writers integrity violation audit message is emitted
> and a violation record is added to the IMA measurement list, even if an
> open-writers violation has already been recorded.
> 
> Limit the number of open-writers integrity violations for an existing
> file open for write to one.  After the existing file open for write
> closes (__fput), subsequent open-writers integrity violations may occur.

More precisely, may be emitted again.

> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> Change log v1:
> - Basesd on Stefan's RFC comments, updated the patch description and code.

Based.

Could be also useful to have here what is changed.

>  security/integrity/ima/ima.h      |  1 +
>  security/integrity/ima/ima_main.c | 11 +++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index a4f284bd846c..7f21568544dd 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -182,6 +182,7 @@ struct ima_kexec_hdr {
>  #define IMA_CHANGE_ATTR		2
>  #define IMA_DIGSIG		3
>  #define IMA_MUST_MEASURE	4
> +#define IMA_LIMIT_VIOLATIONS	5

I like to name variables in a way that it is clear what the intent is.
Thinking about it, maybe:

IMA_OPEN_WRITERS_EMITTED

Thanks

Roberto

>  /* IMA integrity metadata associated with an inode */
>  struct ima_iint_cache {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 28b8b0db6f9b..cde3ae55d654 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -137,8 +137,13 @@ static void ima_rdwr_violation_check(struct file *file,
>  	} else {
>  		if (must_measure)
>  			set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
> -		if (inode_is_open_for_write(inode) && must_measure)
> -			send_writers = true;
> +
> +		/* Limit number of open_writers violations */
> +		if (inode_is_open_for_write(inode) && must_measure) {
> +			if (!test_and_set_bit(IMA_LIMIT_VIOLATIONS,
> +					      &iint->atomic_flags))
> +				send_writers = true;
> +		}
>  	}
> 
>  	if (!send_tomtou && !send_writers)
> @@ -167,6 +172,8 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
>  	if (atomic_read(&inode->i_writecount) == 1) {
>  		struct kstat stat;
> 
> +		clear_bit(IMA_LIMIT_VIOLATIONS, &iint->atomic_flags);
> +
>  		update = test_and_clear_bit(IMA_UPDATE_XATTR,
>  					    &iint->atomic_flags);
>  		if ((iint->flags & IMA_NEW_FILE) ||
> --
> 2.48.1
> 

Re: [PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Roberto Sassu]

On Wed, 2025-02-19 at 11:21 -0500, Mimi Zohar wrote:
> Each time a file in policy, that is already opened for read, is opened
> for write a Time-of-Measure-Time-of-Use (ToMToU) integrity violation
> audit message is emitted and a violation record is added to the IMA
> measurement list, even if a ToMToU violation has already been recorded.
> 
> Limit the number of ToMToU integrity violations for an existing file
> open for read.
> 
> Note: The IMA_MUST_MEASURE atomic flag must be set from the reader side
> based on policy.  This may result in a per open reader additional ToMToU
> violation.

Probably the goal can be summarized as to limit emitting consecutive
ToMToU violations.

In the previous patch, we are not emitting a new open_writers violation
until all writers close the file. Here, it is a bit different, we are
not emitting an additional ToMToU violation until there is another
reader matching the policy. Maybe we should highlight this difference.

> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  security/integrity/ima/ima_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index cde3ae55d654..f1671799a11b 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -129,9 +129,10 @@ static void ima_rdwr_violation_check(struct file *file,
>  		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
>  			if (!iint)
>  				iint = ima_iint_find(inode);
> +
>  			/* IMA_MEASURE is set from reader side */
> -			if (iint && test_bit(IMA_MUST_MEASURE,
> -						&iint->atomic_flags))
> +			if (iint && test_and_clear_bit(IMA_MUST_MEASURE,

Since IMA_MUST_MEASURE is only used for violations, what if we rename
it to:

IMA_TOMTOU_MAY_EMIT

Thanks

Roberto

> +						       &iint->atomic_flags))
>  				send_tomtou = true;
>  		}
>  	} else {

Re: [PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Mimi Zohar]

Hi Roberto,

On Fri, 2025-02-21 at 18:36 +0100, Roberto Sassu wrote:
> On Wed, 2025-02-19 at 11:21 -0500, Mimi Zohar wrote:
> > Each time a file in policy, that is already opened for read, is opened
> > for write a Time-of-Measure-Time-of-Use (ToMToU) integrity violation
> > audit message is emitted and a violation record is added to the IMA
> > measurement list, even if a ToMToU violation has already been recorded.
> > 
> > Limit the number of ToMToU integrity violations for an existing file
> > open for read.
> > 
> > Note: The IMA_MUST_MEASURE atomic flag must be set from the reader side
> > based on policy.  This may result in a per open reader additional ToMToU
> > violation.
> 
> Probably the goal can be summarized as to limit emitting consecutive
> ToMToU violations.

Other audit messages and measurements could have been emitted, so they may not
be consecutive.

> 
> In the previous patch, we are not emitting a new open_writers violation
> until all writers close the file. Here, it is a bit different, we are
> not emitting an additional ToMToU violation until there is another
> reader matching the policy. Maybe we should highlight this difference.
> 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  security/integrity/ima/ima_main.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index cde3ae55d654..f1671799a11b 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -129,9 +129,10 @@ static void ima_rdwr_violation_check(struct file *file,
> >  		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
> >  			if (!iint)
> >  				iint = ima_iint_find(inode);
> > +
> >  			/* IMA_MEASURE is set from reader side */
> > -			if (iint && test_bit(IMA_MUST_MEASURE,
> > -						&iint->atomic_flags))
> > +			if (iint && test_and_clear_bit(IMA_MUST_MEASURE,
> 
> Since IMA_MUST_MEASURE is only used for violations, what if we rename
> it to:
> 
> IMA_TOMTOU_MAY_EMIT

How about naming the atomic flags as IMA_MAY_EMIT_TOMTOU and
IMA_EMIT_OPENWRITERS?

Mimi

Re: [PATCH 2/2] ima: limit the number of ToMToU integrity violations

[Roberto Sassu]

On Wed, 2025-02-26 at 14:19 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> On Fri, 2025-02-21 at 18:36 +0100, Roberto Sassu wrote:
> > On Wed, 2025-02-19 at 11:21 -0500, Mimi Zohar wrote:
> > > Each time a file in policy, that is already opened for read, is opened
> > > for write a Time-of-Measure-Time-of-Use (ToMToU) integrity violation
> > > audit message is emitted and a violation record is added to the IMA
> > > measurement list, even if a ToMToU violation has already been recorded.
> > > 
> > > Limit the number of ToMToU integrity violations for an existing file
> > > open for read.
> > > 
> > > Note: The IMA_MUST_MEASURE atomic flag must be set from the reader side
> > > based on policy.  This may result in a per open reader additional ToMToU
> > > violation.
> > 
> > Probably the goal can be summarized as to limit emitting consecutive
> > ToMToU violations.
> 
> Other audit messages and measurements could have been emitted, so they may not
> be consecutive.

Ah, sorry, not well expressed. I meant if there is a second ToMToU
violation after the first (read -> write -> write). Not consecutive
means when there is a new measurement (more correct would be when there
is a new policy match) on the same file to be invalidated.

> > 
> > In the previous patch, we are not emitting a new open_writers violation
> > until all writers close the file. Here, it is a bit different, we are
> > not emitting an additional ToMToU violation until there is another
> > reader matching the policy. Maybe we should highlight this difference.
> > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > > ---
> > >  security/integrity/ima/ima_main.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index cde3ae55d654..f1671799a11b 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -129,9 +129,10 @@ static void ima_rdwr_violation_check(struct file *file,
> > >  		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
> > >  			if (!iint)
> > >  				iint = ima_iint_find(inode);
> > > +
> > >  			/* IMA_MEASURE is set from reader side */
> > > -			if (iint && test_bit(IMA_MUST_MEASURE,
> > > -						&iint->atomic_flags))
> > > +			if (iint && test_and_clear_bit(IMA_MUST_MEASURE,
> > 
> > Since IMA_MUST_MEASURE is only used for violations, what if we rename
> > it to:
> > 
> > IMA_TOMTOU_MAY_EMIT
> 
> How about naming the atomic flags as IMA_MAY_EMIT_TOMTOU and
> IMA_EMIT_OPENWRITERS?

Yes, I like them.

Thanks

Roberto