New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow su operation for SELinux Enforcing systems #234

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@mohankr
Copy link

mohankr commented Feb 26, 2014

This patch uses virtual resource names for IPC, which is currently not under the purview of
SELinux, for now at least :)

fix: allow su operation for SELinux Enforcing systems
This patch uses virtual resource names for IPC, which is currently not under the purview of
SELinux, for now at least :)
@jcmajor

This comment has been minimized.

Copy link

jcmajor commented May 2, 2014

Would it be appropriate for su in daemon mode to make an selinux call to set permissions on the socket it created? Would that get it across the line?

@mohankr

This comment has been minimized.

Copy link

mohankr commented May 2, 2014

If you could, the constraint I am trying to work against is where the selinux policy cannot be changed (that is, it is embedded in the boot image, like the samsung kernels). The other problem may/may not is the selinux is distributed as a shared lib I believe, we would need to check for whether the system is selinux enabled if you do want to go that route. The abstract resource I think is much cleaner without messing around with the permissions. (currently for locked down boot images we have to "shim" the startup of su in daemon mode anyways, not sure how long even this hack would work, if we the OEM locks that down)..

@koush

This comment has been minimized.

Copy link
Owner

koush commented May 2, 2014

Are abstract resources protected by filesystem permissions? I guess that's a nonissue since the socket peer is identified now. It's been a while since I've digged into this code. Would be good to have other eyes look at this for security problems.

@mohankr

This comment has been minimized.

Copy link

mohankr commented May 2, 2014

I don't believe so (it is more of linux thing, BSD for example completely ignores the file system permission for unix domain sockets), hence the patch removes it. Also the current implementation opens up the permissions anyway (0777).
For this particular use case (su daemon), you are probably much better of securing at the communication protocol level (which I believe is being done), then rely on it at the infrastructure level. For a SElinux hardened system, you can apply security at a much for fine grained level for a socket anyways.

@koush

This comment has been minimized.

Copy link
Owner

koush commented May 2, 2014

I think this is a good change. I recall initially wanting to use abstract sockets, but did not since they were not protected by filesystem permissions, which was the original implementation. Since then, also added checking the socket peer credentials as well to allow non-setuid invocation support.

I'll see if I can get a few more eyes on this, as it pretty dramatically changes the internals of how the su daemon communicates with the world :)

@jcmajor

This comment has been minimized.

Copy link

jcmajor commented May 5, 2014

I think, given the general direction of this app, that, despite the hinderance, SE enforcing represents a significant opportunity. My (limited) reading of the se doco suggests that you can get very fine grained control of access to sockets.

I started a big para about Superuser managing contexts for access to the su binary and then realised all the apps appear as zygote. There might still be something in that.

I looked a bit in the daemon code and can see you have defaulted to denied in places where SE might be configured to deny you access to critical info (like the sockets remote credentials). I haven't looked hard enough yet to say confidently "its good", but it looks to be in reasonable shape. I'll try to review it better this week.

On a slightly different track:

I am currently playing with my 4.3 GS4. I have compiled this patch and put it manually into a system partition, from the providers image, by loop mounting it on linux, then using img2simg and heimdall to get it onto the phone. ** This has not bumped the knox flag. ** I needed to use setfilecon while the image was mounted on linux to give execute rights on the su binary.

I currently get a blank Superuser dialog that flashes almost too fast to read when anything asks for su. The calling app waits until superuser is manually started, and then claims it was denied. I have tried auto allow, no pin, no tick for declared. "There are currently no Superuser app policies". I have one entry in the log for Superuser allow, and three for Titanium backup denied, despite several attempts. I seem to be getting one entry in the log per reboot. Setting auto accept, rebooting and hitting titanium still failed.

For a connectbot local su invoke, logcat shows
su ... su invoked.
su ... starting daemon client 10284 10284
connectbot ... onPause called
connectbot ... onResume called
su ... client exited

Update:
I have root via adb shell. Still looking. I could be blind but it looks like the socket for the interactive response does not get created and does not log an error.

Update2:
Smacking self in head. Forgot to bring patched apk file! Know a whole lot more about workings now though.

@jcmajor

This comment has been minimized.

Copy link

jcmajor commented May 5, 2014

This patch does not appear to reduce security. I do think I might have found a hole in the original (and this version) though. Not sure how to PM koush. If I'm right, I don't want to post it here till theres a fix.

@koush

This comment has been minimized.

Copy link
Owner

koush commented May 5, 2014

koush @ koushikdutta.com

@Djamal57baraja

This comment has been minimized.

Copy link

Djamal57baraja commented Jun 6, 2014

Thank you very much to let me use your Great application.

