[PATCH] flask: Add check for io{port,mem}con sorting

[PATCH] flask: Add check for io{port,mem}con sorting

[Daniel De Graaf]

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 <nicolas.poirot@bertin.fr>
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 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(&c->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(&c->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

Re: [PATCH] flask: Add check for io{port, mem}con sorting

[Jan Beulich]

>>> On 28.09.18 at 21:13, <dgdegra@tycho.nsa.gov> 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

Re: [PATCH] flask: Add check for io{port, mem}con sorting

[nicolas.poirot]

> To: xen-devel@lists.xenproject.org
> From: Daniel De Graaf 
> Sent by: "Xen-devel" 
> Date: 09/28/2018 09:13PM
> Cc: George Dunlap <dunlapg@umich.edu>, Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 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 <nicolas.poirot@bertin.fr>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  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(&c->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(&c->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 <nicolas.poirot@bertin.fr>
Reviewed-by: Nicolas Poirot <nicolas.poirot@bertin.fr>
.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] flask: Add check for io{port, mem}con sorting

[Jan Beulich]

>>> "DeGraaf, Daniel G" <dgdegra@nsa.gov> 10/02/18 7:39 PM >>>
>> From: Jan Beulich <JBeulich@suse.com>
>> >>> On 28.09.18 at 21:13, <dgdegra@tycho.nsa.gov> 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: [PATCH] flask: Add check for io{port, mem}con sorting

[DeGraaf, Daniel G]

> From: Jan Beulich <JBeulich@suse.com>
> >>> On 28.09.18 at 21:13, <dgdegra@tycho.nsa.gov> 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