Re: [PATCH 0/3] Update restorecon to support new digest scheme

[Stephen Smalley <sds@tycho.nsa.gov>]

On 5/30/19 12:22 PM, Richard Haines wrote:
> On Tue, 2019-05-28 at 10:24 -0400, Stephen Smalley wrote:
>> On 5/24/19 2:06 PM, Richard Haines wrote:
>>> On Fri, 2019-05-24 at 13:11 -0400, Stephen Smalley wrote:
>>>> On 5/22/19 12:42 PM, Richard Haines wrote:
>>>>> These patches require [1] and [2] be installed first. They have
>>>>> been implemented on Android and sent to the selinux list,
>>>>> however
>>>>> their
>>>>> merge has been deferred. They will install the core hashing of
>>>>> file_context entries and fix root stem processing.
>>>>>
>>>>> Patch 1/3 updates selinux_restorecon() replacing the per-
>>>>> mountpoint
>>>>> security.restorecon_last attribute with a per-directory
>>>>> security.sehash
>>>>> attribute computed from only those file contexts entries that
>>>>> partially
>>>>> match the directory. This is to avoid the need to walk the
>>>>> entire
>>>>> tree
>>>>> when any part of file_contexts changes, limiting relabels to
>>>>> only
>>>>> those
>>>>> parts of the tree that could have changed.
>>>>>
>>>>> One change is to add a new
>>>>> selabel_get_digests_all_partial_matches(3)
>>>>> function that is explained in the man page. This could replace
>>>>> the
>>>>> Android
>>>>> version of selabel_hash_all_partial_matches(3), that could then
>>>>> be
>>>>> converted into a local function (The Android team would need to
>>>>> approve).
>>>>
>>>> Has Android sorted out all of the ramifications of this change?
>>> As I'm not involved with Android I don't know. Note that as I do
>>> not
>>> change their core selabel code my new function could be added to
>>> libselinux and they pick it up if they want it (or it gets rejected
>>> by
>>> the SELinux team). I've added Nick to the cc list in case any
>>> comments.
>>>
>>> The main reason I added the function was the call retrieved the
>>> info in
>>> one go plus you did't need to know the digest length.
>>>
>>>>     What
>>>> about the triggering of CAP_SYS_ADMIN denials for setting the
>>>> security.sehash attribute?
>>> They solved this by adding the *_RESTORECON_SKIP_SEHASH flag that
>>> I've
>>> copied (the selinux-testsuite restorecon has tests for this).
>>
>> Per
>> https://android-review.googlesource.com/c/platform/external/selinux/+/940326,
>> they say:
>> "TODO: It would be better if the default for restorecon was to
>> suppress
>> the hash computation, since otherwise it encourages programs to be
>> overprivileged with CAP_SYS_ADMIN. I'll plan on doing that in a
>> followup
>> commit."
>>
>> So I think we would want to disable the setting of sehash by default
>> upstream to match their planned future behavior. In fact, I think
>> that
>> is how your version of setting security.restoreconlast worked
>> upstream
>> originally - the caller had to selabel_open() with options containing
>> a
>> SELABEL_OPT_DIGEST option with a non-zero value in order to enable
>> setting of a digest in the first place?  By the way, this patch set
>> seems to dispense with SELABEL_OPT_DIGEST and selabel_digest() usage
>> internally even though it leaves them defined, thereby changing
>> behavior
>> for existing API callers?
> I'll fix this in the next set of patches to follow the original idea of
> calling selabel_open to determine the behavior. However I see Android
> have abandoned these changes (I could not see the reason why).
> 
> I'm happy to continue if you think worthwhile.

I think they abandoned changes to revert these changes, i.e. they are 
actually keeping them.  I assume this means that they were going to 
revert them due to some bug (not accessible to us) but that they 
resolved that bug in some other way.

> 
> 
> [1]
> https://android-review.googlesource.com/c/platform/external/selinux/+/932005
> 
>>
>>>>> Patches 2/3 and 3/3 update restorecon, setfiles and
>>>>> restorecond.
>>>>>
>>>>> I will send a patch for the selinux-testsuite that will perform
>>>>> tests on
>>>>> the new code.
>>>>>     
>>>>> [1]
>>>>> https://lore.kernel.org/selinux/20190311222442.49824-1-xunchang@google.com/
>>>>> [2]
>>>>> https://lore.kernel.org/selinux/20190417180955.136942-1-xunchang@google.com/
>>>>>
>>>>> Richard Haines (3):
>>>>>      libselinux: Save digest of all partial matches for
>>>>> directory
>>>>>      setfiles: Update utilities for the new digest scheme
>>>>>      restorecond: Update to handle new digest scheme
>>>>>
>>>>>     libselinux/include/selinux/label.h            |   5 +
>>>>>     libselinux/include/selinux/restorecon.h       |  17 +-
>>>>>     .../selabel_get_digests_all_partial_matches.3 |  70 +++++
>>>>>     libselinux/man/man3/selinux_restorecon.3      |  91 +++---
>>>>>     .../man3/selinux_restorecon_default_handle.3  |   9 +-
>>>>>     .../man/man3/selinux_restorecon_xattr.3       |  11 +-
>>>>>     libselinux/src/label.c                        |  15 +
>>>>>     libselinux/src/label_file.c                   |  51 ++++
>>>>>     libselinux/src/label_file.h                   |   4 +
>>>>>     libselinux/src/label_internal.h               |   5 +
>>>>>     libselinux/src/selinux_restorecon.c           | 267
>>>>> +++++++++++
>>>>> -------
>>>>>     libselinux/utils/.gitignore                   |   1 +
>>>>>     .../selabel_get_digests_all_partial_matches.c | 170
>>>>> +++++++++++
>>>>>     policycoreutils/setfiles/restore.c            |   7 +-
>>>>>     policycoreutils/setfiles/restore.h            |   2 +-
>>>>>     policycoreutils/setfiles/restorecon.8         |  10 +-
>>>>>     policycoreutils/setfiles/restorecon_xattr.8   |  19 +-
>>>>>     policycoreutils/setfiles/restorecon_xattr.c   |  66 +----
>>>>>     policycoreutils/setfiles/setfiles.8           |  10 +-
>>>>>     policycoreutils/setfiles/setfiles.c           |  19 +-
>>>>>     restorecond/restore.c                         |   8 +-
>>>>>     restorecond/restore.h                         |   2 +-
>>>>>     restorecond/restorecond.c                     |   5 +-
>>>>>     23 files changed, 593 insertions(+), 271 deletions(-)
>>>>>     create mode 100644
>>>>> libselinux/man/man3/selabel_get_digests_all_partial_matches.3
>>>>>     create mode 100644
>>>>> libselinux/utils/selabel_get_digests_all_partial_matches.c
>>>>>
>