@Djamal57baraja

This comment has been minimized.

Copy link

Djamal57baraja commented on ce6c2d4 Jun 16, 2014

Thank you very much. Great appl

This comment has been minimized.

Copy link

malamel replied May 4, 2016

.

This comment has been minimized.

Copy link

marcowill replied Dec 5, 2017

Thanks!!!

@koush

This comment has been minimized.

Copy link
Owner

koush commented Jul 9, 2014

So, Android L dropped, which is in enforcing mode. Connecting to abstract sockets does not work:

I/ShellProcess(14661): Using su...
I/am_on_resume_called(14661): [0,com.koushikdutta.backup.MainActivity]
D/su      (15110): su invoked. 
I/HotwordRecognitionRnr(14582): Hotword detection finished
I/HotwordRecognitionRnr(14582): Stopping hotword detection.
D/su      (15112): starting daemon client 10104 0 
I/auditd  (15109): type=1400 audit(0.0:51): avc:  denied  { connectto } for  comm="su" path=005355504552555345520000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 scontext=u:r:untrusted_app:s0 tcontext=u:r:init:s0 tclass=unix_stream_socket
E/su      (15114): connect failed with 13: Permission denied 
@koush

This comment has been minimized.

Copy link
Owner

koush commented Jul 9, 2014

root from shell works, but root from other apps results in this connect to abstract socket error. i had a similar issue using abstract sockets for IPC inside a zygote based process in another app.

@koush

This comment has been minimized.

Copy link
Owner

koush commented Jul 9, 2014

I may need to go back to a FIFO based method.

@koush

This comment has been minimized.

Copy link
Owner

koush commented Jul 9, 2014

Ugh, apparently fifo doesnt work anymore either. Creating a file as root, and changing ownership to another uid does not work. The new uid still can't access it.

@mohankr

This comment has been minimized.

Copy link

mohankr commented Jul 10, 2014

Yup that is correct..There is way around it by shiming in the
daemon...That's what I did..or obviously create new policy
On Jul 9, 2014 5:48 PM, "Koushik Dutta" notifications@github.com wrote:

Ugh, apparently fifo doesnt work anymore either. Creating a file as root,
and changing ownership to another uid does not work. The new uid still
can't access it.


Reply to this email directly or view it on GitHub
#234 (comment).

@koush

This comment has been minimized.

Copy link
Owner

koush commented Jul 10, 2014

shimming in the daemon? What do you mean?

@koush

This comment has been minimized.

Copy link
Owner

koush commented Jul 10, 2014

Well, creating a new policy is not ideal. Requiring a custom boot image is rather annoying.

@mohankr

This comment has been minimized.

Copy link

mohankr commented Jul 10, 2014

Yes, new policy is not ideal as noted before. By "shiming" I meant find a process (there will always be one :) ) that has all the SE Policies that the SU daemon needs for working in a enforcing mode, replace that with a script, that starts both of them. It is of course a hack, it will preserve the custom boot image. The downside is for proprietary roms, it is not future proof. I believe the other SU program/app uses the same approach.
The only realistic option is to use the SElinux API and open up all the policies, I went through the exercise, don't have it handy, with 4.4.2 drop at least for Sammy ROMS, there were way too many policies that needed to be changed. And then of course unless statically compiled, the shared lib must exist on the target ROM.
After shimming I had to do another thing, since samsung's 4.4.2 derivatives the damn /system/bin/sh (ash shell) that is spawned by the daemon, ran into the SELinux enforcing wall, vaguely remember it has permission denied to open a pty (will need to dig it up), of course there is a work around for that too :), might not be an issue for CM or other derivatives.

@bonog

This comment has been minimized.

Copy link

bonog commented on ce6c2d4 Feb 2, 2015

Im to close

This comment has been minimized.

Copy link

ditta95aR replied May 5, 2017

This comment has been minimized.

Copy link

ononoman replied Jun 26, 2018

good

This comment has been minimized.

Copy link

jockeryeens replied Jul 28, 2018

@eltirol

This comment has been minimized.

Copy link

eltirol commented Apr 1, 2017

Ok

@ditta95aR

This comment has been minimized.

@moragnel

This comment has been minimized.

Copy link

moragnel commented on Superuser/jni/su/daemon.c in ce6c2d4 May 29, 2017

Springfield
if (bind(fed, (structure sock adder*)&

This comment has been minimized.

Copy link

jockeryeens replied Mar 28, 2018

D

@moragnel

This comment has been minimized.

@uiu50

uiu50 approved these changes Dec 13, 2017

@jockeryeens

This comment has been minimized.

Copy link

jockeryeens commented Mar 28, 2018

Dd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment