Re: [Xen-devel] [PATCH] flask: Add check for io{port, mem}con sorting
>>> "DeGraaf, Daniel G" 10/02/18 7:39 PM >>> >> From: Jan Beulich >> >>> On 28.09.18 at 21:13, wrote: >> > These entries are not always sorted by checkpolicy. Enforce the sorting >> > (which can be done manually if using an unpatched checkpolicy) when >> > loading the policy so that later uses by the security server do not >> > incorrectly use the initial sid. >> >> "Enforce the sorting" could mean two things - sorting what's unsorted, >> or (as you do) raise an error. Isn't raising an error here possibly going >> to impact systems which currently work? > >A system whose iomemcon entries are unsorted is currently not enforcing the >intended security policy. It normally ends up enforcing a more restrictive >policy, >but not always (it depends on what you allow access to the default label). My >guess is that anyone impacted by this problem would have noticed when they >added the rule and it had no effect. However, I do agree this could cause an >error on currently-working systems that do things like add iomemcon entries >that they don't use. > >Are you suggesting an update to the commit message to make this breakage >clear, or does the problem need to be fixed in the hypervisor? It would be >possible to sort the entries as they're added, but that's not as easy as just >detecting the mis-sort (since they're a linked list), and the policy creation >process should have already sorted them (except that that part was missing). I think resolving the ambiguity in the description is the minimal adjustment. If that's what you want to go with (you're the maintainer after all), I think it would suffice to suggest revised wording (or even merely your agreement for the committer to make a respective adjustment), without necessarily re-submitting the patch. Personally (but again, I'm not the maintainer of this code) I think it would be better if the actual issue was addressed by doing the sorting. It could be done with a warning logged, and perhaps with the warning suggesting that the built-in sorting will/might go away again in a later release. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] flask: Add check for io{port, mem}con sorting
> From: Jan Beulich > >>> On 28.09.18 at 21:13, wrote: > > These entries are not always sorted by checkpolicy. Enforce the sorting > > (which can be done manually if using an unpatched checkpolicy) when > > loading the policy so that later uses by the security server do not > > incorrectly use the initial sid. > > "Enforce the sorting" could mean two things - sorting what's unsorted, > or (as you do) raise an error. Isn't raising an error here possibly going > to impact systems which currently work? > > Jan A system whose iomemcon entries are unsorted is currently not enforcing the intended security policy. It normally ends up enforcing a more restrictive policy, but not always (it depends on what you allow access to the default label). My guess is that anyone impacted by this problem would have noticed when they added the rule and it had no effect. However, I do agree this could cause an error on currently-working systems that do things like add iomemcon entries that they don't use. Are you suggesting an update to the commit message to make this breakage clear, or does the problem need to be fixed in the hypervisor? It would be possible to sort the entries as they're added, but that's not as easy as just detecting the mis-sort (since they're a linked list), and the policy creation process should have already sorted them (except that that part was missing). ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] flask: Add check for io{port, mem}con sorting
> To: xen-devel@lists.xenproject.org
> From: Daniel De Graaf
> Sent by: "Xen-devel"
> Date: 09/28/2018 09:13PM
> Cc: George Dunlap , Daniel De Graaf
> Subject: [Xen-devel] [PATCH] flask: Add check for io{port,mem}con sorting
>
> These entries are not always sorted by checkpolicy. Enforce the sorting
> (which can be done manually if using an unpatched checkpolicy) when
> loading the policy so that later uses by the security server do not
> incorrectly use the initial sid.
>
> Reported-by: Nicolas Poirot
> Signed-off-by: Daniel De Graaf
> ---
> xen/xsm/flask/ss/policydb.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/xen/xsm/flask/ss/policydb.c b/xen/xsm/flask/ss/policydb.c
> index 3a12d96ef9..fcf63693b9 100644
> --- a/xen/xsm/flask/ss/policydb.c
> +++ b/xen/xsm/flask/ss/policydb.c
> @@ -2007,7 +2007,6 @@ int policydb_read(struct policydb *p, void *fp)
> l->next = c;
> else
> p->ocontexts[i] = c;
> - l = c;
> rc = -EINVAL;
> switch ( i )
> {
> @@ -2050,6 +2049,12 @@ int policydb_read(struct policydb *p, void *fp)
> rc = context_read_and_validate(>context, p, fp);
> if ( rc )
> goto bad;
> + if ( l && l->u.ioport.high_ioport > c->u.ioport.low_ioport )
> + {
> + printk(KERN_ERR
> + "Flask: Invalid policy, ioportcon not sorted\n");
> + goto bad;
> + }
> break;
> case OCON_IOMEM:
> if ( p->target_type != TARGET_XEN )
> @@ -2078,6 +2083,12 @@ int policydb_read(struct policydb *p, void *fp)
> rc = context_read_and_validate(>context, p, fp);
> if ( rc )
> goto bad;
> + if ( l && l->u.iomem.high_iomem > c->u.iomem.low_iomem )
> + {
> + printk(KERN_ERR
> + "Flask: Invalid policy, iomemcon not sorted\n");
> + goto bad;
> + }
> break;
> case OCON_DEVICE:
> if ( p->target_type != TARGET_XEN )
> @@ -2123,6 +2134,7 @@ int policydb_read(struct policydb *p, void *fp)
> rc = -EINVAL;
> goto bad;
> }
> + l = c;
> }
> }
>
> --
> 2.14.4
Looks good to me.
Tested on RELEASE-4.11.0 on a juno-r2 platform, with checkpolicy 2.5.
Thank you.
Tested-by: Nicolas Poirot
Reviewed-by: Nicolas Poirot
.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] flask: Add check for io{port, mem}con sorting
>>> On 28.09.18 at 21:13, wrote: > These entries are not always sorted by checkpolicy. Enforce the sorting > (which can be done manually if using an unpatched checkpolicy) when > loading the policy so that later uses by the security server do not > incorrectly use the initial sid. "Enforce the sorting" could mean two things - sorting what's unsorted, or (as you do) raise an error. Isn't raising an error here possibly going to impact systems which currently work? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] flask: Add check for io{port,mem}con sorting
These entries are not always sorted by checkpolicy. Enforce the sorting
(which can be done manually if using an unpatched checkpolicy) when
loading the policy so that later uses by the security server do not
incorrectly use the initial sid.
Reported-by: Nicolas Poirot
Signed-off-by: Daniel De Graaf
---
xen/xsm/flask/ss/policydb.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/xen/xsm/flask/ss/policydb.c b/xen/xsm/flask/ss/policydb.c
index 3a12d96ef9..fcf63693b9 100644
--- a/xen/xsm/flask/ss/policydb.c
+++ b/xen/xsm/flask/ss/policydb.c
@@ -2007,7 +2007,6 @@ int policydb_read(struct policydb *p, void *fp)
l->next = c;
else
p->ocontexts[i] = c;
-l = c;
rc = -EINVAL;
switch ( i )
{
@@ -2050,6 +2049,12 @@ int policydb_read(struct policydb *p, void *fp)
rc = context_read_and_validate(>context, p, fp);
if ( rc )
goto bad;
+if ( l && l->u.ioport.high_ioport > c->u.ioport.low_ioport )
+{
+printk(KERN_ERR
+"Flask: Invalid policy, ioportcon not sorted\n");
+goto bad;
+}
break;
case OCON_IOMEM:
if ( p->target_type != TARGET_XEN )
@@ -2078,6 +2083,12 @@ int policydb_read(struct policydb *p, void *fp)
rc = context_read_and_validate(>context, p, fp);
if ( rc )
goto bad;
+if ( l && l->u.iomem.high_iomem > c->u.iomem.low_iomem )
+{
+printk(KERN_ERR
+"Flask: Invalid policy, iomemcon not sorted\n");
+goto bad;
+}
break;
case OCON_DEVICE:
if ( p->target_type != TARGET_XEN )
@@ -2123,6 +2134,7 @@ int policydb_read(struct policydb *p, void *fp)
rc = -EINVAL;
goto bad;
}
+l = c;
}
}
--
2.14.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
