Another change we would like to make to libselinux

Another change we would like to make to libselinux

[Daniel J Walsh]

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

Currently we have lots of apps trying to figure out which policy is
installed on the system,  We have a function
selinux_binary_policy_path which returns a path like
'/etc/selinux/targeted/policy'

Then these apps do stuff like:

VER=`cat /sys/fs/selinux/policyver`
while [ -e '/etc/selinux/targetd/policy. + $VER' ]; do
      VER=$VER-1
done

While we have had /sys/fs/selinux/policy for a while now.

I wanted to add an interface to return this path, but I was trying to
figure out a name selinux_loaded_policy_path for example, but as Eric
pointed out to me, selinux_binary_policy_path is what most users would
expect to return this.  If you look at the man page it even suggest this.

man selinux_binary_policy_path
...
       selinux_binary_policy_path() - binary policy file loaded into
kernel


Currently the users of this function are the libselinux package,
setools and policycorutils (sepolgen-ifgen).


I am torn between adding stealing this function to return the
/sys/fs/selinux/policy and then adding selinux_installed_policy_path
for the original function, then updating the effected packages.

The problem with this is we would have different behaviour between
older versions of the library. The other options would be to come up
with a better name for the new function and fix the man pages.

Suggestions welcomed.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk89Fu8ACgkQrlYvE4MpobNimgCeIIpi+6Yee+GfyUlRV+n7jvaE
39MAn064N3sRCAdpyseBKFrGR3+ojrEI
=+Byb
-----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: Another change we would like to make to libselinux

[Stephen Smalley]

On Thu, 2012-02-16 at 09:47 -0500, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Currently we have lots of apps trying to figure out which policy is
> installed on the system,  We have a function
> selinux_binary_policy_path which returns a path like
> '/etc/selinux/targeted/policy'
> 
> Then these apps do stuff like:
> 
> VER=`cat /sys/fs/selinux/policyver`
> while [ -e '/etc/selinux/targetd/policy. + $VER' ]; do
>       VER=$VER-1
> done
> 
> While we have had /sys/fs/selinux/policy for a while now.
> 
> I wanted to add an interface to return this path, but I was trying to
> figure out a name selinux_loaded_policy_path for example, but as Eric
> pointed out to me, selinux_binary_policy_path is what most users would
> expect to return this.  If you look at the man page it even suggest this.
> 
> man selinux_binary_policy_path
> ...
>        selinux_binary_policy_path() - binary policy file loaded into
> kernel
> 
> 
> Currently the users of this function are the libselinux package,
> setools and policycorutils (sepolgen-ifgen).
> 
> 
> I am torn between adding stealing this function to return the
> /sys/fs/selinux/policy and then adding selinux_installed_policy_path
> for the original function, then updating the effected packages.
> 
> The problem with this is we would have different behaviour between
> older versions of the library. The other options would be to come up
> with a better name for the new function and fix the man pages.
> 
> Suggestions welcomed.

Most applications should not be using /sys/fs/selinux/policy, as that
requires the kernel to generate the policy image from its in-core data
structures and is expensive.  So you certainly should not change
selinux_binary_policy_path() to return that pathname.  That also would
be an incompatible interface change due to the version suffix, as you
note.

So I think you need a new interface.  selinux_kernel_policy_path() or
selinux_active_policy_path() or selinux_loaded_policy_path() seem fine
to me.

-- 
Stephen Smalley
National Security Agency


--
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: Another change we would like to make to libselinux

[Daniel J Walsh]

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

On 02/21/2012 03:43 PM, Stephen Smalley wrote:
> On Thu, 2012-02-16 at 09:47 -0500, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> Currently we have lots of apps trying to figure out which policy
>> is installed on the system,  We have a function 
>> selinux_binary_policy_path which returns a path like 
>> '/etc/selinux/targeted/policy'
>> 
>> Then these apps do stuff like:
>> 
>> VER=`cat /sys/fs/selinux/policyver` while [ -e
>> '/etc/selinux/targetd/policy. + $VER' ]; do VER=$VER-1 done
>> 
>> While we have had /sys/fs/selinux/policy for a while now.
>> 
>> I wanted to add an interface to return this path, but I was
>> trying to figure out a name selinux_loaded_policy_path for
>> example, but as Eric pointed out to me,
>> selinux_binary_policy_path is what most users would expect to
>> return this.  If you look at the man page it even suggest this.
>> 
>> man selinux_binary_policy_path ... selinux_binary_policy_path() -
>> binary policy file loaded into kernel
>> 
>> 
>> Currently the users of this function are the libselinux package, 
>> setools and policycorutils (sepolgen-ifgen).
>> 
>> 
>> I am torn between adding stealing this function to return the 
>> /sys/fs/selinux/policy and then adding
>> selinux_installed_policy_path for the original function, then
>> updating the effected packages.
>> 
>> The problem with this is we would have different behaviour
>> between older versions of the library. The other options would be
>> to come up with a better name for the new function and fix the
>> man pages.
>> 
>> Suggestions welcomed.
> 
> Most applications should not be using /sys/fs/selinux/policy, as
> that requires the kernel to generate the policy image from its
> in-core data structures and is expensive.  So you certainly should
> not change selinux_binary_policy_path() to return that pathname.
> That also would be an incompatible interface change due to the
> version suffix, as you note.
> 
> So I think you need a new interface.  selinux_kernel_policy_path()
> or selinux_active_policy_path() or selinux_loaded_policy_path()
> seem fine to me.
> 

