avc_has_perm() returns -1 even when SELinux is in permissive mode

avc_has_perm() returns -1 even when SELinux is in permissive mode

[Laurent Bigonville]

Hello,

After some debugging on Debian to figure out why D-Bus why denying
messages between my user session and policykit with SELinux in
permissive mode, eparis pointed me that Fedora has a patch for this in
the avc_has_perm() function.

The patch[0] itself seems pretty trivial and I was wondering if it (or
something similar) could be merged in the upstream codebase.

But, if I'm not wrong, this patch makes avc_has_perm() and
avc_has_perm_noaudit() have different behavior when the machine is
running in permissive mode, shouldn't this be tested in the
avc_has_perm_noaudit() function instead?

my 2¢,

Laurent Bigonville

[0]
http://pkgs.fedoraproject.org/cgit/libselinux.git/tree/libselinux-rhat.patch#n704


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

On 10/27/2013 09:43 AM, Laurent Bigonville wrote:
> Hello,
> 
> After some debugging on Debian to figure out why D-Bus why denying
> messages between my user session and policykit with SELinux in
> permissive mode, eparis pointed me that Fedora has a patch for this in
> the avc_has_perm() function.
> 
> The patch[0] itself seems pretty trivial and I was wondering if it (or
> something similar) could be merged in the upstream codebase.
> 
> But, if I'm not wrong, this patch makes avc_has_perm() and
> avc_has_perm_noaudit() have different behavior when the machine is
> running in permissive mode, shouldn't this be tested in the
> avc_has_perm_noaudit() function instead?

I'm pretty sure I NAKed that previously.  Permissive mode isn't supposed
to hide other kinds of errors/bugs other than policy denials, so making
it hide arbitrary error conditions (which could include completely bogus
security contexts, security classes, memory allocation failure, etc) is
definitely not a good thing.  Particularly if there is absolutely no
logging of what the issue was so that you have a hope of noting it
before switching to enforcing.



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/27/2013 09:43 AM, Laurent Bigonville wrote:
> Hello,
> 
> After some debugging on Debian to figure out why D-Bus why denying messages
> between my user session and policykit with SELinux in permissive mode,
> eparis pointed me that Fedora has a patch for this in the avc_has_perm()
> function.
> 
> The patch[0] itself seems pretty trivial and I was wondering if it (or 
> something similar) could be merged in the upstream codebase.
> 
> But, if I'm not wrong, this patch makes avc_has_perm() and 
> avc_has_perm_noaudit() have different behavior when the machine is running
> in permissive mode, shouldn't this be tested in the avc_has_perm_noaudit()
> function instead?
> 
> my 2¢,
> 
> Laurent Bigonville
> 
> [0] 
> http://pkgs.fedoraproject.org/cgit/libselinux.git/tree/libselinux-rhat.patch#n704
>
> 
> 
> -- This message was distributed to subscribers of the selinux mailing
> list. If you no longer wish to subscribe, send mail to
> majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes
> as the message.
> 
I believe this patch was rejected upstream.  Basically upstream wanted the
calling apps to check the permissive flags themselves.  DBUS argued against
it, so we carry a patch for it.

The reason this is not in avc_has_perm_noaudit is that we want the avc to be
still audited.  I agree that it should be moved up to avc_has_perm_noaudit.

avc_has_perm_noaudit currently checks the permissive flag on only one code
path, but not on failures.

The argument is whether or not avc_has_perm* should ever block anything in
permissive mode.  We believe it should not.

I will move the override check to avc_has_perm_noaudit.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlJuXt0ACgkQrlYvE4MpobNkPwCgmAqYTTwRqfW2HxzyVz2AKrPc
9MgAoLEkCxZ2iNHsWRs+BEJlTwRmV14Y
=TiuS
-----END PGP SIGNATURE-----


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Laurent Bigonville]

Le Mon, 28 Oct 2013 08:49:32 -0400,
Stephen Smalley <sds@tycho.nsa.gov> a écrit :

> On 10/27/2013 09:43 AM, Laurent Bigonville wrote:
> > Hello,
> > 
> > After some debugging on Debian to figure out why D-Bus why denying
> > messages between my user session and policykit with SELinux in
> > permissive mode, eparis pointed me that Fedora has a patch for this
> > in the avc_has_perm() function.
> > 
> > The patch[0] itself seems pretty trivial and I was wondering if it
> > (or something similar) could be merged in the upstream codebase.
> > 
> > But, if I'm not wrong, this patch makes avc_has_perm() and
> > avc_has_perm_noaudit() have different behavior when the machine is
> > running in permissive mode, shouldn't this be tested in the
> > avc_has_perm_noaudit() function instead?
> 
> I'm pretty sure I NAKed that previously.  Permissive mode isn't
> supposed to hide other kinds of errors/bugs other than policy
> denials, so making it hide arbitrary error conditions (which could
> include completely bogus security contexts, security classes, memory
> allocation failure, etc) is definitely not a good thing.
> Particularly if there is absolutely no logging of what the issue was
> so that you have a hope of noting it before switching to enforcing.

Thanks for the answers, I've reopened a bug[0] against d-bus, also I
have the feeling that other applications are expecting the Fedora
behavior (systemd?). If somebody want to add a comment, to the bug that
would be great.

I'm not too sure what to do here, personally don't want to diverge from
upstream too much, especially with such behavior changes, Sven are you
adding a patch for this in Gentoo?

Cheers,

Laurent Bigonville

[0] https://bugs.freedesktop.org/show_bug.cgi?id=70894

PS: The avc_has_perm(3) manpage is wrong as it states that it will
returns 0 in permissive mode, I guess this should be removed.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Maybe the solution here is to add logging messages to the function.

My opionion is that if something is wrong with SELinux, IE The labels are
wrong, the policy is wrong or the app is wrong, we should not block in
permissive mode.

Having the tool write "foobar_t is not a valid source context" would be better
then what we have now, which is a silent denial even in permissive mode.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlJueKwACgkQrlYvE4MpobNU6wCeKWF79vOSxUH0PKDiuo64DbY/
EwgAoKjSD2BMe6KHIv2Smvkxwh/Ta02r
=6Agn
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Eric Paris]

On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
> Maybe the solution here is to add logging messages to the function.
> 
> My opionion is that if something is wrong with SELinux, IE The labels are
> wrong, the policy is wrong or the app is wrong, we should not block in
> permissive mode.
> 
> Having the tool write "foobar_t is not a valid source context" would be better
> then what we have now, which is a silent denial even in permissive mode.

I understand Stephen's argument.  But agree with dwalsh/bigon that
hiding this in the library is a lot better than moving the logic to
userspace programs.  So this might not be so super simple to do.  How
about the idea of a new interface which always returns 0 in permissive?
But it does a couple of extra things.  These are just rough early
thoughts....

0) new interface just like avc_has_perm() but which always returns 0 in
permissive.

1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
message into the audit log if not.

2) a new /sys/fs/selinux/context like mechanism, which will both
validate the context and will force it into the sid cache.  So
subsequent broken calls to avc_has_perm() will not generate a second
SELINX_USER_ERR message, since the second call to 'access' will find a
valid type and will give a denial for that unlabeled_t type?

maybe /sys/fs/selinux/access should be changed/new interface added to do
all of this in kernel?  generating a real SELINUX_ERR in kernel and
forcing the invalid label into the sid cache?

I really do think that userspace object managers should be allowed to
call avc_has_perm() and either get an error that should be handled as a
hard failure or a 0...   checking permissive in userspace object
managers just seems prone to breakage...


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

