| Message ID | d204aaea-ca46-49c2-f7cd-6f20889cecbf@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | chcat: don't crash if access to binary policy is prohibited | expand |
On Sat, May 9, 2020 at 4:06 PM bauen1 <j2468h@googlemail.com> wrote: > > sobject will crash if access to the binary policy is prohibited by > selinux, e.g. refpolicy > this also breaks file operations that don't require seobject. > > Signed-off-by: bauen1 <j2468h@gmail.com> Hello, This patch looks very hackish. In fact, an underlying issue that exists with seobject is that "import seobject" raises an exception when it is used from an environment that is not allowed to read the policy: >>> import seobject Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python3.8/site-packages/seobject.py", line 33, in <module> import sepolicy File "/usr/lib/python3.8/site-packages/sepolicy/__init__.py", line 186, in <module> raise e File "/usr/lib/python3.8/site-packages/sepolicy/__init__.py", line 182, in <module> policy_file = get_installed_policy() File "/usr/lib/python3.8/site-packages/sepolicy/__init__.py", line 137, in get_installed_policy raise ValueError(_("No SELinux Policy installed")) ValueError: No SELinux Policy installed Is this the issue you encountered when you write "seobject will crash"? In my humble opinion, trying to hide such an issue by moving "import seobject" makes maintaining the project more difficult. I would prefer seeing a way to allow using "import seobject" without raising exceptions, but working on this is unfortunately quite time-consuming (I have not seen a straightforward way to deal with this, and there exist several ways to solve this in not-very-direct ways, for example with lazy loading of the policy when needed or with replacing some API with stub functions if the policy cannot be loaded). Therefore I will not ack this patch, but I will not block ("Nack") it if another maintainer wants to include it. Thanks, Nicolas > --- > python/chcat/chcat | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/python/chcat/chcat b/python/chcat/chcat > index fdd2e46e..55408577 100755 > --- a/python/chcat/chcat > +++ b/python/chcat/chcat > @@ -28,7 +28,6 @@ import os > import pwd > import getopt > import selinux > -import seobject > > PROGNAME = "policycoreutils" > try: > @@ -65,6 +64,7 @@ def verify_users(users): > > > def chcat_user_add(newcat, users): > + import seobject > errors = 0 > logins = seobject.loginRecords() > seusers = logins.get_all() > @@ -144,6 +144,7 @@ def chcat_add(orig, newcat, objects, login_ind): > > > def chcat_user_remove(newcat, users): > + import seobject > errors = 0 > logins = seobject.loginRecords() > seusers = logins.get_all() > @@ -233,6 +234,7 @@ def chcat_remove(orig, newcat, objects, login_ind): > > > def chcat_user_replace(newcat, users): > + import seobject > errors = 0 > logins = seobject.loginRecords() > seusers = logins.get_all() > @@ -376,6 +378,7 @@ def listcats(): > > > def listusercats(users): > + import seobject > if len(users) == 0: > try: > users.append(os.getlogin()) > -- > 2.26.2 >
On Sun, May 10, 2020 at 1:26 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Sat, May 9, 2020 at 4:06 PM bauen1 <j2468h@googlemail.com> wrote: > > > > sobject will crash if access to the binary policy is prohibited by > > selinux, e.g. refpolicy > > this also breaks file operations that don't require seobject. > > > > Signed-off-by: bauen1 <j2468h@gmail.com> > > Hello, > This patch looks very hackish. In fact, an underlying issue that > exists with seobject is that "import seobject" raises an exception > when it is used from an environment that is not allowed to read the > policy: > > >>> import seobject > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > File "/usr/lib/python3.8/site-packages/seobject.py", line 33, in <module> > import sepolicy > File "/usr/lib/python3.8/site-packages/sepolicy/__init__.py", line > 186, in <module> > raise e > File "/usr/lib/python3.8/site-packages/sepolicy/__init__.py", line > 182, in <module> > policy_file = get_installed_policy() > File "/usr/lib/python3.8/site-packages/sepolicy/__init__.py", line > 137, in get_installed_policy > raise ValueError(_("No SELinux Policy installed")) > ValueError: No SELinux Policy installed > > Is this the issue you encountered when you write "seobject will crash"? > > In my humble opinion, trying to hide such an issue by moving "import > seobject" makes maintaining the project more difficult. I would prefer > seeing a way to allow using "import seobject" without raising > exceptions, but working on this is unfortunately quite time-consuming > (I have not seen a straightforward way to deal with this, and there > exist several ways to solve this in not-very-direct ways, for example > with lazy loading of the policy when needed or with replacing some API > with stub functions if the policy cannot be loaded). > > Therefore I will not ack this patch, but I will not block ("Nack") it > if another maintainer wants to include it. I'm not opposed to the patch itself (I assume the current code breaks usage of chcat under MLS policy by regular users who lack access to the policy file), but your Signed-off-by line ought to be revised to contain your real name. Otherwise, it doesn't really serve its purpose. See the discussion of Signed-off-by in https://www.kernel.org/doc/html/latest/process/submitting-patches.html.
diff --git a/python/chcat/chcat b/python/chcat/chcat index fdd2e46e..55408577 100755 --- a/python/chcat/chcat +++ b/python/chcat/chcat @@ -28,7 +28,6 @@ import os import pwd import getopt import selinux -import seobject PROGNAME = "policycoreutils" try: @@ -65,6 +64,7 @@ def verify_users(users): def chcat_user_add(newcat, users): + import seobject errors = 0 logins = seobject.loginRecords() seusers = logins.get_all() @@ -144,6 +144,7 @@ def chcat_add(orig, newcat, objects, login_ind): def chcat_user_remove(newcat, users): + import seobject errors = 0 logins = seobject.loginRecords() seusers = logins.get_all() @@ -233,6 +234,7 @@ def chcat_remove(orig, newcat, objects, login_ind): def chcat_user_replace(newcat, users): + import seobject errors = 0 logins = seobject.loginRecords() seusers = logins.get_all() @@ -376,6 +378,7 @@ def listcats(): def listusercats(users): + import seobject if len(users) == 0: try: users.append(os.getlogin())
sobject will crash if access to the binary policy is prohibited by selinux, e.g. refpolicy this also breaks file operations that don't require seobject. Signed-off-by: bauen1 <j2468h@gmail.com> --- python/chcat/chcat | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)