About XSM/flask

About XSM/flask

[Jean-Edouard LEJOSNE]

  Hi!

I've got some trouble with XSM/flask recently.
Basically, it blocks stuffs when not enforced, which is not (?) supposed to happen.

The problem is actually pretty simple when looking at the code.
As an example, here is a function from xen/xsm/flask/hooks.c :

static int flask_hvm_param(struct domain *d, unsigned long op)
{
      u32 perm;

      switch ( op )
      {
          case HVMOP_set_param:
              perm = HVM__SETPARAM;
          break;
          case HVMOP_get_param:
              perm = HVM__GETPARAM;
          break;
          default:
              return -EPERM;
      }

      return domain_has_perm(current->domain, d, SECCLASS_HVM, perm);
}

As it is pretty obvious, if "op" is not "HVMOP_set_param" or
"HVMOP_get_param", XSM will deny the action, even if we are in permissive mode.
It is currently a problem for us because, for this particular function
at least, we use other values of "op" (for dirty bit tracking).

I think in that case, flask should just print a warning and return 0
when in permissive mode.
The only other solution I see is to make sure every possible values are
treated by flask, and that it's maintained that way, which is probably a
pain.

So my question is : is there something that should be done about that? Is the current behaviour mandatory for some reason I didn't think about?

Thanks,

-- 
Jean-Edouard LEJOSNE
XenClient Lead Software Developpment Engineer
Citrix Systems
Cambridge, UK

Re: About XSM/flask

[George S. Coker, II]

This is a bug.  You should be able to run this in permissive mode without a
problem.  There have been some updates and extensions to this section since
this check was originally implemented.  The other ops may or may not be
important here.  Flask should be updated to track the other ops but it may
not be necessary to associate each op with a unique permission or any
security check at all.  Something like this,

>           case HVMOP_set_param:
            case HVMOP_set_XX:
            case HVMOP_set_YY:
>               perm = HVM__SETPARAM;
>           break;
>           case HVMOP_get_param:
            case HVMOP_get_XX:
            case HVMOP_get_YY:
>               perm = HVM__GETPARAM;
>           break;
            case HVMOP_ZZ:
                /* security relevant but not a get or set capability */
                perm = HVM__ZZ;
            break;
            case HVMOP_ETC:
                /* ops that have no security impact */
                return 0;
            break;
>           default:
>               return -EPERM;

would address the issue, allow future checks and maintain the original
intent of protecting against a potentially illegal op being passed through.
If all the ops can reasonably be paired as get/set, one could just implement
the above as a patch but recognize the loss of granularity as all get(s) or
set(s) will be treated as equivalents in the policy.  Deeper thought is
required to extend the granularity on a per op basis.

George



On 9/22/10 11:46 AM, "Jean-Edouard LEJOSNE"
<jean-edouard.lejosne@citrix.com> wrote:

>   Hi!
> 
> I've got some trouble with XSM/flask recently.
> Basically, it blocks stuffs when not enforced, which is not (?) supposed to
> happen.
> 
> The problem is actually pretty simple when looking at the code.
> As an example, here is a function from xen/xsm/flask/hooks.c :
> 
> static int flask_hvm_param(struct domain *d, unsigned long op)
> {
>       u32 perm;
> 
>       switch ( op )
>       {
>           case HVMOP_set_param:
>               perm = HVM__SETPARAM;
>           break;
>           case HVMOP_get_param:
>               perm = HVM__GETPARAM;
>           break;
>           default:
>               return -EPERM;
>       }
> 
>       return domain_has_perm(current->domain, d, SECCLASS_HVM, perm);
> }
> 
> As it is pretty obvious, if "op" is not "HVMOP_set_param" or
> "HVMOP_get_param", XSM will deny the action, even if we are in permissive
> mode.
> It is currently a problem for us because, for this particular function
> at least, we use other values of "op" (for dirty bit tracking).
> 
> I think in that case, flask should just print a warning and return 0
> when in permissive mode.
> The only other solution I see is to make sure every possible values are
> treated by flask, and that it's maintained that way, which is probably a
> pain.
> 
> So my question is : is there something that should be done about that? Is the
> current behaviour mandatory for some reason I didn't think about?
> 
> Thanks,

