init: optionally load the system SELinux policy

[WavyEbuilder]

Implements #399 . Currently a draft PR for some of the reasons noted in that issue. Another thing to add:

• I saw mentioned here, https://github.com/davmac314/dinit/issues/240#issuecomment-1963096377, that you don't want to be falling back to C interfaces. For now, I'm just calling fprintf if the SELinux policy fails to load (as /dev/console is likely unable to be accessed at that point). Would you prefer std::cout to be used for now in this case?

[WavyEbuilder]

Thanks for the review, I will get this sorted soon.

[WavyEbuilder]

The documentation is in general pretty light. Is there anything more we need to say about SELinux either in the dinit man page or in a separate readme file? Eg: does the SELinux support require anything - does it need special filesystems (/proc and/or /sys) to be mounted before dinit starts, for example?

As for the special filesystems, selinux_init_load_policy(3) handles this all automatically. For example, taking a snippet from it's function body:

    /* Check for an override of the mode via the kernel command line. */
    rc = mount("proc", "/proc", "proc", 0, 0);

Including mounting what it needs (i.e. /sys/fs/selinux isn't needed to be mounted either). Everything else (such as the enforcing=0 cmdline parameter) is outside of the scope of init's documentation imo, as that is init system agnostic and documented elsewhere in SELinux's documentation. I suspect future things I want to work on will make some selinux specific documentation nice for them, but for this commit, it's basically just works outside of the --disable-selinux-policy flag.

[WavyEbuilder]

Done, ready for review. Just a quick note, this comment:

//   exe - the path that we are invoked with (to calculate our new security context to transition into.)

extends the line wrapping to 105, and I saw you mentioned it was okay up until 110 chars if it means it can be fit on one line. I'm not sure how that applies here as that is part of a bigger comment block, but I think this should be kept all on one line? Let me know if you like me to change this.

[davmac314]

As for the special filesystems, selinux_init_load_policy(3) handles this all automatically. For example, taking a snippet from it's function body: [...] Including mounting what it needs (i.e. /sys/fs/selinux isn't needed to be mounted either).

Holy mackerel. Dinit does not, and will not, do anything even halfway as disruptive to the system as mounting actual filesystems, completely behind the user's back.

On the selinux_transition function, I said:

I would like a reasonably extensive comment on this explaining what it does

I certainly would expect "mounts /proc and /sys" to be part of that comment if those are indeed being mounted by that function. And it must be documented that that is what happens, in the dinit man page.

The documentation is important, and so is the philosophy of the software being transparent about what it does.

[davmac314]

Done, ready for review. Just a quick note, this comment:

//   exe - the path that we are invoked with (to calculate our new security context to transition into.)

extends the line wrapping to 105, and I saw you mentioned it was okay up until 110 chars if it means it can be fit on one line. I'm not sure how that applies here as that is part of a bigger comment block, but I think this should be kept all on one line? Let me know if you like me to change this.

What I said was: "may extend to 110 in cases where that means the entire comment will fit on a single line". Since this is a small part of the comment, and not the entire comment, then no, it should be maximum 100 characters and should wrap before going beyond that.

Edit: and, since we're mentioning that comment, can you please put it in the format required by CODE-STYLE? No blank lines between short description and "Parameters:" / "Returns:", longer description goes after the "Returns".

[WavyEbuilder]

As for the special filesystems, selinux_init_load_policy(3) handles this all automatically. For example, taking a snippet from it's function body: [...] Including mounting what it needs (i.e. /sys/fs/selinux isn't needed to be mounted either).

Holy mackerel. Dinit does not, and will not, do anything even halfway as disruptive to the system as mounting actual filesystems, completely behind the user's back.

On the selinux_transition function, I said:

I would like a reasonably extensive comment on this explaining what it does

I certainly would expect "mounts /proc and /sys" to be part of that comment if those are indeed being mounted by that function. And it must be documented that that is what happens, in the dinit man page.

The documentation is important, and so is the philosophy of the software being transparent about what it does.

Sorry about that, I was having a bit of tunnelvision on anything the user would be expected to setup. Noted, I'll make sure to update the documentation to reflect that. Apologies

[WavyEbuilder]

Done, ready for review. Just a quick note, this comment:

//   exe - the path that we are invoked with (to calculate our new security context to transition into.)

extends the line wrapping to 105, and I saw you mentioned it was okay up until 110 chars if it means it can be fit on one line. I'm not sure how that applies here as that is part of a bigger comment block, but I think this should be kept all on one line? Let me know if you like me to change this.

What I said was: "may extend to 110 in cases where that means the entire comment will fit on a single line". Since this is a small part of the comment, and not the entire comment, then no, it should be maximum 100 characters and should wrap before going beyond that.

Edit: and, since we're mentioning that comment, can you please put it in the format required by CODE-STYLE? No blank lines between short description and "Parameters:" / "Returns:", longer description goes after the "Returns".

Got it, will fix

[WavyEbuilder]

What I said was: "may extend to 110 in cases where that means the entire comment will fit on a single line". Since this is a small part of the comment, and not the entire comment, then no, it should be maximum 100 characters and should wrap before going beyond that.

