Under current implementation, rc-update, runscript, rc-service, and start-stop-daemon are all symlinks to /sbin/rc. This causes selinux to end up transitioning to run_init_t domain, instead of initrc_t as it should. Reproducible: Always Steps to Reproduce: 1. Create selinux system 2. run rc-update 3. observe rc-update failing to work. Actual Results: rc-update fails with errors. AVCs are recorded in /var/log/avc.log indicating access violations due to rc-update being run in run_init_t domain, instead of transitioning to initrc_t domain as it should. Expected Results: rc-update and other utilities should work properly on selinux system. The problem arises because rc-update and other utilities are symlinks to rc, which changes behavior based on argv[0]. This causes selinux to get wrong context information for the utilities, and to run them in the wrong domain. To resolve this, selinux systems will need to replace the symlinks with wrapper scripts to call rc with a --applet argument, specifying the functionality to use, and rc itself will need to be modified to allow selection of applet based on the --applet argument as well as the argv[0] parameter.
"rc --applet $APPLET" is now implemented as of commit 4933952. I have not updated the ebuild to build the wrappers yet. Please provide examples of how to do them with the correct contexts in the ebuild.
that step should not be in the ebuild. i already provided examples on the openrc@g.o thread for how to generate the wrappers in the build system. also, your code has quite a number of problems. style wise, there should not be any trailing whitespace, you need spaces between "if" and "for" and the open paren, and a space with the simple math like "i-2". runtime wise, i'm pretty sure it segfaults when argc==0. or if someone runs `rc --applet`. and the "argv[argc-2] = NULL" is useless. this is fairly basic stuff man :P.
Created attachment 261047 [details] selinux patches for openrc 0.7.0 Adds --applet functionality to rc.c. Also modifies Makefile to create 'wrapper scripts' rather than symlinks when installed on selinux systems.
Created attachment 261049 [details, diff] apply selinux patch in ebuild Modify ebuild to add selinux USE flag (since makefile has to now modify system configuration based on whether selinux is enabled or disabled during installation). Also modify ebuild to apply selinux patch for rc.c and Makefile.
Comment on attachment 261047 [details] selinux patches for openrc 0.7.0 I'll be able to use the patch to the Makefile, but the code that adds --applet support to rc is already in git, see comment #1.
Comment on attachment 261049 [details, diff] apply selinux patch in ebuild If you would like to write a patch to modify an ebuild, please use the openrc-9999 ebuild, and just add the selinux use flag handling. I will probably just do a new openrc release with this functionality.
Comment on attachment 261047 [details] selinux patches for openrc 0.7.0 Your patch did not work correctly on my system. It only created the following symbolic links: /bin/binlinks /sbin/sbinlinks /lib/rc/bin/rc-binlinks /lib/rc/sbin/rc-sbinlinks It looks like the first argument isn't being passed to the make-links function correctly.
Created attachment 261071 [details, diff] patch for replacing openrc symlinks with wrapper scripts Well sniggelnorf. The patch I gave you was the wrong one; somehow I grabbed a patch that was about 3 generations old (which actually bugs me a bit). Here's the correct one. Also, I thought based on comment #2 that the patch in commit #1 was undesirable?
(In reply to comment #8) > Well sniggelnorf. The patch I gave you was the wrong one; somehow I grabbed a > patch that was about 3 generations old (which actually bugs me a bit). Here's > the correct one. Heh, ok, no problem. :) I looked over your patch as well as the documentation for gnu make and came up with an updated version of your patch which I will attach soon. Basically we can go further and use the foreach make function. > Also, I thought based on comment #2 that the patch in commit #1 was > undesirable? True, there were some issues with it, but those are fixed in a later commit.
Created attachment 261125 [details, diff] openrc-selinux.patch Chris and all, here is my proposal for another way to do this. Is this ok if we require gnu make? Are there any issues with doing macro expansions this way instead of using a loop in the shell command like the previous patch does?
Darn, here I was all proud of my first-ever make file, and now you're going and modifying it before the bits have even been completely stored. ;) I personally prefer to encapsulate all of the logic in the make-links command, rather than requiring a loop to be hand coded at each step, but that's just me. Also, I didn't do anything with the 'links' target in my version because, as near as I can tell, it never gets called. Can anyone confirm if it actually WORKS on other systems, as the logic appears to assume that both rc and all targets live in the same directory? (i.e. do we really even NEED the 'links' target)
(In reply to comment #11) > Darn, here I was all proud of my first-ever make file, and now you're going and > modifying it before the bits have even been completely stored. ;) Actually you did a really good job with it; I learned a lot from what you did because I've never used make functions before. ;) > I personally prefer to encapsulate all of the logic in the make-links command, > rather than requiring a loop to be hand coded at each step, but that's just me. I have no idea whether one performs better than the other or not. The difference would be in whether running a for loop in the shell is faster than running a long series of if/else/fi. What I can do is test and see if I can detect a difference. > Also, I didn't do anything with the 'links' target in my version because, as > near as I can tell, it never gets called. Can anyone confirm if it actually > WORKS on other systems, as the logic appears to assume that both rc and all > targets live in the same directory? (i.e. do we really even NEED the 'links' > target) The links target is not used in the normal build, but for testing, as far as I can tell. I just changed it so that it stays in sync with the rest of the Makefile.
Created attachment 261140 [details] openrc-selinux.patch Hi Chris, after more testing, it looks like your method of encapsulating the loop inside make-links works better. here is the current patch. Does anyone have any comments about this patch?
This has been added to openrc git, commit 21c5a02. Also, the openrc live ebuild now has selinux use flag support. Let me know how it works for you and whether or not we can close this bug.
The git patch appears to work perfectly. Thanks!
was there an issue with Chris's --applet version ? that is a clean way of handling options unlike what has been committed [1]. so unless there is some limitation in the version Chris proposed, i say we revert [1] and merge Chris's. [1] http://git.overlays.gentoo.org/gitweb/?p=proj/openrc.git;a=commitdiff;h=49339525a98b5f472c902144706a663f8a9903d1
(In reply to comment #16) > was there an issue with Chris's --applet version ? that is a clean way of > handling options unlike what has been committed [1]. so unless there is some > limitation in the version Chris proposed, i say we revert [1] and merge > Chris's. > [1] > http://git.overlays.gentoo.org/gitweb/?p=proj/openrc.git;a=commitdiff;h=49339525a98b5f472c902144706a663f8a9903d1 I didn't see any issues with Chris's code, but if we revert the commit you mentioned, we also have to revert at least 6804edf. I'll look into that and replacing it with Chris's code. Should we re-open this bug though or open another one?
i didnt mean a literal `git revert`. simply delete the code in question. Chris: your --applet code seemed to work fine for your needs ? Robin: is there anything that is missed with Chris's version ?
The version I submitted in comment #8 worked for me just fine.
(In reply to comment #19) > The version I submitted in comment #8 worked for me just fine. I'm looking over this version, on a local branch, and I see a couple of style issues. - we should use /* ... */ for comments, and not //. - Also, I'm curious why you create an extra indentation block and put variables in there instead of just making the variables local to the function?
(In reply to comment #18) > i didnt mean a literal `git revert`. simply delete the code in question. > Chris: your --applet code seemed to work fine for your needs ? > Robin: is there anything that is missed with Chris's version ? My commit went into the tree 2011/01/17, and Chris posted his first patch almost 2 weeks later in comment #3, 2011/01/29. I don't see why he needed to drop my --applet code in the first place. I specifically followed the previous handling of --version to implement --applet, and added the requirement that the existing argv[0] MUST be "rc" to ensure we're not already following a symlink that would impose an applet name.
My apologies if I've stepped on anyone's toes. It was not my desire to do so. The only reason comment #3 and #8 included code to patch rc is because I understood fromm comment #2 that there were issues with that code. I saw no further message indicating that the code had been fixed, nor was I aware at the time I submitted those changes where the git repo in question resided. I therefore was completely unaware that the problems highlighted in comment #2 had been addressed. Had I been, I would not have bothered submitting the code in comments #3 and #8, as it would have been largely pointless. Again, the only reason I submitted the code was because I did not know that a WORKING patch had already been accepted.
(In reply to comment #20) > (In reply to comment #19) > - Also, I'm curious why you create an extra indentation block and put variables > in there instead of just making the variables local to the function? > Because it makes them local to that scope, thus preventing them from polluting the larger function namspace. Since I only intended to ever use them right there, I didn't see any point in keeping them around. As a matter of style, I've always found it bothersome to have vars hanging around through the lifetime of a function when they are only needed for a few lines of localized code.
(In reply to comment #22) > My apologies if I've stepped on anyone's toes. It was not my desire to do so. I'm wondering what problems vapier has with my final code since I fixed the style problems the same day as request in comment #2. > The only reason comment #3 and #8 included code to patch rc is because I > understood fromm comment #2 that there were issues with that code. I saw no > further message indicating that the code had been fixed, nor was I aware at the > time I submitted those changes where the git repo in question resided. I > therefore was completely unaware that the problems highlighted in comment #2 > had been addressed. Vapier took this to the openrc mail alias and we hashed it out there, we added scripts to detect style and other issues. > Had I been, I would not have bothered submitting the code > in comments #3 and #8, as it would have been largely pointless. > > Again, the only reason I submitted the code was because I did not know that a > WORKING patch had already been accepted. http://git.overlays.gentoo.org/gitweb/?p=proj/openrc.git;a=blame;f=src/rc/rc.c;h=bdda2ef626e48149c5e9e74ed5637bef85923318;hb=43678fd2c44ec35acdcf0316c8ff3b07ee1e5f57#l820 That's the final version after vapier's requested cleanups. One useful thing I see in your version is adding the -a fake entry into getopt, and the related --help output.
poor --version implementation isnt a good reason to add more code like it. Chris's patch adds flags using the existing getopt functionality and avoids manual parsing. while you have addressed the feedback i posted, that doesnt mean we cant punt it in favor of a better implementation. if you really want to keep the requirement that the applet be "rc", it's trivial to add a strcmp() to enforce it. although i'm not sure how important it is.
If we want to covert to his version: - do we really need getopt_long invoked twice? - Add the strcmp for "rc", mainly as I'm trying to enforce consistency in what the official binary to call for multicall support is.
(In reply to comment #26) > If we want to covert to his version: > - do we really need getopt_long invoked twice? The only reason I used a separate getop_long call is because run_applets is explicitly called in the code well before any other command-line options are parsed. Although I couldn't see any reason that this had to be so, I operated on the assumption that the least invasion possible to code I didn't fully understand was the best approach. You obviously know more than I regarding the implementation, but I think it would make much more sense to handle it all in the single getopt_long loop if there isn't any particular reason to NOT to do so.
(In reply to comment #26) > - do we really need getopt_long invoked twice? We might be able to avoid calling it twice using the value of optind. We might also be able to use this to change the --version option to be handled by the parser instead of having a special case for it. > - Add the strcmp for "rc", mainly as I'm trying to enforce consistency in what > the official binary to call for multicall support is. I can take a look at adding this as well to the --applet portion of the switch.
Created attachment 262605 [details] applet.patch All, Please review this reworked implementation of the --applet option. Chris, this includes the option and help additions from your patch. The part that was reworked however, is the part that actually selects the applet to run when the --applet option is present. This was moved to run_applets(). Robin, I believe this meets your requirements, because we don't try to readjust the applet name unless it is "rc" to begin with (see the code in the modified run_applets() function), and we do not call getopts_long() twice. Please let me know what you think of this implementation. Mike, what do you think of this implementation?
WilliamH: +1 on the patch, looks good. Minor request: refactor the comment on with the bug number and put it above the new block in rc-applets.c?
(In reply to comment #30) > Minor request: refactor the comment on with the bug number and put it above the > new block in rc-applets.c? No problem, I'll do that before I commit.
The patch from comment #29 is now in the git repository, commit b512d0d. I am closing this bug since it was re-opened by a maintainer of openrc. Please re-open if there is still an issue.