-- 
George S. Coker, II <gscoker@alpha.ncsc.mil>

Re: About XSM/flask

[Jean Guyader]

I have a question on this piece of code too.

Why do we return EPERM in this switch case statement and not ENOENT?
(It actually concerns anyway other switch case statement here the operation
isn't understood).

The idea would be to have something on the top of the ops that will
decide to turn
a ENOENT into a EPERM in non permissive mode and a ENOENT into a 0 in
permissive mode. This wrapper function could advertise as a warning all the
unknown operations and it will make updating the xsm code easier in case
we got out of sync.

But it's probably looking for details at this point, and it means
going through all
the code to change EPERM into ENOENT.

Jean

On 22 September 2010 17:33, George S. Coker, II <gscoker@alpha.ncsc.mil> wrote:
> This is a bug.  You should be able to run this in permissive mode without a
> problem.  There have been some updates and extensions to this section since
> this check was originally implemented.  The other ops may or may not be
> important here.  Flask should be updated to track the other ops but it may
> not be necessary to associate each op with a unique permission or any
> security check at all.  Something like this,
>
>>           case HVMOP_set_param:
>            case HVMOP_set_XX:
>            case HVMOP_set_YY:
>>               perm = HVM__SETPARAM;
>>           break;
>>           case HVMOP_get_param:
>            case HVMOP_get_XX:
>            case HVMOP_get_YY:
>>               perm = HVM__GETPARAM;
>>           break;
>            case HVMOP_ZZ:
>                /* security relevant but not a get or set capability */
>                perm = HVM__ZZ;
>            break;
>            case HVMOP_ETC:
>                /* ops that have no security impact */
>                return 0;
>            break;
>>           default:
>>               return -EPERM;
>
> would address the issue, allow future checks and maintain the original
> intent of protecting against a potentially illegal op being passed through.
> If all the ops can reasonably be paired as get/set, one could just implement
> the above as a patch but recognize the loss of granularity as all get(s) or
> set(s) will be treated as equivalents in the policy.  Deeper thought is
> required to extend the granularity on a per op basis.
>
> George
>
>
>
> On 9/22/10 11:46 AM, "Jean-Edouard LEJOSNE"
> <jean-edouard.lejosne@citrix.com> wrote:
>
>>   Hi!
>>
>> I've got some trouble with XSM/flask recently.
>> Basically, it blocks stuffs when not enforced, which is not (?) supposed to
>> happen.
>>
>> The problem is actually pretty simple when looking at the code.
>> As an example, here is a function from xen/xsm/flask/hooks.c :
>>
>> static int flask_hvm_param(struct domain *d, unsigned long op)
>> {
>>       u32 perm;
>>
>>       switch ( op )
>>       {
>>           case HVMOP_set_param:
>>               perm = HVM__SETPARAM;
>>           break;
>>           case HVMOP_get_param:
>>               perm = HVM__GETPARAM;
>>           break;
>>           default:
>>               return -EPERM;
>>       }
>>
>>       return domain_has_perm(current->domain, d, SECCLASS_HVM, perm);
>> }
>>
>> As it is pretty obvious, if "op" is not "HVMOP_set_param" or
>> "HVMOP_get_param", XSM will deny the action, even if we are in permissive
>> mode.
>> It is currently a problem for us because, for this particular function
>> at least, we use other values of "op" (for dirty bit tracking).
>>
>> I think in that case, flask should just print a warning and return 0
>> when in permissive mode.
>> The only other solution I see is to make sure every possible values are
>> treated by flask, and that it's maintained that way, which is probably a
>> pain.
>>
>> So my question is : is there something that should be done about that? Is the
>> current behaviour mandatory for some reason I didn't think about?
>>
>> Thanks,
>
> --
> George S. Coker, II <gscoker@alpha.ncsc.mil>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>