On 10/28/2013 11:56 AM, Eric Paris wrote:
> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>> Maybe the solution here is to add logging messages to the function.
>>
>> My opionion is that if something is wrong with SELinux, IE The labels are
>> wrong, the policy is wrong or the app is wrong, we should not block in
>> permissive mode.
>>
>> Having the tool write "foobar_t is not a valid source context" would be better
>> then what we have now, which is a silent denial even in permissive mode.
> 
> I understand Stephen's argument.  But agree with dwalsh/bigon that
> hiding this in the library is a lot better than moving the logic to
> userspace programs.  So this might not be so super simple to do.  How
> about the idea of a new interface which always returns 0 in permissive?
> But it does a couple of extra things.  These are just rough early
> thoughts....
> 
> 0) new interface just like avc_has_perm() but which always returns 0 in
> permissive.
> 
> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
> message into the audit log if not.
> 
> 2) a new /sys/fs/selinux/context like mechanism, which will both
> validate the context and will force it into the sid cache.  So
> subsequent broken calls to avc_has_perm() will not generate a second
> SELINX_USER_ERR message, since the second call to 'access' will find a
> valid type and will give a denial for that unlabeled_t type?
> 
> maybe /sys/fs/selinux/access should be changed/new interface added to do
> all of this in kernel?  generating a real SELINUX_ERR in kernel and
> forcing the invalid label into the sid cache?
> 
> I really do think that userspace object managers should be allowed to
> call avc_has_perm() and either get an error that should be handled as a
> hard failure or a 0...   checking permissive in userspace object
> managers just seems prone to breakage...

I'm ok with changing avc_has_perm as long as:
a) Something gets logged/audited so you'll see that something went wrong
in permissive mode and not just get silent failures in enforcing mode,

b) We are careful about what error conditions are remapped to 0 in
permissive mode.  If we just hit a memory allocation failure, we
shouldn't hide that from the caller.  It should only affect things
relating to policy.



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Eric Paris]

On Mon, 2013-10-28 at 12:58 -0400, Stephen Smalley wrote:
> On 10/28/2013 11:56 AM, Eric Paris wrote:
> > On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
> >> Maybe the solution here is to add logging messages to the function.
> >>
> >> My opionion is that if something is wrong with SELinux, IE The labels are
> >> wrong, the policy is wrong or the app is wrong, we should not block in
> >> permissive mode.
> >>
> >> Having the tool write "foobar_t is not a valid source context" would be better
> >> then what we have now, which is a silent denial even in permissive mode.
> > 
> > I understand Stephen's argument.  But agree with dwalsh/bigon that
> > hiding this in the library is a lot better than moving the logic to
> > userspace programs.  So this might not be so super simple to do.  How
> > about the idea of a new interface which always returns 0 in permissive?
> > But it does a couple of extra things.  These are just rough early
> > thoughts....
> > 
> > 0) new interface just like avc_has_perm() but which always returns 0 in
> > permissive.
> > 
> > 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
> > scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
> > message into the audit log if not.
> > 
> > 2) a new /sys/fs/selinux/context like mechanism, which will both
> > validate the context and will force it into the sid cache.  So
> > subsequent broken calls to avc_has_perm() will not generate a second
> > SELINX_USER_ERR message, since the second call to 'access' will find a
> > valid type and will give a denial for that unlabeled_t type?
> > 
> > maybe /sys/fs/selinux/access should be changed/new interface added to do
> > all of this in kernel?  generating a real SELINUX_ERR in kernel and
> > forcing the invalid label into the sid cache?
> > 
> > I really do think that userspace object managers should be allowed to
> > call avc_has_perm() and either get an error that should be handled as a
> > hard failure or a 0...   checking permissive in userspace object
> > managers just seems prone to breakage...
> 
> I'm ok with changing avc_has_perm as long as:
> a) Something gets logged/audited so you'll see that something went wrong
> in permissive mode and not just get silent failures in enforcing mode,
> 
> b) We are careful about what error conditions are remapped to 0 in
> permissive mode.  If we just hit a memory allocation failure, we
> shouldn't hide that from the caller.  It should only affect things
> relating to policy.

I'm currently thinking about something like a change
in /sys/fs/selinux/access which forcibly maps invalid contexts to
SECINITSID_NULL (in both the enforcing and permissive case) and which
sends a new audit message SELINUX_USER_ERR() when it does that invalid
mapping...

It should mean that we get ONE audit messages in permissive and
enforcing per invalid label.  Kernel policy will make the decision
against the null sid.  Userspace (avc_has_perm_noaudit) will add in the
right flags if the system is in permissive, so those errors will never
percolate back up the stack...

Is this a bad idea Stephen?


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

On 10/28/2013 01:11 PM, Eric Paris wrote:
> On Mon, 2013-10-28 at 12:58 -0400, Stephen Smalley wrote:
>> On 10/28/2013 11:56 AM, Eric Paris wrote:
>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>>>> Maybe the solution here is to add logging messages to the function.
>>>>
>>>> My opionion is that if something is wrong with SELinux, IE The labels are
>>>> wrong, the policy is wrong or the app is wrong, we should not block in
>>>> permissive mode.
>>>>
>>>> Having the tool write "foobar_t is not a valid source context" would be better
>>>> then what we have now, which is a silent denial even in permissive mode.
>>>
>>> I understand Stephen's argument.  But agree with dwalsh/bigon that
>>> hiding this in the library is a lot better than moving the logic to
>>> userspace programs.  So this might not be so super simple to do.  How
>>> about the idea of a new interface which always returns 0 in permissive?
>>> But it does a couple of extra things.  These are just rough early
>>> thoughts....
>>>
>>> 0) new interface just like avc_has_perm() but which always returns 0 in
>>> permissive.
>>>
>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
>>> message into the audit log if not.
>>>
>>> 2) a new /sys/fs/selinux/context like mechanism, which will both
>>> validate the context and will force it into the sid cache.  So
>>> subsequent broken calls to avc_has_perm() will not generate a second
>>> SELINX_USER_ERR message, since the second call to 'access' will find a
>>> valid type and will give a denial for that unlabeled_t type?
>>>
>>> maybe /sys/fs/selinux/access should be changed/new interface added to do
>>> all of this in kernel?  generating a real SELINUX_ERR in kernel and
>>> forcing the invalid label into the sid cache?
>>>
>>> I really do think that userspace object managers should be allowed to
>>> call avc_has_perm() and either get an error that should be handled as a
>>> hard failure or a 0...   checking permissive in userspace object
>>> managers just seems prone to breakage...
>>
>> I'm ok with changing avc_has_perm as long as:
>> a) Something gets logged/audited so you'll see that something went wrong
>> in permissive mode and not just get silent failures in enforcing mode,
>>
>> b) We are careful about what error conditions are remapped to 0 in
>> permissive mode.  If we just hit a memory allocation failure, we
>> shouldn't hide that from the caller.  It should only affect things
>> relating to policy.
> 
> I'm currently thinking about something like a change
> in /sys/fs/selinux/access which forcibly maps invalid contexts to
> SECINITSID_NULL (in both the enforcing and permissive case) and which
> sends a new audit message SELINUX_USER_ERR() when it does that invalid
> mapping...
> 
> It should mean that we get ONE audit messages in permissive and
> enforcing per invalid label.  Kernel policy will make the decision
> against the null sid.  Userspace (avc_has_perm_noaudit) will add in the
> right flags if the system is in permissive, so those errors will never
> percolate back up the stack...
> 
> Is this a bad idea Stephen?

