OwlCyberDefense / setools

setools has moved to https://github.com/SELinuxProject/setools
Other
91 stars 34 forks source link

Ftrules hashtab #164

Closed karlwmacmillan closed 7 years ago

karlwmacmillan commented 7 years ago

The current setools does not compile against the current libsepol because the filename transition rules and range transition rules are now stored in hash tables rather than simple linked lists.

This pull request fixes that.

Unfortunately it is lightly tested because:

  1. The current range_trans searching seems broken (e.g., sesearch --range_trans fails in my version AND a version without my changes).

  2. I see no way in either apol or sesearch to search for filename transitions rules. Perhaps I am missing it?

karlwmacmillan commented 7 years ago

The continuous integration checks are failing because the compilation is against an older version of libsepol.

pebenito commented 7 years ago

I'm seeing compile errors using git SELinux userspace: https://travis-ci.org/pebenito/setools/jobs/193243427#L2059

My intention is not to expose the fact that filename transitions have their own hash table separate from other TE rules beyond the low-level interface (qpol). There is currently no support for searching for filename transition rules by the filename.

karlwmacmillan commented 7 years ago

On Jan 18, 2017, at 7:54 PM, Chris PeBenito notifications@github.com wrote:

I'm seeing compile errors using git SELinux userspace: https://travis-ci.org/pebenito/setools/jobs/193243427#L2059

Looks like it's because I'm using some c99 features like declarations in for loops. Would you take a patch to turn on c99 for gcc? It's 2017 after all :)

I missed this because I was using clang.

My intention is that the implementation detail that filename transitions have their own hash table separate from other TE rules exposed beyond the low-level interface (qpol). There is currently no support for searching for filename transition rules by the filename.

These changes don't change external interfaces at all. Just makes this compile with current libsepol.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

karlwmacmillan commented 7 years ago

This is compiling for me on Fedora 25 against head of libsepol (cdc653a447a807298d158c22759f76c2892d9008).

pebenito commented 7 years ago

Unit tests are failing with a segfault. The CI output isn't useful (https://travis-ci.org/pebenito/setools/jobs/193532614#L2552) but I was able to reproduce on my Gentoo system:

test_nodecon_invalid_range (tests.policyrep.selinuxpolicy.SELinuxPolicyLoadError)
SELinuxPolicy: invalid nodecon range (category not associated) error. ... (unknown source)::ERROR 'category elsewhere can not be associated with level low_s' at token '' on line 134:
nodecon ::1 ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff system:object_r:system:low_s:elsewhere

The "category can not be associated" error above is expected.ok
test_user_level_not_in_range (tests.policyrep.selinuxpolicy.SELinuxPolicyLoadError)
SELinuxPolicy: error for user's default level isn't within the range. ... expected failure

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff26bcc68 in copy_and_expand_avrule_block () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
(gdb) bt
#0  0x00007ffff26bcc68 in copy_and_expand_avrule_block () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
#1  0x00007ffff26bdee9 in expand_module_avrules () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
#2  0x00007ffff268d573 in qpol_expand_module () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
#3  0x00007ffff2699f88 in qpol_policy_open_from_file () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
#4  0x00007ffff265c132 in new_qpol_policy () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
#5  0x00007ffff2663e99 in _wrap_new_qpol_policy_t () from /home/pebenito/projects/setools/setools/policyrep/_qpol.cpython-34m.so
#6  0x00007ffff7a5d77e in PyEval_EvalFrameEx () from /usr/lib64/libpython3.4m.so.1.0
#7  0x00007ffff7a5e1af in PyEval_EvalCodeEx () from /usr/lib64/libpython3.4m.so.1.0
#8  0x00007ffff79cece2 in function_call () from /usr/lib64/libpython3.4m.so.1.0

The backtrace goes further, but its all Python frames. Unfortunately I'm not sure which unit test is failing, as it looks like it just finished the SELinuxPolicyLoadError test suite. It might be SELinuxPolicyTest which is in the same file (tests/policyrep/selinuxpolicy.py) and does have a setUpClass method.

karlwmacmillan commented 7 years ago

Ok - this was a complete and total pain to track down. The problem is the error message in expand_filename_trans in libsepol (expand.c ~1448). It's referencing otype in the filename_trans_datum_t. Well in policy_define.c copied over from checkpolicy otype is never set (it appears that the hashtable is only used for duplicate checking so there is no reason to set it). But that meant it was zero, and then one was subtracted to get the type name, and segfault.

I've just made the otype get set in policy_define.c and all of the tests pass.

Now - I don't understand all of this code well enough to know whether this change also needs to be pushed to checkpolicy. Your guidance there would be helpful.

pebenito commented 7 years ago

policy_define.c is a copy from checkpolicy (with a few very very minor additions). A quick diff shows this relevant change upstream:

@@ -3348,6 +3375,7 @@ int define_filename_trans(void)
                    yyerror("out of memory");
                    goto bad;
                }
+               ftdatum->otype = otype;
                rc = hashtab_insert(policydbp->filename_trans,
                            (hashtab_key_t)ft,
                            ftdatum);

So your change is correct (and already upstream).

karlwmacmillan commented 7 years ago

It fails for clang with the casts from hashtab_key_t casting.

On Jan 21, 2017, at 2:59 PM, Chris PeBenito notifications@github.com wrote:

@pebenito commented on this pull request.

In setup.py:

@@ -147,7 +147,6 @@ def run(self): include_dirs=include_dirs, extra_compile_args=['-Werror', '-Wextra', '-Waggregate-return',

  • '-Wcast-align', Why is this being removed? I readded it locally and it works fine for me.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

karlwmacmillan commented 7 years ago

Thanks Chris!