Right now we added selinux_current_policy_path and we are using it as
the default for seinfo, audit2allow, sepolgen-ifgen

It currently looks like

const char *selinux_current_policy_path(void)
{
	int rc = 0;
	int vers = 0;
	static char policy_path[PATH_MAX];

	snprintf(policy_path, sizeof(policy_path), "%s/policy", selinux_mnt);
	if (access(policy_path, F_OK) != 0 ) {
		vers = security_policyvers();
		do {
			/* Check prior versions to see if old policy is available */
			snprintf(policy_path, sizeof(policy_path), "%s.%d",
				 selinux_binary_policy_path(), vers);
		} while ((rc = access(policy_path, F_OK)) && --vers > 0);

		if (rc) return NULL;
	}

	return policy_path;
}


Although Eric wants to make some changes to it.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9EEU8ACgkQrlYvE4MpobPk3ACePR4lD5RG9eFSMHz0ghJDey+b
tWkAoNzpy203Z99/tb1aI0aVYaoug5P3
=vdSB
-----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: Another change we would like to make to libselinux

[Christopher J. PeBenito]

On 02/21/12 16:49, Daniel J Walsh wrote:
> On 02/21/2012 03:43 PM, Stephen Smalley wrote:
>> On Thu, 2012-02-16 at 09:47 -0500, Daniel J Walsh wrote:
>>> Currently we have lots of apps trying to figure out which policy
>>> is installed on the system,  We have a function 
>>> selinux_binary_policy_path which returns a path like 
>>> '/etc/selinux/targeted/policy'
>>>
>>> Then these apps do stuff like:
>>>
>>> VER=`cat /sys/fs/selinux/policyver` while [ -e
>>> '/etc/selinux/targetd/policy. + $VER' ]; do VER=$VER-1 done
>>>
>>> While we have had /sys/fs/selinux/policy for a while now.
>>>
>>> I wanted to add an interface to return this path, but I was
>>> trying to figure out a name selinux_loaded_policy_path for
>>> example, but as Eric pointed out to me,
>>> selinux_binary_policy_path is what most users would expect to
>>> return this.  If you look at the man page it even suggest this.
>>>
>>> man selinux_binary_policy_path ... selinux_binary_policy_path() -
>>> binary policy file loaded into kernel
>>>
>>>
>>> Currently the users of this function are the libselinux package, 
>>> setools and policycorutils (sepolgen-ifgen).
>>>
>>>
>>> I am torn between adding stealing this function to return the 
>>> /sys/fs/selinux/policy and then adding
>>> selinux_installed_policy_path for the original function, then
>>> updating the effected packages.
>>>
>>> The problem with this is we would have different behaviour
>>> between older versions of the library. The other options would be
>>> to come up with a better name for the new function and fix the
>>> man pages.
>>>
>>> Suggestions welcomed.
> 
>> Most applications should not be using /sys/fs/selinux/policy, as
>> that requires the kernel to generate the policy image from its
>> in-core data structures and is expensive.  So you certainly should
>> not change selinux_binary_policy_path() to return that pathname.
>> That also would be an incompatible interface change due to the
>> version suffix, as you note.
> 
>> So I think you need a new interface.  selinux_kernel_policy_path()
>> or selinux_active_policy_path() or selinux_loaded_policy_path()
>> seem fine to me.
> 
> 
> Right now we added selinux_current_policy_path and we are using it as
> the default for seinfo, audit2allow, sepolgen-ifgen

So why is it so critical that the in-kernel policy is used for any of these tools?  I can see that for certain high assurance systems that, for example, want to measure policy integrity, would want this, but for 99.9% of the users, its unnecessary.  Its just extra overhead, when the policy is already on disk.  I think it should be an option to use the kernel policy interface, but not the default.

-- 
Chris PeBenito
Tresys Technology, LLC
www.tresys.com | oss.tresys.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: Another change we would like to make to libselinux

[Stephen Smalley]

On Tue, 2012-02-21 at 16:49 -0500, Daniel J Walsh wrote:
> Right now we added selinux_current_policy_path and we are using it as
> the default for seinfo, audit2allow, sepolgen-ifgen