Kernel remaps invalid contexts internally to the unlabeled SID.  I don't
think you want the NULL SID.

Userspace AVC could detect and handle an EINVAL from
security_compute_av_flags_raw() by rechecking context validity, grabbing
the unlabeled context via avc_get_initial_sid(), and replace it and
retry.  Benefit is you don't have to wait for a new kernel to show up.

We don't automatically remap invalid contexts coming into the selinuxfs
interface (or /proc/pid/attr interface) to the unlabeled context
intentionally, as there is too much risk there of hiding bugs in
userspace and ending up labeling things with the unlabeled context.




--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Paul Moore]

On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
> On 10/28/2013 11:56 AM, Eric Paris wrote:
> > On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
> >> Maybe the solution here is to add logging messages to the function.
> >> 
> >> My opionion is that if something is wrong with SELinux, IE The labels are
> >> wrong, the policy is wrong or the app is wrong, we should not block in
> >> permissive mode.
> >> 
> >> Having the tool write "foobar_t is not a valid source context" would be
> >> better then what we have now, which is a silent denial even in
> >> permissive mode.> 
> > I understand Stephen's argument.  But agree with dwalsh/bigon that
> > hiding this in the library is a lot better than moving the logic to
> > userspace programs.  So this might not be so super simple to do.  How
> > about the idea of a new interface which always returns 0 in permissive?
> > But it does a couple of extra things.  These are just rough early
> > thoughts....
> > 
> > 0) new interface just like avc_has_perm() but which always returns 0 in
> > permissive.
> > 
> > 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
> > scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
> > message into the audit log if not.
> > 
> > 2) a new /sys/fs/selinux/context like mechanism, which will both
> > validate the context and will force it into the sid cache.  So
> > subsequent broken calls to avc_has_perm() will not generate a second
> > SELINX_USER_ERR message, since the second call to 'access' will find a
> > valid type and will give a denial for that unlabeled_t type?
> > 
> > maybe /sys/fs/selinux/access should be changed/new interface added to do
> > all of this in kernel?  generating a real SELINUX_ERR in kernel and
> > forcing the invalid label into the sid cache?
> > 
> > I really do think that userspace object managers should be allowed to
> > call avc_has_perm() and either get an error that should be handled as a
> > hard failure or a 0...   checking permissive in userspace object
> > managers just seems prone to breakage...
> 
> I'm ok with changing avc_has_perm as long as:
> a) Something gets logged/audited so you'll see that something went wrong
> in permissive mode and not just get silent failures in enforcing mode,
> 
> b) We are careful about what error conditions are remapped to 0 in
> permissive mode.  If we just hit a memory allocation failure, we
> shouldn't hide that from the caller.  It should only affect things
> relating to policy.

I'm far from an expert on the SELinux userland libraries, but as far as my two 
cents are concerned I like the idea of changing avc_has_perm() to incorporate 
the permissive/enforcing logic.  I think asking applications to worry about 
things like that is a step in the wrong direction. 

I also agree with Stephen's comments above: logging is important and the only 
failures that should be ignored by avc_has_perm() are those relating to access 
denials from policy, error conditions should propagate to the caller.

-- 
paul moore
www.paul-moore.com


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Paul Moore]

On Monday, October 28, 2013 01:21:07 PM Stephen Smalley wrote:
> On 10/28/2013 01:11 PM, Eric Paris wrote:
> > On Mon, 2013-10-28 at 12:58 -0400, Stephen Smalley wrote:
> >> On 10/28/2013 11:56 AM, Eric Paris wrote:
> >>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
> >>>> Maybe the solution here is to add logging messages to the function.
> >>>> 
> >>>> My opionion is that if something is wrong with SELinux, IE The labels
> >>>> are
> >>>> wrong, the policy is wrong or the app is wrong, we should not block in
> >>>> permissive mode.
> >>>> 
> >>>> Having the tool write "foobar_t is not a valid source context" would be
> >>>> better then what we have now, which is a silent denial even in
> >>>> permissive mode.>>> 
> >>> I understand Stephen's argument.  But agree with dwalsh/bigon that
> >>> hiding this in the library is a lot better than moving the logic to
> >>> userspace programs.  So this might not be so super simple to do.  How
> >>> about the idea of a new interface which always returns 0 in permissive?
> >>> But it does a couple of extra things.  These are just rough early
> >>> thoughts....
> >>> 
> >>> 0) new interface just like avc_has_perm() but which always returns 0 in
> >>> permissive.
> >>> 
> >>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
> >>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
> >>> message into the audit log if not.
> >>> 
> >>> 2) a new /sys/fs/selinux/context like mechanism, which will both
> >>> validate the context and will force it into the sid cache.  So
> >>> subsequent broken calls to avc_has_perm() will not generate a second
> >>> SELINX_USER_ERR message, since the second call to 'access' will find a
> >>> valid type and will give a denial for that unlabeled_t type?
> >>> 
> >>> maybe /sys/fs/selinux/access should be changed/new interface added to do
> >>> all of this in kernel?  generating a real SELINUX_ERR in kernel and
> >>> forcing the invalid label into the sid cache?
> >>> 
> >>> I really do think that userspace object managers should be allowed to
> >>> call avc_has_perm() and either get an error that should be handled as a
> >>> hard failure or a 0...   checking permissive in userspace object
> >>> managers just seems prone to breakage...
> >> 
> >> I'm ok with changing avc_has_perm as long as:
> >> a) Something gets logged/audited so you'll see that something went wrong
> >> in permissive mode and not just get silent failures in enforcing mode,
> >> 
> >> b) We are careful about what error conditions are remapped to 0 in
> >> permissive mode.  If we just hit a memory allocation failure, we
> >> shouldn't hide that from the caller.  It should only affect things
> >> relating to policy.
> > 
> > I'm currently thinking about something like a change
> > in /sys/fs/selinux/access which forcibly maps invalid contexts to
> > SECINITSID_NULL (in both the enforcing and permissive case) and which
> > sends a new audit message SELINUX_USER_ERR() when it does that invalid
> > mapping...
> > 
> > It should mean that we get ONE audit messages in permissive and
> > enforcing per invalid label.  Kernel policy will make the decision
> > against the null sid.  Userspace (avc_has_perm_noaudit) will add in the
> > right flags if the system is in permissive, so those errors will never
> > percolate back up the stack...
> > 
> > Is this a bad idea Stephen?
> 
> Kernel remaps invalid contexts internally to the unlabeled SID.  I don't
> think you want the NULL SID.

I don't either, unlabeled seems the better option.  I think you might see some 
weird behavior if you mapped invalid labels to SECINITSID_NULL.

> Userspace AVC could detect and handle an EINVAL from
> security_compute_av_flags_raw() by rechecking context validity, grabbing
> the unlabeled context via avc_get_initial_sid(), and replace it and
> retry.  Benefit is you don't have to wait for a new kernel to show up.
> 
> We don't automatically remap invalid contexts coming into the selinuxfs
> interface (or /proc/pid/attr interface) to the unlabeled context
> intentionally, as there is too much risk there of hiding bugs in
> userspace and ending up labeling things with the unlabeled context.

Perhaps I missed something, but what if an invalid label is used, why not just 
return -EINVAL in the case of enforcing and 0 in the case of permissive?  I 
would expect an audit/error message in the system logs in both cases.

