Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 351712 - sys-apps/openrc symlinks prevent selinux from assigning proper contexts to utils such as rc-update
Summary: sys-apps/openrc symlinks prevent selinux from assigning proper contexts to ut...
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: [OLD] baselayout (show other bugs)
Hardware: All Linux
: High normal (vote)
Assignee: SE Linux Bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 295613
  Show dependency tree
 
Reported: 2011-01-15 00:03 UTC by Chris Richards
Modified: 2011-05-04 19:32 UTC (History)
2 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
selinux patches for openrc 0.7.0 (openrc-0.7.0-selinux.patch,4.08 KB, text/plain)
2011-01-29 22:46 UTC, Chris Richards
Details
apply selinux patch in ebuild (openrc-0.7.0-r1.ebuild.diff,866 bytes, patch)
2011-01-29 22:49 UTC, Chris Richards
Details | Diff
patch for replacing openrc symlinks with wrapper scripts (openrc-0.7.0-selinux.patch,3.87 KB, patch)
2011-01-30 05:32 UTC, Chris Richards
Details | Diff
openrc-selinux.patch (openrc-selinux.patch,2.08 KB, patch)
2011-01-30 22:18 UTC, William Hubbs
Details | Diff
openrc-selinux.patch (openrc-selinux.patch,2.01 KB, text/plain)
2011-01-31 00:54 UTC, William Hubbs
Details
applet.patch (applet.patch,2.31 KB, text/plain)
2011-02-15 15:13 UTC, William Hubbs
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Richards 2011-01-15 00:03:23 UTC
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.
Comment 1 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2011-01-17 07:39:50 UTC
"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.
Comment 2 SpanKY gentoo-dev 2011-01-17 08:14:45 UTC
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.
Comment 3 Chris Richards 2011-01-29 22:46:44 UTC
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.
Comment 4 Chris Richards 2011-01-29 22:49:03 UTC
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 5 William Hubbs gentoo-dev 2011-01-30 01:20:41 UTC
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 6 William Hubbs gentoo-dev 2011-01-30 03:51:22 UTC
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 7 William Hubbs gentoo-dev 2011-01-30 03:56:27 UTC
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.
Comment 8 Chris Richards 2011-01-30 05:32:37 UTC
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?
Comment 9 William Hubbs gentoo-dev 2011-01-30 22:15:11 UTC
(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.
Comment 10 William Hubbs gentoo-dev 2011-01-30 22:18:01 UTC
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?
Comment 11 Chris Richards 2011-01-30 22:57:57 UTC
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)
Comment 12 William Hubbs gentoo-dev 2011-01-30 23:43:41 UTC
(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.
Comment 13 William Hubbs gentoo-dev 2011-01-31 00:54:44 UTC
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?
Comment 14 William Hubbs gentoo-dev 2011-02-01 19:32:38 UTC
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.
Comment 15 Chris Richards 2011-02-02 02:50:13 UTC
The git patch appears to work perfectly.  Thanks!
Comment 16 SpanKY gentoo-dev 2011-02-06 18:58:37 UTC
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
Comment 17 William Hubbs gentoo-dev 2011-02-06 19:38:23 UTC
(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?
Comment 18 SpanKY gentoo-dev 2011-02-06 22:01:53 UTC
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 ?
Comment 19 Chris Richards 2011-02-06 22:27:36 UTC
The version I submitted in comment #8 worked for me just fine.
Comment 20 William Hubbs gentoo-dev 2011-02-06 23:05:25 UTC
(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?
Comment 21 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2011-02-06 23:17:25 UTC
(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.
Comment 22 Chris Richards 2011-02-06 23:35:19 UTC
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.
Comment 23 Chris Richards 2011-02-06 23:39:06 UTC
(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.
Comment 24 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2011-02-06 23:59:40 UTC
(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.
Comment 25 SpanKY gentoo-dev 2011-02-07 00:36:04 UTC
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.
Comment 26 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2011-02-07 00:48:26 UTC
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.
Comment 27 Chris Richards 2011-02-07 02:09:24 UTC
(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.
Comment 28 William Hubbs gentoo-dev 2011-02-07 04:34:27 UTC
(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.
Comment 29 William Hubbs gentoo-dev 2011-02-15 15:13:30 UTC
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?
Comment 30 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2011-02-15 21:55:33 UTC
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?
Comment 31 William Hubbs gentoo-dev 2011-02-15 22:27:47 UTC
(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.
Comment 32 William Hubbs gentoo-dev 2011-02-16 15:19:32 UTC
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.