I'm not sure this is a good idea to always use it, as it is definitely
more costly to generate the policy image in the kernel than to just read
the policy file.  I had only envisioned it being used by tools that want
to verify that the kernel policy matches the policy file.

> It currently looks like
> 
> const char *selinux_current_policy_path(void)
> {
> 	int rc = 0;
> 	int vers = 0;
> 	static char policy_path[PATH_MAX];
> 
> 	snprintf(policy_path, sizeof(policy_path), "%s/policy", selinux_mnt);
> 	if (access(policy_path, F_OK) != 0 ) {
> 		vers = security_policyvers();

The selinux_mkload_policy() logic starts with
sepol_policy_kern_vers_max() rather than security_policyvers() if
libsepol is available, since it can downgrade the policy file via
libsepol to the kernel version.  So your logic may end up using the
wrong policy file.

> 		do {
> 			/* Check prior versions to see if old policy is available */
> 			snprintf(policy_path, sizeof(policy_path), "%s.%d",
> 				 selinux_binary_policy_path(), vers);
> 		} while ((rc = access(policy_path, F_OK)) && --vers > 0);
> 
> 		if (rc) return NULL;
> 	}
> 
> 	return policy_path;
> }


-- 
Stephen Smalley
National Security Agency


--
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: Another change we would like to make to libselinux

[Daniel J Walsh]

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

On 02/22/2012 08:27 AM, Christopher J. PeBenito wrote:
> On 02/21/12 16:49, Daniel J Walsh wrote:
>> On 02/21/2012 03:43 PM, Stephen Smalley wrote:
>>> On Thu, 2012-02-16 at 09:47 -0500, Daniel J Walsh wrote:
>>>> Currently we have lots of apps trying to figure out which
>>>> policy is installed on the system,  We have a function 
>>>> selinux_binary_policy_path which returns a path like 
>>>> '/etc/selinux/targeted/policy'
>>>> 
>>>> Then these apps do stuff like:
>>>> 
>>>> VER=`cat /sys/fs/selinux/policyver` while [ -e 
>>>> '/etc/selinux/targetd/policy. + $VER' ]; do VER=$VER-1 done
>>>> 
>>>> While we have had /sys/fs/selinux/policy for a while now.
>>>> 
>>>> I wanted to add an interface to return this path, but I was 
>>>> trying to figure out a name selinux_loaded_policy_path for 
>>>> example, but as Eric pointed out to me, 
>>>> selinux_binary_policy_path is what most users would expect
>>>> to return this.  If you look at the man page it even suggest
>>>> this.
>>>> 
>>>> man selinux_binary_policy_path ...
>>>> selinux_binary_policy_path() - binary policy file loaded into
>>>> kernel
>>>> 
>>>> 
>>>> Currently the users of this function are the libselinux
>>>> package, setools and policycorutils (sepolgen-ifgen).
>>>> 
>>>> 
>>>> I am torn between adding stealing this function to return the
>>>>  /sys/fs/selinux/policy and then adding 
>>>> selinux_installed_policy_path for the original function,
>>>> then updating the effected packages.
>>>> 
>>>> The problem with this is we would have different behaviour 
>>>> between older versions of the library. The other options
>>>> would be to come up with a better name for the new function
>>>> and fix the man pages.
>>>> 
>>>> Suggestions welcomed.
>> 
>>> Most applications should not be using /sys/fs/selinux/policy,
>>> as that requires the kernel to generate the policy image from
>>> its in-core data structures and is expensive.  So you certainly
>>> should not change selinux_binary_policy_path() to return that
>>> pathname. That also would be an incompatible interface change
>>> due to the version suffix, as you note.
>> 
>>> So I think you need a new interface.
>>> selinux_kernel_policy_path() or selinux_active_policy_path() or
>>> selinux_loaded_policy_path() seem fine to me.
>> 
>> 
>> Right now we added selinux_current_policy_path and we are using
>> it as the default for seinfo, audit2allow, sepolgen-ifgen
> 
> So why is it so critical that the in-kernel policy is used for any
> of these tools?  I can see that for certain high assurance systems
> that, for example, want to measure policy integrity, would want
> this, but for 99.9% of the users, its unnecessary.  Its just extra
> overhead, when the policy is already on disk.  I think it should be
> an option to use the kernel policy interface, but not the default.
> 


Well I see for multiple reasons.