-- 
paul moore
www.paul-moore.com


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/28/2013 02:10 PM, Paul Moore wrote:
> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
>> On 10/28/2013 11:56 AM, Eric Paris wrote:
>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>>>> Maybe the solution here is to add logging messages to the function.
>>>> 
>>>> My opionion is that if something is wrong with SELinux, IE The labels
>>>> are wrong, the policy is wrong or the app is wrong, we should not
>>>> block in permissive mode.
>>>> 
>>>> Having the tool write "foobar_t is not a valid source context" would
>>>> be better then what we have now, which is a silent denial even in 
>>>> permissive mode.>
>>> I understand Stephen's argument.  But agree with dwalsh/bigon that 
>>> hiding this in the library is a lot better than moving the logic to 
>>> userspace programs.  So this might not be so super simple to do.  How 
>>> about the idea of a new interface which always returns 0 in
>>> permissive? But it does a couple of extra things.  These are just rough
>>> early thoughts....
>>> 
>>> 0) new interface just like avc_has_perm() but which always returns 0
>>> in permissive.
>>> 
>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the 
>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR 
>>> message into the audit log if not.
>>> 
>>> 2) a new /sys/fs/selinux/context like mechanism, which will both 
>>> validate the context and will force it into the sid cache.  So 
>>> subsequent broken calls to avc_has_perm() will not generate a second 
>>> SELINX_USER_ERR message, since the second call to 'access' will find a 
>>> valid type and will give a denial for that unlabeled_t type?
>>> 
>>> maybe /sys/fs/selinux/access should be changed/new interface added to
>>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and 
>>> forcing the invalid label into the sid cache?
>>> 
>>> I really do think that userspace object managers should be allowed to 
>>> call avc_has_perm() and either get an error that should be handled as
>>> a hard failure or a 0...   checking permissive in userspace object 
>>> managers just seems prone to breakage...
>> 
>> I'm ok with changing avc_has_perm as long as: a) Something gets
>> logged/audited so you'll see that something went wrong in permissive mode
>> and not just get silent failures in enforcing mode,
>> 
>> b) We are careful about what error conditions are remapped to 0 in 
>> permissive mode.  If we just hit a memory allocation failure, we 
>> shouldn't hide that from the caller.  It should only affect things 
>> relating to policy.
> 
> I'm far from an expert on the SELinux userland libraries, but as far as my
> two cents are concerned I like the idea of changing avc_has_perm() to
> incorporate the permissive/enforcing logic.  I think asking applications to
> worry about things like that is a step in the wrong direction.
> 
> I also agree with Stephen's comments above: logging is important and the
> only failures that should be ignored by avc_has_perm() are those relating
> to access denials from policy, error conditions should propagate to the
> caller.
> 
The only functions that can fail are the following:

		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
		if (rc) {
			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
							   tclass, requested,
							   &entry.avd);
			if (rc)
				goto out;
			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);


Are we stating that we should check for EINVAL and return 0 if permissive, or
is there some more complicated thing we should look up.

I forget the original bugzilla that caused us to do this check but it was
something to do with dbus running witht he wrong context, I believe.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlJuq+MACgkQrlYvE4MpobPE5gCeOyKKG3Q14e/mvaEOeCHthTaY
Z6cAoNkmhAKZKVBGnP/KBKMUy875fQ2M
=84wx
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

On 10/28/2013 02:24 PM, Daniel J Walsh wrote:
> On 10/28/2013 02:10 PM, Paul Moore wrote:
>> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
>>> On 10/28/2013 11:56 AM, Eric Paris wrote:
>>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>>>>> Maybe the solution here is to add logging messages to the function.
>>>>>
>>>>> My opionion is that if something is wrong with SELinux, IE The labels
>>>>> are wrong, the policy is wrong or the app is wrong, we should not
>>>>> block in permissive mode.
>>>>>
>>>>> Having the tool write "foobar_t is not a valid source context" would
>>>>> be better then what we have now, which is a silent denial even in 
>>>>> permissive mode.>
>>>> I understand Stephen's argument.  But agree with dwalsh/bigon that 
>>>> hiding this in the library is a lot better than moving the logic to 
>>>> userspace programs.  So this might not be so super simple to do.  How 
>>>> about the idea of a new interface which always returns 0 in
>>>> permissive? But it does a couple of extra things.  These are just rough
>>>> early thoughts....
>>>>
>>>> 0) new interface just like avc_has_perm() but which always returns 0
>>>> in permissive.
>>>>
>>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the 
>>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR 
>>>> message into the audit log if not.
>>>>
>>>> 2) a new /sys/fs/selinux/context like mechanism, which will both 
>>>> validate the context and will force it into the sid cache.  So 
>>>> subsequent broken calls to avc_has_perm() will not generate a second 
>>>> SELINX_USER_ERR message, since the second call to 'access' will find a 
>>>> valid type and will give a denial for that unlabeled_t type?
>>>>
>>>> maybe /sys/fs/selinux/access should be changed/new interface added to
>>>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and 
>>>> forcing the invalid label into the sid cache?
>>>>
>>>> I really do think that userspace object managers should be allowed to 
>>>> call avc_has_perm() and either get an error that should be handled as
>>>> a hard failure or a 0...   checking permissive in userspace object 
>>>> managers just seems prone to breakage...
>>>
>>> I'm ok with changing avc_has_perm as long as: a) Something gets
>>> logged/audited so you'll see that something went wrong in permissive mode
>>> and not just get silent failures in enforcing mode,
>>>
>>> b) We are careful about what error conditions are remapped to 0 in 
>>> permissive mode.  If we just hit a memory allocation failure, we 
>>> shouldn't hide that from the caller.  It should only affect things 
>>> relating to policy.
> 
>> I'm far from an expert on the SELinux userland libraries, but as far as my
>> two cents are concerned I like the idea of changing avc_has_perm() to
>> incorporate the permissive/enforcing logic.  I think asking applications to
>> worry about things like that is a step in the wrong direction.
> 
>> I also agree with Stephen's comments above: logging is important and the
>> only failures that should be ignored by avc_has_perm() are those relating
>> to access denials from policy, error conditions should propagate to the
>> caller.
> 
> The only functions that can fail are the following:
> 
> 		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
> 		if (rc) {
> 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
> 							   tclass, requested,
> 							   &entry.avd);
> 			if (rc)
> 				goto out;
> 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
> 
> 
> Are we stating that we should check for EINVAL and return 0 if permissive, or
> is there some more complicated thing we should look up.
> 
> I forget the original bugzilla that caused us to do this check but it was
> something to do with dbus running witht he wrong context, I believe.

I think the only error case of interest is when
security_compute_av_flags_raw() returns -1 with errno EINVAL.  That can
mean that the source or target security context are no longer valid,
e.g. due to a policy reload since the time the SIDs were allocated, or
that the target security class is invalid, e.g. a userspace security
class unknown to the loaded policy.  (Or it could just represent random
memory corruption in the application, of course).

Returning 0 from avc_has_perm_noaudit() could be a source of subtle bugs
without ensuring that the avd has been initialized sanely.
avc_has_perm() initializes it before calling avc_has_perm_noaudit(), but
maybe that should be moved inside of avc_has_perm_noaudit() if avd is set?

Then the subsequent avc_audit() call by avc_has_perm() should set denied
to 0 (as avd->allowed will be initialized to 0), but audited will be set
to 0 because avd->auditdeny will be initialized to 0 too.  So we'll
audit nothing and get no indication of the error at that point.

We could instead initialize avd in a similar way as the kernel does:
static void avd_init(struct av_decision *avd)
{
        avd->allowed = 0;
        avd->auditallow = 0;
        avd->auditdeny = 0xffffffff;
        avd->seqno = avc_cache.latest_notif;
        avd->flags = 0;
}

