[PATCH 1/2] ipe: return -ESTALE instead of -EINVAL on update when new policy has a lower version

[PATCH 1/2] ipe: return -ESTALE instead of -EINVAL on update when new policy has a lower version

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

When loading policies in userspace we want a recognizable error when an
update attempts to use an old policy, as that is an error that needs
to be treated differently from an invalid policy. Use -ESTALE as it is
clear enough for an update mechanism.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
Found while adding policy loading to systemd, like we do for IMA/SELinux
https://github.com/systemd/systemd/pull/34418

 security/ipe/policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/ipe/policy.c b/security/ipe/policy.c
index 67a051620889..5de64441dfe7 100644
--- a/security/ipe/policy.c
+++ b/security/ipe/policy.c
@@ -116,7 +116,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
 	}
 
 	if (ver_to_u64(old) > ver_to_u64(new)) {
-		rc = -EINVAL;
+		rc = -ESTALE;
 		goto err;
 	}
 
-- 
2.39.5

[PATCH 2/2] ipe: also reject policy updates with the same version

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

Currently IPE accepts an update that has the same version as the policy
being updated, but it doesn't make it a no-op nor it checks that the
old and new policyes are the same. So it is possible to change the
content of a policy, without changing its version. This is very
confusing from userspace when managing policies.
Instead change the update logic to reject updates that have the same
version with ESTALE, as that is much clearer and intuitive behaviour.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 security/ipe/policy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/ipe/policy.c b/security/ipe/policy.c
index 5de64441dfe7..01da3a377e7f 100644
--- a/security/ipe/policy.c
+++ b/security/ipe/policy.c
@@ -115,7 +115,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
 		goto err;
 	}
 
-	if (ver_to_u64(old) > ver_to_u64(new)) {
+	if (ver_to_u64(old) >= ver_to_u64(new)) {
 		rc = -ESTALE;
 		goto err;
 	}
-- 
2.39.5

Re: [PATCH 2/2] ipe: also reject policy updates with the same version

[Serge E. Hallyn]

On Sun, Sep 22, 2024 at 03:56:14PM +0200, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
> Currently IPE accepts an update that has the same version as the policy
> being updated, but it doesn't make it a no-op nor it checks that the
> old and new policyes are the same. So it is possible to change the
> content of a policy, without changing its version. This is very
> confusing from userspace when managing policies.
> Instead change the update logic to reject updates that have the same
> version with ESTALE, as that is much clearer and intuitive behaviour.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>

Makes sense.

Reviewed-by: Serge Hallyn <serge@hallyn.com>

for both, thanks.

-serge

> ---
>  security/ipe/policy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index 5de64441dfe7..01da3a377e7f 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -115,7 +115,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
>  		goto err;
>  	}
>  
> -	if (ver_to_u64(old) > ver_to_u64(new)) {
> +	if (ver_to_u64(old) >= ver_to_u64(new)) {
>  		rc = -ESTALE;
>  		goto err;
>  	}
> -- 
> 2.39.5
> 

Re: [PATCH 2/2] ipe: also reject policy updates with the same version

[Fan Wu]

On 9/22/2024 6:56 AM, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
> Currently IPE accepts an update that has the same version as the policy
> being updated, but it doesn't make it a no-op nor it checks that the
> old and new policyes are the same. So it is possible to change the
> content of a policy, without changing its version. This is very
> confusing from userspace when managing policies.
> Instead change the update logic to reject updates that have the same
> version with ESTALE, as that is much clearer and intuitive behaviour.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>   security/ipe/policy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> index 5de64441dfe7..01da3a377e7f 100644
> --- a/security/ipe/policy.c
> +++ b/security/ipe/policy.c
> @@ -115,7 +115,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
>   		goto err;
>   	}
>   
> -	if (ver_to_u64(old) > ver_to_u64(new)) {
> +	if (ver_to_u64(old) >= ver_to_u64(new)) {
>   		rc = -ESTALE;
>   		goto err;
>   	}
Hi Luca,

Can you elaborate more about the potential confusion for the userspace 
users?

The policy version is currently used to prevent the activation of 
outdated or vulnerable policies (e.g., to avoid activating a policy 
trusting a compromised device). The version is not incremented unless a 
vulnerability is identified. Essentially, version comparison acts as a 
minimum threshold, ensuring only policies that meet or exceed this 
version can be activated.

Additionally, the version check is performed in ipe_set_active_pol(), so 
it will need to be updated accordingly. The documentation should also be 
refreshed to reflect these changes and ensure consistency with the new 
version handling process.

-Fan

Re: [PATCH 2/2] ipe: also reject policy updates with the same version

[Luca Boccassi]

On Mon, 23 Sept 2024 at 20:01, Fan Wu <wufan@linux.microsoft.com> wrote:
>
>
>
> On 9/22/2024 6:56 AM, luca.boccassi@gmail.com wrote:
> > From: Luca Boccassi <bluca@debian.org>
> >
> > Currently IPE accepts an update that has the same version as the policy
> > being updated, but it doesn't make it a no-op nor it checks that the
> > old and new policyes are the same. So it is possible to change the
> > content of a policy, without changing its version. This is very
> > confusing from userspace when managing policies.
> > Instead change the update logic to reject updates that have the same
> > version with ESTALE, as that is much clearer and intuitive behaviour.
> >
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> >   security/ipe/policy.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/ipe/policy.c b/security/ipe/policy.c
> > index 5de64441dfe7..01da3a377e7f 100644
> > --- a/security/ipe/policy.c
> > +++ b/security/ipe/policy.c
> > @@ -115,7 +115,7 @@ int ipe_update_policy(struct inode *root, const char *text, size_t textlen,
> >               goto err;
> >       }
> >
> > -     if (ver_to_u64(old) > ver_to_u64(new)) {
> > +     if (ver_to_u64(old) >= ver_to_u64(new)) {
> >               rc = -ESTALE;
> >               goto err;
> >       }
> Hi Luca,
>
> Can you elaborate more about the potential confusion for the userspace
> users?
>
> The policy version is currently used to prevent the activation of
> outdated or vulnerable policies (e.g., to avoid activating a policy
> trusting a compromised device). The version is not incremented unless a
> vulnerability is identified. Essentially, version comparison acts as a
> minimum threshold, ensuring only policies that meet or exceed this
> version can be activated.

"Version" suggests something that is bumped every time there is a
change, that's usually what the term is used for. The fact that one
can change the policy without changing the version confused me a lot.
Perhaps it should be renamed to "generation" or so, to make it more
clear that it is not intended to be changed every time, but just to
signal the start of a new generation to avoid downgrade attacks?

> Additionally, the version check is performed in ipe_set_active_pol(), so
> it will need to be updated accordingly. The documentation should also be
> refreshed to reflect these changes and ensure consistency with the new
> version handling process.
>
> -Fan

Re: [PATCH 2/2] ipe: also reject policy updates with the same version

[Fan Wu]

On 9/23/2024 2:48 PM, Luca Boccassi wrote:
> On Mon, 23 Sept 2024 at 20:01, Fan Wu <wufan@linux.microsoft.com> wrote:
>>
>>
>>
...
>> Hi Luca,
>>
>> Can you elaborate more about the potential confusion for the userspace
>> users?
>>
>> The policy version is currently used to prevent the activation of
>> outdated or vulnerable policies (e.g., to avoid activating a policy
>> trusting a compromised device). The version is not incremented unless a
>> vulnerability is identified. Essentially, version comparison acts as a
>> minimum threshold, ensuring only policies that meet or exceed this
>> version can be activated.
> 
> "Version" suggests something that is bumped every time there is a
> change, that's usually what the term is used for. The fact that one
> can change the policy without changing the version confused me a lot.
> Perhaps it should be renamed to "generation" or so, to make it more
> clear that it is not intended to be changed every time, but just to
> signal the start of a new generation to avoid downgrade attacks?
> 

I’m inclined to keep the 'version' name, but I agree with your point. 
Requiring a newer version for policy updates makes sense to me. As for 
the version check in ipe_set_active_pol(), we can maintain the current 
behavior, allowing the version to continue serving as a minimum 
threshold for activating a policy. In this case, I think the only change 
needed for this patch is to update the documentation for the `update` 
operation.

-Fan

Re: [PATCH 2/2] ipe: also reject policy updates with the same version

[Luca Boccassi]

On Tue, 24 Sept 2024 at 18:32, Fan Wu <wufan@linux.microsoft.com> wrote:
>
>
>
> On 9/23/2024 2:48 PM, Luca Boccassi wrote:
> > On Mon, 23 Sept 2024 at 20:01, Fan Wu <wufan@linux.microsoft.com> wrote:
> >>
> >>
> >>
> ...
> >> Hi Luca,
> >>
> >> Can you elaborate more about the potential confusion for the userspace
> >> users?
> >>
> >> The policy version is currently used to prevent the activation of
> >> outdated or vulnerable policies (e.g., to avoid activating a policy
> >> trusting a compromised device). The version is not incremented unless a
> >> vulnerability is identified. Essentially, version comparison acts as a
> >> minimum threshold, ensuring only policies that meet or exceed this
> >> version can be activated.
> >
> > "Version" suggests something that is bumped every time there is a
> > change, that's usually what the term is used for. The fact that one
> > can change the policy without changing the version confused me a lot.
> > Perhaps it should be renamed to "generation" or so, to make it more
> > clear that it is not intended to be changed every time, but just to
> > signal the start of a new generation to avoid downgrade attacks?
> >
>
> I’m inclined to keep the 'version' name, but I agree with your point.
> Requiring a newer version for policy updates makes sense to me. As for
> the version check in ipe_set_active_pol(), we can maintain the current
> behavior, allowing the version to continue serving as a minimum
> threshold for activating a policy. In this case, I think the only change
> needed for this patch is to update the documentation for the `update`
> operation.
>
> -Fan

Sure, just sent v2 with the doc update, thanks.