init: optionally load the system SELinux policy #400
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,12 @@ | |
|
|
||
| #include "mconfig.h" | ||
|
|
||
| #if SUPPORT_SELINUX | ||
| #include <selinux/avc.h> | ||
| #include <selinux/label.h> | ||
| #include <selinux/selinux.h> | ||
| #endif | ||
|
|
||
| /* | ||
| * When running as the system init process, Dinit processes the following signals: | ||
| * | ||
|
|
@@ -210,6 +216,10 @@ struct options { | |
|
|
||
| // list of services to start | ||
| std::list<const char *> services_to_start; | ||
|
|
||
| #ifdef SUPPORT_SELINUX | ||
| bool load_selinux_policy = true; | ||
| #endif | ||
| }; | ||
|
|
||
| // Process a command line argument (and possibly its follow-up value) | ||
|
|
@@ -365,6 +375,11 @@ static int process_commandline_arg(char **argv, int argc, int &i, options &opts) | |
| } | ||
| } | ||
| #endif | ||
| #ifdef SUPPORT_SELINUX | ||
| else if (strcmp(argv[i], "--disable-selinux-policy") == 0) { | ||
| opts.load_selinux_policy = false; | ||
| } | ||
| #endif | ||
| else if (strcmp(argv[i], "--service") == 0 || strcmp(argv[i], "-t") == 0) { | ||
| if (++i < argc && argv[i][0] != '\0') { | ||
| services_to_start.push_back(argv[i]); | ||
|
|
@@ -399,6 +414,9 @@ static int process_commandline_arg(char **argv, int argc, int &i, options &opts) | |
| " --cgroup-path <path>, -b <path>\n" | ||
| " cgroup base path (for resolving relative paths)\n" | ||
| #endif | ||
| #ifdef SUPPORT_SELINUX | ||
| " --disable-selinux-policy don't load the system SELinux policy\n" | ||
| #endif | ||
| " --log-file <file>, -l <file> log to the specified file\n" | ||
| " --quiet, -q disable output to standard output\n" | ||
| " <service-name>, --service <service-name>, -t <service-name>\n" | ||
|
|
@@ -453,6 +471,90 @@ static int process_commandline_arg(char **argv, int argc, int &i, options &opts) | |
| return 0; | ||
| } | ||
|
|
||
| #if SUPPORT_SELINUX | ||
| // Load the system SELinux policy and transition ourselves to it. When successful, | ||
|
There was a problem hiding this comment. Line width should extend to (just under) 100 characters, please see CODE-STYLE. This comment wraps at just over 80 characters. |
||
| // this will cause SELinux labels as per the policy to be attached to processes (and | ||
| // file descriptors owned by those processes). The SELinux framework will begin to | ||
| // enforce restrictions on access based on these labels and the loaded policy. | ||
| // | ||
| // We might lose access to any file descriptors we have open when this is called (since | ||
| // they will still be labelled with the kernel context), so it is best done early (i.e. | ||
| // before we start opening file descriptors). | ||
| // | ||
| // Parameters: | ||
| // exe - the path that we are invoked with (to calculate our new security | ||
| // context to tranition into.) | ||
WavyEbuilder marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // | ||
| // Returns: | ||
| // If we fail to load the system SELinux policy, return false, otherwise, | ||
| // return true. | ||
| static bool selinux_transition(const char *exe) { | ||
davmac314 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Let's use std::cerr instead of the log for logging messages here. | ||
| // If we output anything, we return failure, which indicates dinit should | ||
| // terminate before the log is initalised and flushed. | ||
davmac314 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| using std::cerr; | ||
| using std::endl; | ||
|
|
||
davmac314 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| char *current_context = nullptr; | ||
| char *file_context = nullptr; | ||
| security_class_t security_class; | ||
| char *new_context = nullptr; | ||
|
|
||
| if (is_selinux_enabled() == 1) { | ||
| return true; | ||
| } | ||
|
|
||
| int enforce = 0; | ||
| // We don't need to worry about the enforcing=0 kernel cmdline option or | ||
|
There was a problem hiding this comment. Please extend comments to the correct line width before wrapping them. See CODE-STYLE. |
||
| // parsing /etc/selinux/config, selinux_init_load_policy(3) will handle | ||
| // all cases for us. | ||
| if (selinux_init_load_policy(&enforce) != 0) { | ||
| if (enforce > 0) { | ||
| cerr << "Failed to load SELinux policy." << endl; | ||
| return false; | ||
| } | ||
|
There was a problem hiding this comment. So if the load fails ( There was a problem hiding this comment. For the
In the case of enforcing, the system is expected to boot in a secure state, i.e. with the system's SELinux policy enforcing. If we fail to load it, we should halt the boot as that is an untrusted state. In the case of permissive, it's generally better to be more lenient. In permissive mode, actions are expected to be logged, but any failures revolving around the policy aren't expected to have an impact on the system's operation. Other SELinux aware applications, such as Let me know your thoughts, I can add a comment explaining this too if desired There was a problem hiding this comment. That's not answering the question I asked. If the policy fails to load ( The comment says:
But it's not even returning at this point, it's just going on through to the following call to
That code checks whether loading the policy failed, and if so, prints an error message and retuns an exit code: That's different to what is here, which just continues as if no error had happened. There was a problem hiding this comment. And when I say "continues as if no error had happened", I mean that it goes on to try and set a new context ( There was a problem hiding this comment. Those won't fail as it'll just continue on with the kernel context. There was a problem hiding this comment.
I know that you have done some work on this and are probably trying hard, but in all honesty I am very close to pulling the plug. I need you to clearly answer the questions that I'm asking, not just push more code. The question I asked was:
I still don't have a satisfactory answer to that. I can't accept this PR without understanding what it is doing. There was a problem hiding this comment.
I decided to clarify this a bit further, so There was a problem hiding this comment.
Understood. So: This is accurate, i.e. that wasn't right. I don't think we should fail (as in exit) there, and I incorrectly gathered that it would just fall through (because it'll attempt to set the same context). I overlooked the fact There was a problem hiding this comment.
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.
"An early return of true" will currently fail the boot. So is that what is supposed to happen?
Where does this information come from? (I'm having trouble even finding the source for getcon_raw). There was a problem hiding this comment. Sorry: an early return of true won't fail the boot, I was thinking an early return of false. So that might need to be updated, or expanded on in the descriptive part of the comment. |
||
| } | ||
|
|
||
| bool ret = true; | ||
| if (getcon_raw(¤t_context) < 0) { | ||
| ret = false; | ||
| cerr << "Failed to get current context: " << strerror(errno) << endl; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| if (getfilecon_raw(exe, &file_context) < 0) { | ||
| ret = false; | ||
| cerr << "Failed to get file context for " << exe << ": " << strerror(errno) << endl; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| security_class = string_to_security_class("process"); | ||
| if (security_class == 0) { | ||
| ret = false; | ||
| cerr << "Failed to get security class for process" << endl; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| if (security_compute_create_raw(current_context, file_context, security_class, &new_context) < 0) { | ||
| ret = false; | ||
| cerr << "Failed to compute create context: " << strerror(errno) << endl; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| if (setcon_raw(new_context) < 0) { | ||
| ret = false; | ||
| cerr << "Failed to set transition context to " << new_context << ": " << strerror(errno) << endl; | ||
| goto cleanup; | ||
| } | ||
|
|
||
| cleanup: | ||
| if (current_context) freecon(current_context); | ||
| if (file_context) freecon(file_context); | ||
| if (new_context) freecon(new_context); | ||
| return ret; | ||
| } | ||
| #endif | ||
|
|
||
| // Main entry point | ||
| int dinit_main(int argc, char **argv) | ||
| { | ||
|
|
@@ -486,6 +588,18 @@ int dinit_main(int argc, char **argv) | |
| } | ||
| } | ||
|
|
||
| #if SUPPORT_SELINUX | ||
| // Error exit if we are PID 1 and fail to load the selinux policy and transition. | ||
| // | ||
| // This should be done directly after argument parsing, it's best to do this as early as possible to get | ||
|
There was a problem hiding this comment. This comment line, in contrast, is too long. And so are some of the others. Please check all comments and make sure the line widths are correct. Basically: they should be as close to (but less than) 100 characters as is possible, but may extend to 110 in cases where that means the entire comment will fit on a single line. |
||
| // init in the domain specified in the policy, and hence confine it, quickly. | ||
| // | ||
| // If selinux_transition fails, the system is not in the state requested by the user, and there is nothing | ||
| // we can do about it. Instead of continuing to boot the rest of the system without loading the user's policy, | ||
| // let's bail now to avoid an insecure and untrusted state. | ||
| if (am_system_mgr && am_system_init && opts.load_selinux_policy && !selinux_transition(argv[0])) return 1; | ||
davmac314 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| #endif | ||
|
|
||
| if (am_system_mgr) { | ||
| // setup STDIN, STDOUT, STDERR so that we can use them | ||
| int onefd = open("/dev/console", O_RDONLY, 0); | ||
|
|
@@ -1207,6 +1321,9 @@ static void printVersion() | |
| #endif | ||
| #if USE_INITGROUPS | ||
| " supplemental-groups" | ||
| #endif | ||
| #if SUPPORT_SELINUX | ||
| " selinux" | ||
| #endif | ||
| "\n"; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As was mentioned:
Now it is:
That is not specifying the default, it's just a statement of availability.
Also "avilable" has a typo.