On Thu, Sep 10, 2015 at 10:08 AM, Daniel Cashman <[email protected]> wrote:
> Thank you Stephen for addressing this and sorry for the delay. > > I've just uploaded https://android-review.googlesource.com/#/c/169898/ which > allows for restorecon to properly label symlinks directly, but does not > address the desire to label underlying files pointed to by symlinks. The > original change was designed to prevent just such a thing due to our desire > to make sure expanded use of restorecon in the codebase does not come with > missing path validation checks. The pathological example of what we want > to avoid could be characterized by the following: > > root@shamu:/ # ls -ladZ /data/misc/perfprofd > > drwxrwxr-x root root u:object_r:perfprofd_data_file:s0 > perfprofd > root@shamu:/ # restorecon -R /data/misc/keystore/../perfprofd > > SELinux: Loaded file_contexts contexts from /file_contexts. > root@shamu:/ # ls -ladZ /data/misc/perfprofd > > drwxrwxr-x root root u:object_r:keystore_data_file:s0 > perfprofd > > As has been mentioned, it is very useful especially in the case of sysfs > to use symlinks to do underlying labels. I'd be open to perhaps adding a > flag which allows that behavior and only adding it to the init built-in, or > adding special behavior for sysfs, but am not sure yet which the preferred > approach would be. > Can we do anything with the mode field of file_contexts perhaps to indicate the desire? > > On Wed, Sep 9, 2015 at 12:15 PM Stephen Smalley <[email protected]> wrote: > >> On 09/09/2015 04:17 AM, weiyuan wrote: >> > Thanks to your kindly help. >> > >> > On 2015/9/9 0:44, Stephen Smalley wrote: >> >> On 09/08/2015 05:34 AM, weiyuan wrote: >> >>> Dear All: >> >>> >> >>> On Android 6.0, >> >>> >> >>> I have a file "/sys/class/leds/red/brightness" under /sys, its parent >> directory is a symlink. >> >>> >> >>> "u:object_r:sysfs:s0 red -> >> ../../devices/fff34000.pmic/pmic_led.118/leds/red" >> >>> "u:object_r:sysfs:s0 brightness" >> >>> >> >>> I notice that there is a patch "restorecon: only operate on canonical >> paths.", >> >>> so I add some logs like "--SELINUX--:" in the function >> "selinux_android_restorecon_common", then I runs some tests. >> >>> >> >>> -----------test A.----------- >> >>> >> >>> file_contexts: >> >>> "/sys/class/leds/red/brightness u:object_r:sysfs_led:s0" >> >>> >> >>> # restorecon /sys/class/leds/red/brightness >> >>> => "--SELINUX--: selabel_lookup failed. pathname = >> /sys/devices/fff34000.pmic/pmic_led.118/leds/red/brightness" >> >>> >> >>> # ls -Z /sys/class/leds/red/brightness >> >>> u:object_r:sysfs:s0 brightness unchanged >> >>> >> >>> >> >>> restorecon find the realpath of "brightness" has no match in >> file_contexts, so it failed. >> >>> >> >>> -----------test B.----------- >> >>> >> >>> file_contexts: >> >>> "/sys/class/leds/red(/.*)? u:object_r:sysfs_led:s0" >> >>> >> >>> # restorecon -Rv /sys/class/leds/red >> >>> => "--SELinux--: pathname >> =/sys/devices/fff34000.pmic/pmic_led.118/leds/red" >> >>> >> >>> # ls -Z /sys/class/leds/red >> >>> u:object_r:sysfs:s0 red -> >> ../../devices/fff34000.pmic/pmic_led.118/leds/red unchanged >> >>> >> >>> >> >>> restorecon find the realpath of "red" has no match in file_contexts, >> so it failed. >> >>> -----------test C.----------- >> >>> >> >>> file_contexts: >> >>> "/sys/class/leds/red(/.*)? u:object_r:sysfs_led:s0" >> >>> >> >>> # restorecon -Rv /sys/class/leds >> >>> => "--SELINUX--:selabel_lookup failed. pathname = /sys/class/leds >> >>> SELinux: Relabeling /sys/class/leds/red from >> u:object_r:sysfs:s0 to u:object_r:sysfs_led:s0." >> >>> >> >>> # ls -Z /sys/class/leds >> >>> u:object_r:sysfs_led:s0 red -> >> ../../devices/fff34000.pmic/pmic_led.118/leds/red changed >> >>> >> >>> # cd /sys/class/leds/red >> >>> # ls -Z >> >>> u:object_r:sysfs:s0 brightness >> unchanged >> >>> >> >>> # ls -Z /sys/devices/fff34000.pmic/pmic_led.118/leds >> >>> u:object_r:sysfs:s0 red unchanged >> >>> >> >>> >> >>> restorecon find the realpath of "leds" has a match in file_contexts, >> so set "red" successed; >> >>> BUT it failed to set files in "red". And the original file's selable >> is unchanged. >> >>> >> >>> -----------test D.----------- >> >>> Use "stat" on these files: >> >>> "/sys/class/leds/red" and >> "/sysdevices/fff34000.pmic/pmic_led.118/leds/red" are different inodes。 >> >>> "/sys/class/leds/red/brightness" and >> "/sysdevices/fff34000.pmic/pmic_led.118/leds/red/brightness" are the same >> inode. >> >>> (Which means that any change on >> realpath"/sysdevices/fff34000.pmic/pmic_led.118/leds/red/brightness" will >> >>> simultaneously reflect on the symlink file >> "/sys/class/leds/red/brightness" ) >> >> >> >> Note: /sys/class/leds/red/brightness is NOT a symlink file. It is a >> >> regular file that you looked up via the /sys/class/leds symlink. But >> >> only /sys/class/leds/red is a symlink here. >> >> >> >>> >> >>> >> >>> My problem is : >> >>> 1. The realpath of "/sys/class/leds/red" is various on different >> devices, but the symlink path is fixed. >> >>> If I want to set the selabel of "/sys/class/leds/red/brightness", >> I have to >> >>> add "[realpath]/brightness [label]" in file_contexts on every >> devices differently, >> >>> because the realpath of "brightness" is different. >> >>> Can this be done with other ways that not so inconvenient? >> >> >> >> How did you do it in Android 5.0? I guess there you could add a >> >> "restorecon /sys/class/leds/red/brightness" command to your >> >> init.<board>.rc file and it wouldn't try to canonicalize the path and >> >> would therefore lookup the provided path and label it with the matching >> >> context. Is that what you were doing previously? >> >> >> > >> > >> > Yes, that is exactly what I did in Android 5.0. >> > >> > >> >>> >> >>> 2. Can symlink and realpath have different selables? >> >> >> >> Yes, they resolve to different inodes (a symbolic link is its own >> >> file/inode in Linux, separate and independent of the target). The >> >> symbolic link SELinux label only controls access to the symlink (i.e. >> >> the ability to unlink, rename, or read it), not access to its target >> >> (which is controlled based on the target's label). >> >> >> >>> If they have different selables, what about the files in symlink >> directory, like "brightness"? >> >>> Which selable should it follow, since it has only one inode exist. >> >> >> >> brightness is a regular file; only /sys/class/leds/red is a symlink, so >> >> there is only one inode and one SELinux label to consider for >> brightness. >> >> >> >>> 3. If symlink and realpath can have different selables, >> >>> I think the patch "restorecon: only operate on canonical paths." >> is not appropriate. >> >>> If I want to set symlink's selable, I have to run restorecon on >> its parent directory, >> >>> and it will only change the directory self, not the files in the >> directory. >> >>> In the meanwhile, if I restorecon the symlink directory directly, >> it will fail. >> >>> Is this a Bug? >> >> >> >> Yes and no; on the one hand, the patch broke something that used to >> work >> >> in Android 5.0 IIUC; on the other hand, that approach only worked as a >> >> hack. file_contexts is supposed to specify real paths (there is an >> >> exception for /dev/block/platform/.../by-name symlinks, but that's >> >> handled specially by init/ueventd). See >> >> http://marc.info/?l=seandroid-list&m=142482974726583&w=2 and >> >> https://android-review.googlesource.com/#/c/97701/ >> >> >> >>> 4. How about enforce symlink and realpath have the same selable? >> >>> When restorecon meet a symlink, >> >>> 1) find the realpath >> >>> 2) call selabel_lookup with the realpath, if failed, call >> selabel_lookup with the symlink. >> >>> 3) use the selabel find in step 2) to set label to both symlink >> and realpath. >> >> >> >> This is somewhat akin to the selabel_lookup_best_match() concept used >> by >> >> init/ueventd for the /dev symlinks, but is problematic for restorecon. >> >> ueventd handles it at device file creation time, and knows the real >> path >> >> and all of the symlink paths up front. restorecon is just walking the >> >> file tree and relabeling files as it goes. When it reaches the real >> >> file, it doesn't know about any symbolic links to it. When it reaches >> >> one symbolic link, it doesn't know about any others. It might visit >> the >> >> symbolic link, label the file to match your >> >> /sys/class/leds/red/brightness entry, and then visit the real file >> later >> >> and relabel it back to a different type. It may also be unsafe if used >> >> on any directory tree that can be modified by an untrusted entity, as >> in >> >> restorecon -R /data, since that could allow someone to force a >> sensitive >> >> file to be relabeled by creating a symlink to it. >> >> >> >> However, your approach might work in the restricted case of sysfs, just >> >> by virtue of the fact that there is no default /sys(/.*)? entry in >> >> file_contexts (intentionally, so that we can bail immediately when >> there >> >> is no specific match), so we are unlikely to have two conflicting >> >> entries for the real path and the symlink path, and the /sys tree >> should >> >> be safe against such manipulation. >> >> >> >> Nonetheless, it probably carries a cost - invoking realpath() on every >> >> symlink under /sys that is visited could be expensive, although we are >> >> at least pruning the tree walk based on selabel_partial_match(). Would >> >> require some investigation and performance testing at least. >> >> >> >> Alternative would be to just revert the "restorecon: only operate on >> >> canonical paths" change so you can still use restorecon commands in >> >> init.<board>.rc files and have them "work" on paths containing symbolic >> >> links. Not sure though what may be depending on that change. >> >> >> >> >> >> >> >> >> > >> > Yes, I'm afraid that revert the patch will cause some unknown issues. >> > >> > I notice that, when fts_read meets a symlink directory, it appears >> > to not continue to scaning files in it but just stops there. >> > >> > How about add a -S flag to restorecon, which will act like: >> > 1. If -R is presented, -S will be ignored. (Because for -R, I don't know >> > how to make fts_read keep looking files under symlink directory) >> > 2. With -S flag, restorecon will not invoke realpath, but directly >> > go to restorecon_sb. >> > >> > This will change restorecon's behavior in two ways, one is that >> restorecon can >> > set symlink's lable without change symlink's realpath's lable; another >> one is >> > that restorecon can change a regular file's lable with any path the >> caller >> > may provide. >> > >> > So it will keep thing that restorecon used to work in Android 5.0 >> somehow,and >> > without reverting the patch. >> >> init restorecon command is a built-in; it does not exec the toolbox >> restorecon command, so adding options there won't help you with a >> restorecon from init.<board>.rc. >> >> The latter is implemented by system/core/init/builtins.cpp, in the >> do_restorecon() function, which calls utils.cpp:restorecon(), which >> calls selinux_android_restorecon() in libselinux with 0 flags argument. >> >> init commands aren't supposed to take options; that's why it has >> separate restorecon and restorecon_recursive built-in commands even >> though underneath it is just passing different flags. >> >> You could add a restorecon_norealpath or similar built-in command, and >> have it pass a new NOREALPATH flag to selinux_android_restorecon(), and >> use that to decide whether to call realpath or not. It isn't really >> about whether or not the file is a symlink but whether you want realpath >> used. >> >> It isn't a general solution however, and it requires you to manually add >> such restorecon_norealpath calls to your init.<board>.rc file. It would >> be better to fix the underlying libselinux logic to handle it correctly >> so that the existing restorecon_recursive("/sys") by init would just >> label it the way you want in the first place. >> >> > _______________________________________________ > Seandroid-list mailing list > [email protected] > To unsubscribe, send email to [email protected]. > To get help, send an email containing "help" to > [email protected]. > -- Respectfully, William C Roberts
_______________________________________________ Seandroid-list mailing list [email protected] To unsubscribe, send email to [email protected]. To get help, send an email containing "help" to [email protected].
