Re: [PATCH] setfiles: Do not abort on labeling error

[Petr Lautrbach <plautrba@redhat.com>]

Stephen Smalley <stephen.smalley.work@gmail.com> writes:

> On Wed, Jan 13, 2021 at 7:15 AM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Commit 602347c7422e ("policycoreutils: setfiles - Modify to use
>> selinux_restorecon") changed behavior of setfiles. Original
>> implementation skipped files which it couldn't set context to while the
>> new implementation aborts on them. setfiles should abort only if it
>> can't validate 10 contexts from spec_file.
>>
>> Reproducer:
>>
>>     # mkdir -p r/1 r/2 r/3
>>     # touch r/1/1 r/2/1
>>     # chattr +i r/2/1
>>     # touch r/3/1
>>     # setfiles -r r -v /etc/selinux/targeted/contexts/files/file_contexts r
>>     Relabeled r from unconfined_u:object_r:mnt_t:s0 to unconfined_u:object_r:root_t:s0
>>     Relabeled r/2 from unconfined_u:object_r:mnt_t:s0 to unconfined_u:object_r:default_t:s0
>>     setfiles: Could not set context for r/2/1:  Operation not permitted
>>
>> r/3 and r/1 are not relabeled.
>>
>> Also drop some old unused code in order to prevent future confusion.
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>  policycoreutils/setfiles/setfiles.c | 49 +----------------------------
>>  1 file changed, 1 insertion(+), 48 deletions(-)
>>
>> diff --git a/policycoreutils/setfiles/setfiles.c b/policycoreutils/setfiles/setfiles.c
>> index 422c3767b845..b96ee814bad2 100644
>> --- a/policycoreutils/setfiles/setfiles.c
>> +++ b/policycoreutils/setfiles/setfiles.c
>> @@ -23,14 +23,6 @@ static int nerr;
>>
>>  #define STAT_BLOCK_SIZE 1
>>
>> -/* setfiles will abort its operation after reaching the
>> - * following number of errors (e.g. invalid contexts),
>> - * unless it is used in "debug" mode (-d option).
>> - */
>> -#ifndef ABORT_ON_ERRORS
>> -#define ABORT_ON_ERRORS        10
>> -#endif
>> -
>>  #define SETFILES "setfiles"
>>  #define RESTORECON "restorecon"
>>  static int iamrestorecon;
>> @@ -56,15 +48,6 @@ static __attribute__((__noreturn__)) void usage(const char *const name)
>>         exit(-1);
>>  }
>>
>> -void inc_err(void)
>> -{
>> -       nerr++;
>> -       if (nerr > ABORT_ON_ERRORS - 1 && !r_opts.debug) {
>> -               fprintf(stderr, "Exiting after %d errors.\n", ABORT_ON_ERRORS);
>> -               exit(-1);
>> -       }
>> -}
>> -
>>  void set_rootpath(const char *arg)
>>  {
>>         if (strlen(arg) == 1 && strncmp(arg, "/", 1) == 0) {
>> @@ -82,27 +65,6 @@ void set_rootpath(const char *arg)
>>         }
>>  }
>>
>> -int canoncon(char **contextp)
>> -{
>> -       char *context = *contextp, *tmpcon;
>> -       int rc = 0;
>> -
>> -       if (policyfile) {
>> -               if (sepol_check_context(context) < 0) {
>> -                       fprintf(stderr, "invalid context %s\n", context);
>> -                       exit(-1);
>> -               }
>> -       } else if (security_canonicalize_context_raw(context, &tmpcon) == 0) {
>> -               free(context);
>> -               *contextp = tmpcon;
>> -       } else if (errno != ENOENT) {
>> -               rc = -1;
>> -               inc_err();
>> -       }
>> -
>> -       return rc;
>> -}
>> -
>>  #ifndef USE_AUDIT
>>  static void maybe_audit_mass_relabel(int mass_relabel __attribute__((unused)),
>>                                 int mass_relabel_errs __attribute__((unused)))
>> @@ -181,6 +143,7 @@ int main(int argc, char **argv)
>>         policyfile = NULL;
>>         nerr = 0;
>>
>> +       r_opts.abort_on_error = 0;
>>         r_opts.progname = strdup(argv[0]);
>>         if (!r_opts.progname) {
>>                 fprintf(stderr, "%s:  Out of memory!\n", argv[0]);
>> @@ -193,7 +156,6 @@ int main(int argc, char **argv)
>>                  * setfiles:
>>                  * Recursive descent,
>>                  * Does not expand paths via realpath,
>> -                * Aborts on errors during the file tree walk,
>>                  * Try to track inode associations for conflict detection,
>>                  * Does not follow mounts (sets SELINUX_RESTORECON_XDEV),
>>                  * Validates all file contexts at init time.
>> @@ -201,7 +163,6 @@ int main(int argc, char **argv)
>>                 iamrestorecon = 0;
>>                 r_opts.recurse = SELINUX_RESTORECON_RECURSE;
>>                 r_opts.userealpath = 0; /* SELINUX_RESTORECON_REALPATH */
>> -               r_opts.abort_on_error = SELINUX_RESTORECON_ABORT_ON_ERROR;
>>                 r_opts.add_assoc = SELINUX_RESTORECON_ADD_ASSOC;
>>                 /* FTS_PHYSICAL and FTS_NOCHDIR are always set by selinux_restorecon(3) */
>>                 r_opts.xdev = SELINUX_RESTORECON_XDEV;
>> @@ -225,7 +186,6 @@ int main(int argc, char **argv)
>>                 iamrestorecon = 1;
>>                 r_opts.recurse = 0;
>>                 r_opts.userealpath = SELINUX_RESTORECON_REALPATH;
>> -               r_opts.abort_on_error = 0;
>>                 r_opts.add_assoc = 0;
>>                 r_opts.xdev = 0;
>>                 r_opts.ignore_mounts = 0;
>> @@ -420,13 +380,6 @@ int main(int argc, char **argv)
>>                                 usage(argv[0]);
>>                 }
>>
>> -               /* Use our own invalid context checking function so that
>> -                * we can support either checking against the active policy or
>> -                * checking against a binary policy file.
>> -                */
>> -               cb.func_validate = canoncon;
>> -               selinux_set_callback(SELINUX_CB_VALIDATE, cb);
>> -
>
> I could be wrong but I think we still need this for setfiles -c.

Looks like you are right. I'll send updated version.