Re: [PATCH] secilc: resolve conflicts in expandattribute.

[Jeffrey Vander Stoep via Selinux]

On Wed, Mar 14, 2018 at 4:05 PM, William Roberts
 wrote:
> On Wed, Mar 14, 2018 at 3:17 PM, Tri Vo  wrote:
>> When Android combines multiple .cil files from system.img and vendor.img
>> it's possible to have conflicting expandattribute statements, e.g.
>>  expandattribute hal_audio true;
>>  expandattribute hal_audio false;
>
> Isn't this the policy.conf version? I thought cil files had:
> expandtypeattribute, am I wrong here?

You'll need to fix checkpolicy too for consistency. See
https://github.com/SELinuxProject/selinux/blob/master/checkpolicy/policy_define.c#L1185

>
>>
>> This change deals with scenario be resolving the value of the
>> corresponding expandattribute to false. The rationale behind this
>> override is that true is used for reduce run-time lookups, while
>> false is used for tests which must pass.
>
> So in a conflict, it's always forced to false, which prevents expansion.
> That seems reasonable. I would imagine this should also update some
> document somewhere that describes this behavior, does one exist?
> I couldn't find anything, but not sure if it's on some external webpage.
> Stephen do you know?
>
>> ---
>>  libsepol/cil/src/cil_resolve_ast.c | 12 
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/libsepol/cil/src/cil_resolve_ast.c 
>> b/libsepol/cil/src/cil_resolve_ast.c
>> index d1a5ed87..5c66f663 100644
>> --- a/libsepol/cil/src/cil_resolve_ast.c
>> +++ b/libsepol/cil/src/cil_resolve_ast.c
>> @@ -271,7 +271,6 @@ exit:
>>
>>  int cil_type_used(struct cil_symtab_datum *datum, int used)

Change from int to void. The return value is no longer useful.

>>  {
>> -   int rc = SEPOL_ERR;
>> struct cil_typeattribute *attr = NULL;
>>
>> if (FLAVOR(datum) == CIL_TYPEATTRIBUTE) {
>> @@ -279,16 +278,13 @@ int cil_type_used(struct cil_symtab_datum *datum, int 
>> used)
>> attr->used |= used;
>> if ((attr->used & CIL_ATTR_EXPAND_TRUE) &&
>> (attr->used & CIL_ATTR_EXPAND_FALSE)) {
>> -   cil_log(CIL_ERR, "Conflicting use of 
>> expandtypeattribute. "
>> -   "Expandtypeattribute may be set to 
>> true or false "
>> -   "but not both. \n");
>> -   goto exit;
>> +   cil_log(CIL_WARN, "Conflicting use of 
>> expandtypeattribute. "
>> +   "Expandtypeattribute was set to both 
>> true or false for %s. "
>> +   "Resolving to false. \n", 
>> attr->datum.name);
>> +   attr->used ^= CIL_ATTR_EXPAND_TRUE;
>
> Sure, this saves an operation, but:
>
> attr->used &= ~CIL_ATTR_EXPAND_TRUE;

+1

>
> Is less fragile and the usual unset idiom. I won't request this changed unless
> either you agree or someone else has the same opinion as me.
>
> One could always argue that conditional code that relies on the entry
> condition is the whole
> point of conditional code :-P
>
>> }
>> }
>> -
>> return SEPOL_OK;
>> -exit:
>> -   return rc;
>>  }
>>
>>  int cil_resolve_permissionx(struct cil_tree_node *current, struct 
>> cil_permissionx *permx, void *extra_args)
>> --
>> 2.16.2.804.g6dcf76e118-goog
>>
>>
>

Re: [PATCH] secilc: resolve conflicts in expandattribute.

[William Roberts]

On Wed, Mar 14, 2018 at 3:17 PM, Tri Vo  wrote:
> When Android combines multiple .cil files from system.img and vendor.img
> it's possible to have conflicting expandattribute statements, e.g.
>  expandattribute hal_audio true;
>  expandattribute hal_audio false;

Isn't this the policy.conf version? I thought cil files had:
expandtypeattribute, am I wrong here?

>
> This change deals with scenario be resolving the value of the
> corresponding expandattribute to false. The rationale behind this
> override is that true is used for reduce run-time lookups, while
> false is used for tests which must pass.

