Some questions about using restorecon on symlink

Discussion:

weiyuan

2015-09-08 09:34:10 UTC

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" )

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?

2. Can symlink and realpath have different selables?
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.

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?

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.

Any help is appreciated.

Regards,

Weiyuan

Stephen Smalley

2015-09-08 16:44:31 UTC

Permalink

Post by weiyuan
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.-----------
"/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.-----------
"/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.-----------
"/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.-----------
"/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.

Post by weiyuan
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?

Post by weiyuan
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).

Post by weiyuan
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.

Post by weiyuan
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/

Post by weiyuan
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.

weiyuan

2015-09-09 08:17:37 UTC

Permalink

Thanks to your kindly help.

Post by Stephen Smalley

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.

Yes, that is exactly what I did in Android 5.0.

Post by Stephen Smalley

Post by weiyuan
2. Can symlink and realpath have different selables?

Post by weiyuan
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.

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.

Stephen Smalley

2015-09-09 19:15:16 UTC

Permalink

Post by weiyuan
Thanks to your kindly help.

Post by Stephen Smalley

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.

Yes, that is exactly what I did in Android 5.0.

Post by Stephen Smalley

Post by weiyuan
2. Can symlink and realpath have different selables?

Post by weiyuan
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.

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.
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.

Daniel Cashman

2015-09-10 17:08:14 UTC

Permalink

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:

***@shamu:/ # ls -ladZ /data/misc/perfprofd

drwxrwxr-x root root u:object_r:perfprofd_data_file:s0
perfprofd
***@shamu:/ # restorecon -R /data/misc/keystore/../perfprofd

SELinux: Loaded file_contexts contexts from /file_contexts.
***@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.