And call that on entry to avc_has_perm_noaudit() if avd is non-NULL.
Then we'll get an audit message as well, although the security contexts
may be invalid (but will be shown as their string form) or the class may
be invalid (in which case we'll get a null string).










--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Paul Moore]

On Monday, October 28, 2013 02:24:35 PM Daniel J Walsh wrote:
> On 10/28/2013 02:10 PM, Paul Moore wrote:
> > On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
> >> On 10/28/2013 11:56 AM, Eric Paris wrote:
> >>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
> >>>> Maybe the solution here is to add logging messages to the function.
> >>>> 
> >>>> My opionion is that if something is wrong with SELinux, IE The labels
> >>>> are wrong, the policy is wrong or the app is wrong, we should not
> >>>> block in permissive mode.
> >>>> 
> >>>> Having the tool write "foobar_t is not a valid source context" would
> >>>> be better then what we have now, which is a silent denial even in
> >>>> permissive mode.>
> >>> 
> >>> I understand Stephen's argument.  But agree with dwalsh/bigon that
> >>> hiding this in the library is a lot better than moving the logic to
> >>> userspace programs.  So this might not be so super simple to do.  How
> >>> about the idea of a new interface which always returns 0 in
> >>> permissive? But it does a couple of extra things.  These are just rough
> >>> early thoughts....
> >>> 
> >>> 0) new interface just like avc_has_perm() but which always returns 0
> >>> in permissive.
> >>> 
> >>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
> >>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
> >>> message into the audit log if not.
> >>> 
> >>> 2) a new /sys/fs/selinux/context like mechanism, which will both
> >>> validate the context and will force it into the sid cache.  So
> >>> subsequent broken calls to avc_has_perm() will not generate a second
> >>> SELINX_USER_ERR message, since the second call to 'access' will find a
> >>> valid type and will give a denial for that unlabeled_t type?
> >>> 
> >>> maybe /sys/fs/selinux/access should be changed/new interface added to
> >>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and
> >>> forcing the invalid label into the sid cache?
> >>> 
> >>> I really do think that userspace object managers should be allowed to
> >>> call avc_has_perm() and either get an error that should be handled as
> >>> a hard failure or a 0...   checking permissive in userspace object
> >>> managers just seems prone to breakage...
> >> 
> >> I'm ok with changing avc_has_perm as long as: a) Something gets
> >> logged/audited so you'll see that something went wrong in permissive mode
> >> and not just get silent failures in enforcing mode,
> >> 
> >> b) We are careful about what error conditions are remapped to 0 in
> >> permissive mode.  If we just hit a memory allocation failure, we
> >> shouldn't hide that from the caller.  It should only affect things
> >> relating to policy.
> > 
> > I'm far from an expert on the SELinux userland libraries, but as far as my
> > two cents are concerned I like the idea of changing avc_has_perm() to
> > incorporate the permissive/enforcing logic.  I think asking applications
> > to worry about things like that is a step in the wrong direction.
> > 
> > I also agree with Stephen's comments above: logging is important and the
> > only failures that should be ignored by avc_has_perm() are those relating
> > to access denials from policy, error conditions should propagate to the
> > caller.
> 
> The only functions that can fail are the following:
> 
> 		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
> 		if (rc) {
> 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
> 							   tclass, requested,
> 							   &entry.avd);
> 			if (rc)
> 				goto out;
> 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
> 
> 
> Are we stating that we should check for EINVAL and return 0 if permissive,
> or is there some more complicated thing we should look up.

Perhaps I'm mistaken, but I believe the concern has to do with what the kernel 
returns via security_compute_av_flags_raw().  I think the idea is to ensure 
that non-policy related error codes returned from the kernel are not papered 
over when running in permissive mode.

-- 
paul moore
www.paul-moore.com


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