So in a conflict, it's always forced to false, which prevents expansion.
That seems reasonable. I would imagine this should also update some
document somewhere that describes this behavior, does one exist?
I couldn't find anything, but not sure if it's on some external webpage.
Stephen do you know?

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c 
> b/libsepol/cil/src/cil_resolve_ast.c
> index d1a5ed87..5c66f663 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -271,7 +271,6 @@ exit:
>
>  int cil_type_used(struct cil_symtab_datum *datum, int used)
>  {
> -   int rc = SEPOL_ERR;
> struct cil_typeattribute *attr = NULL;
>
> if (FLAVOR(datum) == CIL_TYPEATTRIBUTE) {
> @@ -279,16 +278,13 @@ int cil_type_used(struct cil_symtab_datum *datum, int 
> used)
> attr->used |= used;
> if ((attr->used & CIL_ATTR_EXPAND_TRUE) &&
> (attr->used & CIL_ATTR_EXPAND_FALSE)) {
> -   cil_log(CIL_ERR, "Conflicting use of 
> expandtypeattribute. "
> -   "Expandtypeattribute may be set to 
> true or false "
> -   "but not both. \n");
> -   goto exit;
> +   cil_log(CIL_WARN, "Conflicting use of 
> expandtypeattribute. "
> +   "Expandtypeattribute was set to both 
> true or false for %s. "
> +   "Resolving to false. \n", 
> attr->datum.name);
> +   attr->used ^= CIL_ATTR_EXPAND_TRUE;

Sure, this saves an operation, but:

attr->used &= ~CIL_ATTR_EXPAND_TRUE;

Is less fragile and the usual unset idiom. I won't request this changed unless
either you agree or someone else has the same opinion as me.

One could always argue that conditional code that relies on the entry
condition is the whole
point of conditional code :-P

> }
> }
> -
> return SEPOL_OK;
> -exit:
> -   return rc;
>  }
>
>  int cil_resolve_permissionx(struct cil_tree_node *current, struct 
> cil_permissionx *permx, void *extra_args)
> --
> 2.16.2.804.g6dcf76e118-goog
>
>

Re: [PATCH v2 1/1] libselinux, libsemanage: Replace PYSITEDIR with PYTHONLIBDIR

[Petr Lautrbach]

On Sun, Mar 11, 2018 at 11:15:27PM +0100, Nicolas Iooss wrote:
> libselinux and libsemanage Makefiles invoke site.getsitepackages() in
> order to get the path to the directory /usr/lib/pythonX.Y/site-packages
> that matches the Python interpreter chosen with $(PYTHON). This method
> is incompatible with Python virtual environments, as described in
> https://github.com/pypa/virtualenv/issues/355#issuecomment-10250452 .
> This issue has been opened for more than 5 years.
> 
> On the contrary python/semanage/ and python/sepolgen/ Makefiles use
> distutils.sysconfig.get_python_lib() in order to get the site-packages
> path into a variable named PYTHONLIBDIR. This way of computing
> PYTHONLIBDIR is compatible with virtual environments and gives the same
> result as PYSITEDIR.
> 
> As PYTHONLIBDIR works in more cases than PYSITEDIR, make libselinux and
> libsemanage Makefiles use it. And as native code is installed (as part
> of the SWIG wrapper), use "plat_specific=1" in order to use /usr/lib64
> on systems which distinguish /usr/lib64 from /usr/lib.
> 
> Signed-off-by: Nicolas Iooss 

Looks good to me. Thanks!

https://github.com/SELinuxProject/selinux/pull/86

Acked-by: Petr Lautrbach 


