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

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

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?

> 
>>
>>> 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
>>>
>