[-- Attachment #1: Type: text/plain, Size: 5610 bytes --]

On 10/28/2013 03:00 PM, Stephen Smalley wrote:
> On 10/28/2013 02:24 PM, Daniel J Walsh wrote:
>> On 10/28/2013 02:10 PM, Paul Moore wrote:
>>> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
>>>> On 10/28/2013 11:56 AM, Eric Paris wrote:
>>>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>>>>>> Maybe the solution here is to add logging messages to the function.
>>>>>>
>>>>>> My opionion is that if something is wrong with SELinux, IE The labels
>>>>>> are wrong, the policy is wrong or the app is wrong, we should not
>>>>>> block in permissive mode.
>>>>>>
>>>>>> Having the tool write "foobar_t is not a valid source context" would
>>>>>> be better then what we have now, which is a silent denial even in 
>>>>>> permissive mode.>
>>>>> I understand Stephen's argument.  But agree with dwalsh/bigon that 
>>>>> hiding this in the library is a lot better than moving the logic to 
>>>>> userspace programs.  So this might not be so super simple to do.  How 
>>>>> about the idea of a new interface which always returns 0 in
>>>>> permissive? But it does a couple of extra things.  These are just rough
>>>>> early thoughts....
>>>>>
>>>>> 0) new interface just like avc_has_perm() but which always returns 0
>>>>> in permissive.
>>>>>
>>>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the 
>>>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR 
>>>>> message into the audit log if not.
>>>>>
>>>>> 2) a new /sys/fs/selinux/context like mechanism, which will both 
>>>>> validate the context and will force it into the sid cache.  So 
>>>>> subsequent broken calls to avc_has_perm() will not generate a second 
>>>>> SELINX_USER_ERR message, since the second call to 'access' will find a 
>>>>> valid type and will give a denial for that unlabeled_t type?
>>>>>
>>>>> maybe /sys/fs/selinux/access should be changed/new interface added to
>>>>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and 
>>>>> forcing the invalid label into the sid cache?
>>>>>
>>>>> I really do think that userspace object managers should be allowed to 
>>>>> call avc_has_perm() and either get an error that should be handled as
>>>>> a hard failure or a 0...   checking permissive in userspace object 
>>>>> managers just seems prone to breakage...
>>>>
>>>> I'm ok with changing avc_has_perm as long as: a) Something gets
>>>> logged/audited so you'll see that something went wrong in permissive mode
>>>> and not just get silent failures in enforcing mode,
>>>>
>>>> b) We are careful about what error conditions are remapped to 0 in 
>>>> permissive mode.  If we just hit a memory allocation failure, we 
>>>> shouldn't hide that from the caller.  It should only affect things 
>>>> relating to policy.
>>
>>> I'm far from an expert on the SELinux userland libraries, but as far as my
>>> two cents are concerned I like the idea of changing avc_has_perm() to
>>> incorporate the permissive/enforcing logic.  I think asking applications to
>>> worry about things like that is a step in the wrong direction.
>>
>>> I also agree with Stephen's comments above: logging is important and the
>>> only failures that should be ignored by avc_has_perm() are those relating
>>> to access denials from policy, error conditions should propagate to the
>>> caller.
>>
>> The only functions that can fail are the following:
>>
>> 		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
>> 		if (rc) {
>> 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
>> 							   tclass, requested,
>> 							   &entry.avd);
>> 			if (rc)
>> 				goto out;
>> 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
>>
>>
>> Are we stating that we should check for EINVAL and return 0 if permissive, or
>> is there some more complicated thing we should look up.
>>
>> I forget the original bugzilla that caused us to do this check but it was
>> something to do with dbus running witht he wrong context, I believe.
> 
> I think the only error case of interest is when
> security_compute_av_flags_raw() returns -1 with errno EINVAL.  That can
> mean that the source or target security context are no longer valid,
> e.g. due to a policy reload since the time the SIDs were allocated, or
> that the target security class is invalid, e.g. a userspace security
> class unknown to the loaded policy.  (Or it could just represent random
> memory corruption in the application, of course).
> 
> Returning 0 from avc_has_perm_noaudit() could be a source of subtle bugs
> without ensuring that the avd has been initialized sanely.
> avc_has_perm() initializes it before calling avc_has_perm_noaudit(), but
> maybe that should be moved inside of avc_has_perm_noaudit() if avd is set?
> 
> Then the subsequent avc_audit() call by avc_has_perm() should set denied
> to 0 (as avd->allowed will be initialized to 0), but audited will be set
> to 0 because avd->auditdeny will be initialized to 0 too.  So we'll
> audit nothing and get no indication of the error at that point.
> 
> We could instead initialize avd in a similar way as the kernel does:
> static void avd_init(struct av_decision *avd)
> {
>         avd->allowed = 0;
>         avd->auditallow = 0;
>         avd->auditdeny = 0xffffffff;
>         avd->seqno = avc_cache.latest_notif;
>         avd->flags = 0;
> }
> 
> And call that on entry to avc_has_perm_noaudit() if avd is non-NULL.
> Then we'll get an audit message as well, although the security contexts
> may be invalid (but will be shown as their string form) or the class may
> be invalid (in which case we'll get a null string).

Like this:



[-- Attachment #2: libselinux-avc.diff --]
[-- Type: text/x-patch, Size: 1444 bytes --]

diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
index 802a07f..f14eeb7 100644
--- a/libselinux/src/avc.c
+++ b/libselinux/src/avc.c
@@ -739,6 +739,16 @@ void avc_audit(security_id_t ssid, security_id_t tsid,
 
 hidden_def(avc_audit)
 
+
+static void avd_init(struct av_decision *avd)
+{
+	avd->allowed = 0;
+	avd->auditallow = 0;
+	avd->auditdeny = 0xffffffff;
+	avd->seqno = avc_cache.latest_notif;
+	avd->flags = 0;
+}
+
 int avc_has_perm_noaudit(security_id_t ssid,
 			 security_id_t tsid,
 			 security_class_t tclass,
@@ -751,6 +761,9 @@ int avc_has_perm_noaudit(security_id_t ssid,
 	access_vector_t denied;
 	struct avc_entry_ref ref;
 
+	if (avd)
+		avd_init(avd);
+
 	if (!avc_using_threads && !avc_app_main_loop) {
 		(void)avc_netlink_check_nb();
 	}
@@ -783,6 +796,10 @@ int avc_has_perm_noaudit(security_id_t ssid,
 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
 							   tclass, requested,
 							   &entry.avd);
+			if (rc && errno == EINVAL && !avc_enforcing) {
+				rc = errno = 0;
+				goto out;
+			}
 			if (rc)
 				goto out;
 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
@@ -821,8 +838,6 @@ int avc_has_perm(security_id_t ssid, security_id_t tsid,
 	struct av_decision avd;
 	int errsave, rc;
 
-	memset(&avd, 0, sizeof(avd));
-
 	rc = avc_has_perm_noaudit(ssid, tsid, tclass, requested, aeref, &avd);
 	errsave = errno;
 	avc_audit(ssid, tsid, tclass, requested, &avd, rc, auditdata);

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

On 10/28/2013 03:03 PM, Paul Moore wrote:
> On Monday, October 28, 2013 02:24:35 PM Daniel J Walsh wrote:
>> On 10/28/2013 02:10 PM, Paul Moore wrote:
>>> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
>>>> On 10/28/2013 11:56 AM, Eric Paris wrote:
>>>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>>>>>> Maybe the solution here is to add logging messages to the function.
>>>>>>
>>>>>> My opionion is that if something is wrong with SELinux, IE The labels
>>>>>> are wrong, the policy is wrong or the app is wrong, we should not
>>>>>> block in permissive mode.
>>>>>>
>>>>>> Having the tool write "foobar_t is not a valid source context" would
>>>>>> be better then what we have now, which is a silent denial even in
>>>>>> permissive mode.>
>>>>>
>>>>> I understand Stephen's argument.  But agree with dwalsh/bigon that
>>>>> hiding this in the library is a lot better than moving the logic to
>>>>> userspace programs.  So this might not be so super simple to do.  How
>>>>> about the idea of a new interface which always returns 0 in
>>>>> permissive? But it does a couple of extra things.  These are just rough
>>>>> early thoughts....
>>>>>
>>>>> 0) new interface just like avc_has_perm() but which always returns 0
>>>>> in permissive.
>>>>>
>>>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
>>>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
>>>>> message into the audit log if not.
>>>>>
>>>>> 2) a new /sys/fs/selinux/context like mechanism, which will both
>>>>> validate the context and will force it into the sid cache.  So
>>>>> subsequent broken calls to avc_has_perm() will not generate a second
>>>>> SELINX_USER_ERR message, since the second call to 'access' will find a
>>>>> valid type and will give a denial for that unlabeled_t type?
>>>>>
>>>>> maybe /sys/fs/selinux/access should be changed/new interface added to
>>>>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and
>>>>> forcing the invalid label into the sid cache?
>>>>>
>>>>> I really do think that userspace object managers should be allowed to
>>>>> call avc_has_perm() and either get an error that should be handled as
>>>>> a hard failure or a 0...   checking permissive in userspace object
>>>>> managers just seems prone to breakage...
>>>>
>>>> I'm ok with changing avc_has_perm as long as: a) Something gets
>>>> logged/audited so you'll see that something went wrong in permissive mode
>>>> and not just get silent failures in enforcing mode,
>>>>
>>>> b) We are careful about what error conditions are remapped to 0 in
>>>> permissive mode.  If we just hit a memory allocation failure, we
>>>> shouldn't hide that from the caller.  It should only affect things
>>>> relating to policy.
>>>
>>> I'm far from an expert on the SELinux userland libraries, but as far as my
>>> two cents are concerned I like the idea of changing avc_has_perm() to
>>> incorporate the permissive/enforcing logic.  I think asking applications
>>> to worry about things like that is a step in the wrong direction.
>>>
>>> I also agree with Stephen's comments above: logging is important and the
>>> only failures that should be ignored by avc_has_perm() are those relating
>>> to access denials from policy, error conditions should propagate to the
>>> caller.
>>
>> The only functions that can fail are the following:
>>
>> 		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
>> 		if (rc) {
>> 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
>> 							   tclass, requested,
>> 							   &entry.avd);
>> 			if (rc)
>> 				goto out;
>> 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
>>
>>
>> Are we stating that we should check for EINVAL and return 0 if permissive,
>> or is there some more complicated thing we should look up.
> 
> Perhaps I'm mistaken, but I believe the concern has to do with what the kernel 
> returns via security_compute_av_flags_raw().  I think the idea is to ensure 
> that non-policy related error codes returned from the kernel are not papered 
> over when running in permissive mode.

I think the kernel side is fine as is; it will only return EINVAL if one
of the security contexts is invalid.  Looking again, I see that invalid
class is just handled like any other denial in the kernel these days, so
EINVAL from writing to /sys/fs/selinux/access is unambiguously an
invalid security context string (could be the source or target).

I think we just need the userspace AVC to handle it cleanly and we'll be
fine.   I think my patch will work, but don't have a test case offhand;
I think we'd essentially need to launch dbusd, go permissive, remove its
domain from policy, and then trigger a dbus check?



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Paul Moore]

On Monday, October 28, 2013 03:14:48 PM Stephen Smalley wrote:
> On 10/28/2013 03:03 PM, Paul Moore wrote:
> > On Monday, October 28, 2013 02:24:35 PM Daniel J Walsh wrote:
> >> On 10/28/2013 02:10 PM, Paul Moore wrote:
> >>> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
> >>>> On 10/28/2013 11:56 AM, Eric Paris wrote:
> >>>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
> >>>>>> Maybe the solution here is to add logging messages to the function.
> >>>>>> 
> >>>>>> My opionion is that if something is wrong with SELinux, IE The labels
> >>>>>> are wrong, the policy is wrong or the app is wrong, we should not
> >>>>>> block in permissive mode.
> >>>>>> 
> >>>>>> Having the tool write "foobar_t is not a valid source context" would
> >>>>>> be better then what we have now, which is a silent denial even in
> >>>>>> permissive mode.>
> >>>>> 
> >>>>> I understand Stephen's argument.  But agree with dwalsh/bigon that
> >>>>> hiding this in the library is a lot better than moving the logic to
> >>>>> userspace programs.  So this might not be so super simple to do.  How
> >>>>> about the idea of a new interface which always returns 0 in
> >>>>> permissive? But it does a couple of extra things.  These are just
> >>>>> rough
> >>>>> early thoughts....
> >>>>> 
> >>>>> 0) new interface just like avc_has_perm() but which always returns 0
> >>>>> in permissive.
> >>>>> 
> >>>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the
> >>>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR
> >>>>> message into the audit log if not.
> >>>>> 
> >>>>> 2) a new /sys/fs/selinux/context like mechanism, which will both
> >>>>> validate the context and will force it into the sid cache.  So
> >>>>> subsequent broken calls to avc_has_perm() will not generate a second
> >>>>> SELINX_USER_ERR message, since the second call to 'access' will find a
> >>>>> valid type and will give a denial for that unlabeled_t type?
> >>>>> 
> >>>>> maybe /sys/fs/selinux/access should be changed/new interface added to
> >>>>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and
> >>>>> forcing the invalid label into the sid cache?
> >>>>> 
> >>>>> I really do think that userspace object managers should be allowed to
> >>>>> call avc_has_perm() and either get an error that should be handled as
> >>>>> a hard failure or a 0...   checking permissive in userspace object
> >>>>> managers just seems prone to breakage...
> >>>> 
> >>>> I'm ok with changing avc_has_perm as long as: a) Something gets
> >>>> logged/audited so you'll see that something went wrong in permissive
> >>>> mode
> >>>> and not just get silent failures in enforcing mode,
> >>>> 
> >>>> b) We are careful about what error conditions are remapped to 0 in
> >>>> permissive mode.  If we just hit a memory allocation failure, we
> >>>> shouldn't hide that from the caller.  It should only affect things
> >>>> relating to policy.
> >>> 
> >>> I'm far from an expert on the SELinux userland libraries, but as far as
> >>> my
> >>> two cents are concerned I like the idea of changing avc_has_perm() to
> >>> incorporate the permissive/enforcing logic.  I think asking applications
> >>> to worry about things like that is a step in the wrong direction.
> >>> 
> >>> I also agree with Stephen's comments above: logging is important and the
> >>> only failures that should be ignored by avc_has_perm() are those
> >>> relating
> >>> to access denials from policy, error conditions should propagate to the
> >>> caller.
> >> 
> >> The only functions that can fail are the following:
> >> 		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
> >> 		if (rc) {
> >> 		
> >> 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
> >> 			
> >> 							   tclass, requested,
> >> 							   &entry.avd);
> >> 			
> >> 			if (rc)
> >> 			
> >> 				goto out;
> >> 			
> >> 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
> >> 
> >> Are we stating that we should check for EINVAL and return 0 if
> >> permissive,
> >> or is there some more complicated thing we should look up.
> > 
> > Perhaps I'm mistaken, but I believe the concern has to do with what the
> > kernel returns via security_compute_av_flags_raw().  I think the idea is
> > to ensure that non-policy related error codes returned from the kernel
> > are not papered over when running in permissive mode.
> 
> I think the kernel side is fine as is; it will only return EINVAL if one
> of the security contexts is invalid.