Edit: and, since we're mentioning that comment, can you please put it in the format required by CODE-STYLE? No blank lines between short description and "Parameters:" / "Returns:", longer description goes after the "Returns".

I've reread CODE-STYLE and I think that comment should be sorted now. Please let me know if it isn't as desired

[davmac314]

I've done a little more investigation of my own. It looks like, while selinux_init_load_policy does indeed mount proc (1) and mount sysfs (2), it actually unmounts proc (3) before returning.

I earlier asked:

does the SELinux support require anything - does it need special filesystems (/proc and/or /sys) to be mounted before dinit starts, for example?

And you answered:

As for the special filesystems, selinux_init_load_policy(3) handles this all automatically.

However, it looks to me like that is only half the story, since it unmounts /proc. But the other functions that have been used, such as getcon_raw, do not work without proc mounted. They error out with: No such file or directory. For getcon_raw that is because it calls getprocattrcon_raw (4) which calls openattr (5) which tries to open files in `/proc' (6).

I tested this to be certain. I definitely see that error when getcon_raw is called, if /proc has not been mounted before dinit is started.

It looks a lot like you gave me incorrect information. Have I missed something?

[WavyEbuilder]

Ok, but: you need to slow down and read my questions/comments properly, and answer them comprehensively. I'm spending far more time on this that I would like, and it seems like it's mostly because I have to repeatedly ask questions until I get a straight answer.

Noted. Apologies once again, I sincerly appreciate all the time and patience you have given me, I won't let this happen again.

Where does this information come from? (I'm having trouble even finding the source for getcon_raw).

This is from a local test. In setest.c:

#include <selinux/selinux.h>
#include <stdio.h>
#include <string.h>

int
main(void)
{
  char *context = NULL;
  if (getcon_raw(&context) < 0) {
    perror("getcon_raw");
    return 1;
  }
  printf("context: %s\n", context);
  return 0;
}

Compiling and running without my system policy loaded, I get:

$ cc -l selinux setest.c -o setest
$ ./setest
context: kernel

This makes sense all things considered, as the context of currently running proccesses are available without the policy loaded. For example, on a system without the policy loaded:

$ cat /proc/1/attr/current && printf '\n'
kernel

Running on a system with the policy loaded, this is the output I get:

$ ./setest
context: sys.id:sys.role:sys.subj:s0

(which is the context of the setest)

So it doesn't fail, but just returns the kernel context, which is expected.

I then made a new setest.c as follows:

#include <selinux/selinux.h>
#include <stdio.h>
#include <string.h>

int
main(int argc, char **argv)
{
  char *current_context = NULL;
  char *file_context = NULL;
  int security_class = 0;
  char *new_context = NULL;

  if (getcon_raw(&current_context) < 0) {
    perror("getcon_raw");
    return 1;
  }
  if (getfilecon_raw(argv[0], &file_context) < 0) {
    perror("getfilecon_raw");
    return 1;
  }
  security_class = string_to_security_class("process");
  if (security_class == 0) {
    fprintf(stderr, "error: string_to_security_class failed\n");
    return 1;
  }
  if (security_compute_create_raw(current_context, file_context, security_class, &new_context) < 0) {
    perror("security_compute_create_raw");
    return 1;
  }

  printf("context: %s\n", new_context);
  return 0;
}

Running without the policy loaded, the following occurs:

$ cc -l selinux setest.c -o setest
$ ./setest
error: string_to_security_class failed

Compared to when the policy is loaded:

$ cc -l selinux setest.c -o setest
$ ./setest
context: sys.id:sys.role:sys.subj:s0

Which succeeds as expected.

From the security_class_to_string(3) man page:

string_to_security_class() returns the class value corresponding to the
string name name, or zero if no such class exists.

This (expectly) returns 0 when the policy isn't loaded, as the class process (https://selinuxproject.org/page/ObjectClassesPerms#process) doesn't exist as the policy isn't loaded.

I hope that is detailed enough and makes sense. Let me know if anything isn't clear.

However, an early return of true would break the contract described in the function comment: So that might need to be updated, or expanded on in the descriptive part of the comment.

I'll make sure to do that.

It looks a lot like you gave me incorrect information.

I will be honest, yes - this seems like an oversight on my part, I missed that it unmounted /proc. I am interested in why I didn't catch this earlier though, I did actually test this locally and it seemed to be fine. I'm guessing something else was mounting /proc, though I did tell my initramfs not to.

I think it might be worth me spending some time writing some unit/igr tests for this pr to catch these sort of cases. I'll also make sure to detail how I locally test things (like above) in future to ensure this doesn't happen again.

(I'm having trouble even finding the source for getcon_raw).

They should be here: https://github.com/SELinuxProject/selinux/blob/main/libselinux/include/selinux/selinux.h#L33 https://github.com/SELinuxProject/selinux/blob/main/libselinux/src/procattr.c#L255

[davmac314]

Ok, mistakes get made. I think key takeaway here is that this PR is not yet ready, it needs some additional testing and some additional documentation. Please take whatever time you need and "at" me when ready (but I will personally be giving the PR a rest regardless for a couple of weeks at least, so take your time). Before pinging me please remember to double check everything, "self review" and make sure previous points raised are addressed, code style is according to requirements, comments and documentation are complete and accurate; and, specify (via PR comments) what testing has been done. Thanks.

Note:

I think it might be worth me spending some time writing some unit/igr tests for this pr to catch these sort of cases. I'll also make sure to detail how I locally test things (like above) in future to ensure this doesn't happen again.

I think it is going to be hard to write tests for this. Local testing may be the only viable option. That's ok as long as you test thoroughly.

[WavyEbuilder]

Just want to add something else after some more research.

        r = getcon_raw(&con);
        /* getcon_raw can return 0, and still give us a NULL pointer if /proc/self/attr/current is
         * empty. SELinux guarantees this won't happen, but that file isn't specific to SELinux, and may be
         * provided by some other arbitrary LSM with different semantics. */
        if (r == 0 && con) {
                initialized = !streq(con, "kernel");
                freecon(con);
        } else
                initialized = false;

This check likely applies to us too. I think it'd make sense to check for this. While this doesn't directly affect us, it'd make the overall system a bit more robust and ensures we play well if built with SELinux but another LSM is loaded, as mentioned in the comment. Everything else so far seems to match up with what systemd is doing.

I'll wait for any feedback before rushing to push code. Once that is done, logic wise we should be pretty close to being solid I think.

Edit: just seen the above message.

I think key takeaway here is that this PR is not yet ready, it needs some additional testing and some additional documentation. Please take whatever time you need and "at" me when ready (but I will personally be giving the PR a rest regardless for a couple of weeks at least, so take your time). Before pinging me please remember to double check everything, "self review" and make sure previous points raised are addressed, code style is according to requirements, comments and documentation are complete and accurate; and, specify (via PR comments) what testing has been done. Thanks.

Absolutely. I've noted them all down and I'll make sure to.

I think it is going to be hard to write tests for this. Local testing may be the only viable option. That's ok as long as you test thoroughly.

I'll document all my testing I do locally here then for now.

I'm also going to take 1-3 days of a break from a PR just to regroup myself a bit. To be completely honest, I've had a bit of anxiety responding, but you've been fantastic to work with, and your patience has helped a lot. A huge thanks for that.

After that, I'll get too all the changes you mentioned and work on some documentation for it :)

Thanks again

[davmac314]

I'll wait for any feedback before rushing to push code.

As I said, take your time, get it ready and get it right (as much as you can). As far as I'm concerned the review is over, please make whatever changes are needed to get it into shape and ping me then, and we'll start fresh.

[WavyEbuilder]

Hi Davin,

This message isn't a ping (intentionally) as I still am yet to write a bit more documentation, but a small status update.

Just made a few commits. I've added a comment above the check for current_context being a nullptr to explain why, as that seems a bit strange at first glance, so I think it'd be valuable to add for anyone reading the code.

I've clarified a little more about the kernel context to be more accurate in the selinux_transition comment. I haven't updated that comment regarding the early return if selinux_init_load_policy(3) fails in permissive, as while I am confident in the early return, I'm a little on the fence about what the behavior of dinit should be in permissive if it fails. I think as this is behavioral choice with neither option necessarily being right or wrong, I should lay out some points for and against it and let you as the maintainer decide what you want to do.

For early returning true (and continuing the boot if selinux_init_load_policy(3) fails in permissive mode):

• Would allow users to prod about to investigate why the loading of the policy failed
• Since permissive was requested its not enforcing anything anyway, so it isn't a huge deal in that regard

For early returning false (and halting the boot if selinux_init_load_policy(3) fails in permissive mode):

• In the same way a user should expect something to not enforce actions but just log actions in permissive, if they boot in permissive mode, they are expecting that the SELinux policy is loaded but actions are not enforced. The policy failing to load in permissive is still deviating from that expectation.
• It is common advice in the SELinux community to avoid booting with the selinux=0 kernel cmdline flag, which tells the kernel to not load any SELinux-related components. This is because it can cause files that are modified, and new files that are created to be created with an invalid or non existent context (as SELinux isn't enabled to label said objects). In our case where selinux_init_load_policy(3) has failed in permissive, while the kernel related components of SELinux are enabled, without the policy loaded, said objects will not be labeled correctly. This breaks another expectation of the user, i.e. that as they are booted in permissive mode, objects will be labeled correctly according to the requested policy (that should be loaded).

As for what systemd does, it continues the boot in permissive mode and logs a warning. The code for that can be seen here https://github.com/systemd/systemd/blob/main/src/core/selinux-setup.c#L90-L102.

[davmac314]

I think as this is behavioral choice with neither option necessarily being right or wrong, I should lay out some points for and against it and let you as the maintainer decide what you want to do.

This doesn't seem like a decision that has any sort of major impact on the PR so: make a decision yourself, and justify it. From my point of view I want as little work to do, until the PR is ready, as possible. Thanks.

(No need to give status updates either. Just take whatever time you need, finish the work, do it right, do it well).