One the stupid algorithm for finding what was the currect policy to
read was spreading to multiple tools places.  (setools,
sepolgen-ifgen, load_policy, audit2why, each one doing it slightly
differently.

We had apps like setroubleshoot that are trying to analyze the policy
at the time that an AVC happened and potentially analyzing a different
policy.  (Theoretically try for audit2allow/audit2why also.)

sepolgen-ifgen was not figuring out the correct policy to analyze if
policy.27 was installed but the kernel was looking at policy.26

We had the ability to analyze the loaded policy and no one was using it.

I hate the algorithm for finding the path to the policy on disk.  And
would rather only expose it to one tool load_policy.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9E7u8ACgkQrlYvE4MpobMclACfesQC/naQ0z6tppObyKN8wXsl
hawAnRYEu7xbcuLjuvFZN9CERr32uPH4
=11Fr
-----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: Another change we would like to make to libselinux

[Stephen Smalley]

On Wed, 2012-02-22 at 08:34 -0500, Daniel J Walsh wrote:
> Well I see for multiple reasons.
> 
> One the stupid algorithm for finding what was the currect policy to
> read was spreading to multiple tools places.  (setools,
> sepolgen-ifgen, load_policy, audit2why, each one doing it slightly
> differently.

So that's an argument for providing an interface that gives that full
pathname of the policy file, not for using the kernel policy file by
default.  BTW, we could likely have libsemanage create a symlink
from /etc/selinux/$SELINUXTYPE/policy/policy to the policy.N file it
just created.  And then you could just always use that pathname.

> We had apps like setroubleshoot that are trying to analyze the policy
> at the time that an AVC happened and potentially analyzing a different
> policy.  (Theoretically try for audit2allow/audit2why also.)

I don't quite understand why that would happen, except for transient
boolean changes, and how often do those occur these days?

> sepolgen-ifgen was not figuring out the correct policy to analyze if
> policy.27 was installed but the kernel was looking at policy.26

How did that happen?  If policy.27 exists and libsepol supports policy
version 27, then load_policy will use policy.27 and downgrade it in
memory before loading into the kernel.  It will not use the policy.26
file.

> We had the ability to analyze the loaded policy and no one was using it.

The only reason we added the kernel policy pseudo file was to support
checking that the loaded kernel policy matched the one on disk.

> I hate the algorithm for finding the path to the policy on disk.  And
> would rather only expose it to one tool load_policy.

This is the same as the first one.

-- 
Stephen Smalley
National Security Agency


--
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: Another change we would like to make to libselinux

[Daniel J Walsh]

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

On 02/22/2012 09:08 AM, Stephen Smalley wrote:
> On Wed, 2012-02-22 at 08:34 -0500, Daniel J Walsh wrote:
>> Well I see for multiple reasons.
>> 
>> One the stupid algorithm for finding what was the currect policy
>> to read was spreading to multiple tools places.  (setools, 
>> sepolgen-ifgen, load_policy, audit2why, each one doing it
>> slightly differently.
> 
> So that's an argument for providing an interface that gives that
> full pathname of the policy file, not for using the kernel policy
> file by default.  BTW, we could likely have libsemanage create a
> symlink from /etc/selinux/$SELINUXTYPE/policy/policy to the
> policy.N file it just created.  And then you could just always use
> that pathname.
> 
That would help the problem.
>> We had apps like setroubleshoot that are trying to analyze the
>> policy at the time that an AVC happened and potentially analyzing
>> a different policy.  (Theoretically try for audit2allow/audit2why
>> also.)
> 
> I don't quite understand why that would happen, except for
> transient boolean changes, and how often do those occur these
> days?
> 
Well they might be getting more popular with booleans like
deny_ptrace.   But the fact remains we are checking a file that may or
may not be loaded into the kernel.
>> sepolgen-ifgen was not figuring out the correct policy to analyze
>> if policy.27 was installed but the kernel was looking at
>> policy.26
> 
> How did that happen?  If policy.27 exists and libsepol supports
> policy version 27, then load_policy will use policy.27 and
> downgrade it in memory before loading into the kernel.  It will not
> use the policy.26 file.
> 
Because we don't expose libsepol policy max function in a python
binding, or to libselinux in a reasonable way, so the code currently
reads policy version and reads down.  Since it starts at policy.26 it
does not find it.  Your previous complaint against my patch exposes
this.  Currently we have a hacky load_policy call that loads the
libsepol shared library with a dlopen and then gets the max_ver out of
it.  I am not sure I want to suck this call into any app that wants to
read policy.

>> We had the ability to analyze the loaded policy and no one was
>> using it.
> 
> The only reason we added the kernel policy pseudo file was to
> support checking that the loaded kernel policy matched the one on
> disk.
> 
Potentially this fix would allow the policy file to not be required
after boot.
>> I hate the algorithm for finding the path to the policy on disk.
>> And would rather only expose it to one tool load_policy.
> 
> This is the same as the first one.
> 

Maybe the best solution is for libsemanage to make the symbolic link.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9FCbwACgkQrlYvE4MpobMK1gCglSbrFcUGJW0E+Tj/Qi+3vPY0
swwAmgJ+siTpwfeLpxcWQDWLPjtfiOMe
=iGSC
-----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.