My mistake, I should have been more clear ... Yes, I agree the kernel is fine, 
what I meant to get across was what you already said (far better than I): when 
in permissive mode the library should return 0 when performing access control 
checks and a policy related denial is returned from the kernel.

> Looking again, I see that invalid class is just handled like any other
> denial in the kernel these days, so EINVAL from writing to
> /sys/fs/selinux/access is unambiguously an invalid security context string
> (could be the source or target).
> 
> I think we just need the userspace AVC to handle it cleanly and we'll be
> fine.   I think my patch will work, but don't have a test case offhand;
> I think we'd essentially need to launch dbusd, go permissive, remove its
> domain from policy, and then trigger a dbus check?

Dan is probably the most knowledgeable here ... if it was left to me I'd 
probably just cobble up some little test program, but that's probably 
overkill.

-- 
paul moore
www.paul-moore.com


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

[-- Attachment #1: Type: text/plain, Size: 5914 bytes --]

On 10/28/2013 03:09 PM, Stephen Smalley wrote:
> On 10/28/2013 03:00 PM, Stephen Smalley wrote:
>> On 10/28/2013 02:24 PM, Daniel J Walsh wrote:
>>> On 10/28/2013 02:10 PM, Paul Moore wrote:
>>>> On Monday, October 28, 2013 12:58:55 PM Stephen Smalley wrote:
>>>>> On 10/28/2013 11:56 AM, Eric Paris wrote:
>>>>>> On Mon, 2013-10-28 at 10:46 -0400, Daniel J Walsh wrote:
>>>>>>> Maybe the solution here is to add logging messages to the function.
>>>>>>>
>>>>>>> My opionion is that if something is wrong with SELinux, IE The labels
>>>>>>> are wrong, the policy is wrong or the app is wrong, we should not
>>>>>>> block in permissive mode.
>>>>>>>
>>>>>>> Having the tool write "foobar_t is not a valid source context" would
>>>>>>> be better then what we have now, which is a silent denial even in 
>>>>>>> permissive mode.>
>>>>>> I understand Stephen's argument.  But agree with dwalsh/bigon that 
>>>>>> hiding this in the library is a lot better than moving the logic to 
>>>>>> userspace programs.  So this might not be so super simple to do.  How 
>>>>>> about the idea of a new interface which always returns 0 in
>>>>>> permissive? But it does a couple of extra things.  These are just rough
>>>>>> early thoughts....
>>>>>>
>>>>>> 0) new interface just like avc_has_perm() but which always returns 0
>>>>>> in permissive.
>>>>>>
>>>>>> 1) a new SELINX_USER_ERR audit message.  On EINVAL we check if the 
>>>>>> scontext/tcontext are valid and print the equivalent of a SELINUX_ERR 
>>>>>> message into the audit log if not.
>>>>>>
>>>>>> 2) a new /sys/fs/selinux/context like mechanism, which will both 
>>>>>> validate the context and will force it into the sid cache.  So 
>>>>>> subsequent broken calls to avc_has_perm() will not generate a second 
>>>>>> SELINX_USER_ERR message, since the second call to 'access' will find a 
>>>>>> valid type and will give a denial for that unlabeled_t type?
>>>>>>
>>>>>> maybe /sys/fs/selinux/access should be changed/new interface added to
>>>>>> do all of this in kernel?  generating a real SELINUX_ERR in kernel and 
>>>>>> forcing the invalid label into the sid cache?
>>>>>>
>>>>>> I really do think that userspace object managers should be allowed to 
>>>>>> call avc_has_perm() and either get an error that should be handled as
>>>>>> a hard failure or a 0...   checking permissive in userspace object 
>>>>>> managers just seems prone to breakage...
>>>>>
>>>>> I'm ok with changing avc_has_perm as long as: a) Something gets
>>>>> logged/audited so you'll see that something went wrong in permissive mode
>>>>> and not just get silent failures in enforcing mode,
>>>>>
>>>>> b) We are careful about what error conditions are remapped to 0 in 
>>>>> permissive mode.  If we just hit a memory allocation failure, we 
>>>>> shouldn't hide that from the caller.  It should only affect things 
>>>>> relating to policy.
>>>
>>>> I'm far from an expert on the SELinux userland libraries, but as far as my
>>>> two cents are concerned I like the idea of changing avc_has_perm() to
>>>> incorporate the permissive/enforcing logic.  I think asking applications to
>>>> worry about things like that is a step in the wrong direction.
>>>
>>>> I also agree with Stephen's comments above: logging is important and the
>>>> only failures that should be ignored by avc_has_perm() are those relating
>>>> to access denials from policy, error conditions should propagate to the
>>>> caller.
>>>
>>> The only functions that can fail are the following:
>>>
>>> 		rc = avc_lookup(ssid, tsid, tclass, requested, aeref);
>>> 		if (rc) {
>>> 			rc = security_compute_av_flags_raw(ssid->ctx, tsid->ctx,
>>> 							   tclass, requested,
>>> 							   &entry.avd);
>>> 			if (rc)
>>> 				goto out;
>>> 			rc = avc_insert(ssid, tsid, tclass, &entry, aeref);
>>>
>>>
>>> Are we stating that we should check for EINVAL and return 0 if permissive, or
>>> is there some more complicated thing we should look up.
>>>
>>> I forget the original bugzilla that caused us to do this check but it was
>>> something to do with dbus running witht he wrong context, I believe.
>>
>> I think the only error case of interest is when
>> security_compute_av_flags_raw() returns -1 with errno EINVAL.  That can
>> mean that the source or target security context are no longer valid,
>> e.g. due to a policy reload since the time the SIDs were allocated, or
>> that the target security class is invalid, e.g. a userspace security
>> class unknown to the loaded policy.  (Or it could just represent random
>> memory corruption in the application, of course).
>>
>> Returning 0 from avc_has_perm_noaudit() could be a source of subtle bugs
>> without ensuring that the avd has been initialized sanely.
>> avc_has_perm() initializes it before calling avc_has_perm_noaudit(), but
>> maybe that should be moved inside of avc_has_perm_noaudit() if avd is set?
>>
>> Then the subsequent avc_audit() call by avc_has_perm() should set denied
>> to 0 (as avd->allowed will be initialized to 0), but audited will be set
>> to 0 because avd->auditdeny will be initialized to 0 too.  So we'll
>> audit nothing and get no indication of the error at that point.
>>
>> We could instead initialize avd in a similar way as the kernel does:
>> static void avd_init(struct av_decision *avd)
>> {
>>         avd->allowed = 0;
>>         avd->auditallow = 0;
>>         avd->auditdeny = 0xffffffff;
>>         avd->seqno = avc_cache.latest_notif;
>>         avd->flags = 0;
>> }
>>
>> And call that on entry to avc_has_perm_noaudit() if avd is non-NULL.
>> Then we'll get an audit message as well, although the security contexts
>> may be invalid (but will be shown as their string form) or the class may
>> be invalid (in which case we'll get a null string).
> 
> Like this:

Alternatively, we could go with this one to ensure that in the enforcing
case, we get EACCES rather than EINVAL back in the original caller.





[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-avc_has_perm-returns-1-even-when-SELinux-is-in-p.patch --]
[-- Type: text/x-patch; name="0001-Fix-avc_has_perm-returns-1-even-when-SELinux-is-in-p.patch", Size: 0 bytes --]

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Eric Paris]

On Mon, 2013-10-28 at 15:14 -0400, Stephen Smalley wrote:

> I think we just need the userspace AVC to handle it cleanly and we'll be
> fine.   I think my patch will work, but don't have a test case offhand;

Hard for me to test on Fedora with the return 0;

setenforce 0
touch /etc/systemd/system/hello.service
chcon -t invalid_t /etc/systemd/system/hello.service
semanage permissive -a init_t  (needed so init itself can read the file)

setenforce 1
systemctl status hello.service
This shouldn't be silent, but it seems like it is, I'd have expected an
USER_AVC between my user type and the unlabeled_t...

setenforce 0
systemctl status hello.service
On Fedora this works, on others, it'll likely fail with EINVAL, (since
init will have CAP_MAC_ADMIN in permissive.)  init will be able to read
invalid_t (in enforcing it'll see unlabeled_t) and should pass that down
in the security check and get rejected/need and audit message...

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Paul Moore]