> ---
> v2: add plat_specific=1
> 
>  .travis.yml  |  5 +
>  libselinux/src/Makefile  | 10 +-
>  libsemanage/src/Makefile |  8 
>  3 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 0312e996e333..63c7a544aa45 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -96,9 +96,6 @@ before_script:
>- export PKG_CONFIG_PATH="/opt/python/$($PYTHON -c 'import 
> sys;print("%d.%d.%d" % sys.version_info[:3])')/lib/pkgconfig"
># PyPy does not provide a config file for pkg-config nor a pypy-c.so
>- if echo "$PYVER" | grep -q pypy ; then export PYINC=-I$($PYTHON -c 
> 'import sys;print(sys.prefix)')/include PYLIBS= ; fi
> -  # Python virtualenvs do not support "import site; 
> print(site.getsitepackages()[0]"
> -  # cf. https://github.com/pypa/virtualenv/issues/355#issuecomment-10250452
> -  - export PYSITEDIR="/usr/lib/$($PYTHON -c 'import sys;print("python%d.%d" 
> % sys.version_info[:2])')/site-packages"
>  
># Find the Ruby executable with version $RUBYLIBVER
>- export RUBY="$(ls -d -1 "$HOME/.rvm/rubies/ruby-$RUBYLIBVER"*/bin/ruby | 
> head -n 1)"
> @@ -126,7 +123,7 @@ script:
># Set up environment variables for the tests
>- export LD_LIBRARY_PATH="$DESTDIR/usr/lib:$DESTDIR/lib"
>- export 
> PATH="$DESTDIR/usr/sbin:$DESTDIR/usr/bin:$DESTDIR/sbin:$DESTDIR/bin:$PATH"
> -  - export PYTHONPATH="$DESTDIR$PYSITEDIR"
> +  - export PYTHONPATH="$DESTDIR$($PYTHON -c "from distutils.sysconfig import 
> *;print(get_python_lib(prefix='/usr'))")"
>- export RUBYLIB="$DESTDIR/$($RUBY -e 'puts 
> RbConfig::CONFIG["vendorlibdir"]'):$DESTDIR/$($RUBY -e 'puts 
> RbConfig::CONFIG["vendorarchdir"]')"
>  
># Show variables (to help debugging issues)
> diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> index 18588da586bf..8af04aab0ec2 100644
> --- a/libselinux/src/Makefile
> +++ b/libselinux/src/Makefile
> @@ -14,7 +14,7 @@ SHLIBDIR ?= /lib
>  INCLUDEDIR ?= $(PREFIX)/include
>  PYINC ?= $(shell $(PKG_CONFIG) --cflags $(PYPREFIX))
>  PYLIBS ?= $(shell $(PKG_CONFIG) --libs $(PYPREFIX))
> -PYSITEDIR ?= $(shell $(PYTHON) -c 'import site; 
> print(site.getsitepackages()[0])')
> +PYTHONLIBDIR ?= $(shell $(PYTHON) -c "from distutils.sysconfig import *; 
> print(get_python_lib(plat_specific=1, prefix='$(PREFIX)'))")
>  PYCEXT ?= $(shell $(PYTHON) -c 'import imp;print([s for s,m,t in 
> imp.get_suffixes() if t == imp.C_EXTENSION][0])')
>  RUBYINC ?= $(shell $(RUBY) -e 'puts "-I" + 
> RbConfig::CONFIG["rubyarchhdrdir"] + " -I" + RbConfig::CONFIG["rubyhdrdir"]')
>  RUBYLIBS ?= $(shell $(RUBY) -e 'puts "-L" + RbConfig::CONFIG["libdir"] + " 
> -L" + RbConfig::CONFIG["archlibdir"] + " " + 
> RbConfig::CONFIG["LIBRUBYARG_SHARED"]')
> @@ -191,10 +191,10 @@ install: all
>   ln -sf --relative $(DESTDIR)$(SHLIBDIR)/$(LIBSO) 
> $(DESTDIR)$(LIBDIR)/$(TARGET)
>  
>  install-pywrap: pywrap
> - test -d $(DESTDIR)$(PYSITEDIR)/selinux || install -m 755 -d 
> $(DESTDIR)$(PYSITEDIR)/selinux
> - install -m 755 $(SWIGSO) $(DESTDIR)$(PYSITEDIR)/_selinux$(PYCEXT)
> - install -m 755 $(AUDIT2WHYSO) 
> $(DESTDIR)$(PYSITEDIR)/selinux/audit2why$(PYCEXT)
> - install -m 644 $(SWIGPYOUT) $(DESTDIR)$(PYSITEDIR)/selinux/__init__.py
> + test -d $(DESTDIR)$(PYTHONLIBDIR)/selinux || install -m 755 -d 
> $(DESTDIR)$(PYTHONLIBDIR)/selinux
> + install -m 755 $(SWIGSO) $(DESTDIR)$(PYTHONLIBDIR)/_selinux$(PYCEXT)
> + install -m 755 $(AUDIT2WHYSO) 
> $(DESTDIR)$(PYTHONLIBDIR)/selinux/audit2why$(PYCEXT)
> + install -m 644 $(SWIGPYOUT) 
> $(DESTDIR)$(PYTHONLIBDIR)/selinux/__init__.py
>  
>  install-rubywrap: rubywrap
>   test -d $(DESTDIR)$(RUBYINSTALL) || install -m 755 -d 
> 

Re: [PATCH 1/2] selinux: wrap selinuxfs state

[Stephen Smalley]

On Tue, Mar 13, 2018, 12:18 PM Paul Moore  wrote:

> On Mon, Mar 5, 2018 at 11:47 AM, Stephen Smalley 
> wrote:
> > Move global selinuxfs state to a per-instance structure
> (selinux_fs_info),
> > and include a pointer to the selinux_state in this structure.
> > Pass this selinux_state to all security server operations, thereby
> > ensuring that each selinuxfs instance presents a view of and acts
> > as an interface to a particular selinux_state instance.
> >
> > This change should have no effect on SELinux behavior or APIs
> > (userspace or LSM).  It merely wraps the selinuxfs global state,
> > links it to a particular selinux_state (currently always the single
> > global selinux_state) and uses that state for all operations.
> >
> > Signed-off-by: Stephen Smalley 
> > ---
> >  security/selinux/selinuxfs.c   | 472
> +
> >  security/selinux/ss/services.c |  13 ++
> >  2 files changed, 307 insertions(+), 178 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 0dbd5fd6a396..1a32e93ba7b9 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -94,11 +122,13 @@ static unsigned long sel_last_ino = SEL_INO_NEXT -
> 1;
> >  static ssize_t sel_read_enforce(struct file *filp, char __user *buf,
> > size_t count, loff_t *ppos)
> >  {
> > +   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
> > +   struct selinux_state *state = fsi->state;
> > char tmpbuf[TMPBUFLEN];
> > ssize_t length;
> >
> > length = scnprintf(tmpbuf, TMPBUFLEN, "%d",
> > -  enforcing_enabled(_state));
> > +  enforcing_enabled(state));
>
> Since we only use state once, it seems like we could just use
> fsi->state without problem.
>
> > @@ -186,7 +218,9 @@ static const struct file_operations
> sel_handle_unknown_ops = {
> >
> >  static int sel_open_handle_status(struct inode *inode, struct file
> *filp)
> >  {
> > -   struct page*status =
> selinux_kernel_status_page(_state);
> > +   struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
> > +   struct selinux_state *state = fsi->state;
> > +   struct page*status = selinux_kernel_status_page(state);
>
> Once again, why do we need state instead of fsi->state?
>
> > if (!status)
> > return -ENOMEM;
> > @@ -242,6 +276,8 @@ static ssize_t sel_write_disable(struct file *file,
> const char __user *buf,
> >  size_t count, loff_t *ppos)
> >
> >  {
> > +   struct selinux_fs_info *fsi = file_inode(file)->i_sb->s_fs_info;
> > +   struct selinux_state *state = fsi->state;
> > char *page;
> > ssize_t length;
> > int new_value;
> > @@ -262,7 +298,7 @@ static ssize_t sel_write_disable(struct file *file,
> const char __user *buf,
> > goto out;
> >
> > if (new_value) {
> > -   length = selinux_disable(_state);
> > +   length = selinux_disable(state);
>
> Same as above.
>
> I think if we end up having to reference it more than once, go ahead
> and add another variable to the stack, otherwise just stick with
> fsi->state.  Go ahead and apply this logic to the rest of this patch.
>
> > @@ -1808,8 +1872,11 @@ static struct dentry *sel_make_dir(struct dentry
> *dir, const char *name,
> > return dentry;
> >  }
> >
> > +#define NULL_FILE_NAME "null"
> > +
> >  static int sel_fill_super(struct super_block *sb, void *data, int
> silent)
> >  {
> > +   struct selinux_fs_info *fsi;
> > int ret;
> > struct dentry *dentry;
> > struct inode *inode;
> > @@ -1837,14 +1904,20 @@ static int sel_fill_super(struct super_block
> *sb, void *data, int silent)
> > S_IWUGO},
> > /* last one */ {""}
> > };
> > +
> > +   ret = selinux_fs_info_create(sb);
> > +   if (ret)
> > +   goto err;
> > +
> > ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
> > if (ret)
> > goto err;
> >
> > -   bool_dir = sel_make_dir(sb->s_root, BOOL_DIR_NAME,
> _last_ino);
> > -   if (IS_ERR(bool_dir)) {
> > -   ret = PTR_ERR(bool_dir);
> > -   bool_dir = NULL;
> > +   fsi = sb->s_fs_info;
> > +   fsi->bool_dir = sel_make_dir(sb->s_root, BOOL_DIR_NAME,
> >last_ino);
> > +   if (IS_ERR(fsi->bool_dir)) {
> > +   ret = PTR_ERR(fsi->bool_dir);
> > +   fsi->bool_dir = NULL;
> > goto err;
> > }
> >
> > @@ -1858,7 +1931,7 @@ static int sel_fill_super(struct super_block *sb,
> void *data, int silent)
> > if (!inode)
> > goto err;
> >
> > -   inode->i_ino = ++sel_last_ino;
> > +   inode->i_ino =