On Monday, October 28, 2013 03:26:12 PM Stephen Smalley wrote:
> Alternatively, we could go with this one to ensure that in the enforcing
> case, we get EACCES rather than EINVAL back in the original caller.

Unless there have been reports of applications not being able to deal with 
errors other than EACCESS, I'm in favor of not masking EINVAL in the enforcing 
case.

-- 
paul moore
www.paul-moore.com


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: avc_has_perm() returns -1 even when SELinux is in permissive mode

[Stephen Smalley]

On 10/28/2013 03:41 PM, Eric Paris wrote:
> On Mon, 2013-10-28 at 15:14 -0400, Stephen Smalley wrote:
> 
>> I think we just need the userspace AVC to handle it cleanly and we'll be
>> fine.   I think my patch will work, but don't have a test case offhand;
> 
> Hard for me to test on Fedora with the return 0;
> 
> setenforce 0
> touch /etc/systemd/system/hello.service
> chcon -t invalid_t /etc/systemd/system/hello.service
> semanage permissive -a init_t  (needed so init itself can read the file)
> 
> setenforce 1
> systemctl status hello.service
> This shouldn't be silent, but it seems like it is, I'd have expected an
> USER_AVC between my user type and the unlabeled_t...

# systemctl status hello.service
Failed to issue method call: Access denied
# ausearch -m USER_AVC -ts recent
time->Mon Oct 28 16:46:15 2013
type=USER_AVC msg=audit(1382993175.466:585): pid=1 uid=0 auid=4294967295
ses=4294967295  subj=system_u:system_r:init_t:s0 msg='avc:  denied  {
status } for auid=4204 uid=0 gid=0
path="/etc/systemd/system/hello.service" cmdline="systemctl status
hello.service"
scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
tcontext=unconfined_u:object_r:invalid_t:s0 tclass=service
exe="/usr/lib/systemd/systemd" sauid=0 hostname=? addr=? terminal=?'

> setenforce 0
> systemctl status hello.service
> On Fedora this works, on others, it'll likely fail with EINVAL, (since
> init will have CAP_MAC_ADMIN in permissive.)  init will be able to read
> invalid_t (in enforcing it'll see unlabeled_t) and should pass that down
> in the security check and get rejected/need and audit message...

# systemctl status hello.service
hello.service
   Loaded: masked (/etc/systemd/system/hello.service; masked)
   Active: inactive (dead)





--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.