Bug 1219718 - matchpathcon_init_prefix() does not behave as advertised
Summary: matchpathcon_init_prefix() does not behave as advertised
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: libselinux
Version: 22
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ---
Assignee: Daniel Walsh
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-05-08 01:54 UTC by Adam Williamson
Modified: 2015-07-29 01:57 UTC (History)
6 users (show)

Fixed In Version: libselinux-2.3-10.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-06-01 17:01:14 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
matchpathcon/selabel_file: Fix man pages (5.11 KB, patch)
2015-05-11 15:03 UTC, Stephen Smalley
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1190377 0 unspecified CLOSED SELinux is preventing polkitd from 'read' accesses on the lnk_file localtime. 2021-02-22 00:41:40 UTC

Internal Links: 1190377

Description Adam Williamson 2015-05-08 01:54:07 UTC
This is an upstream bug, but I'm not subscribed to the upstream mailing list and really don't want another ML subscription.

This function is defined thus:

       int matchpathcon_init_prefix(const char *path, const char *subset);

       matchpathcon_init_prefix() is the same as matchpathcon_init() but  only
       loads  entries  with  regular  expressions  that have stems prefixed by
       prefix.

Apart from the obvious error (subset vs. prefix), the function doesn't actually seem to do that at all.

First, once sefcontext_compile has run (well, we think that's the trigger), you can pass it any damn subset/prefix you please and it will load all contexts. I've been doing interactive tests with the Python module since it's easy, but per https://bugzilla.redhat.com/show_bug.cgi?id=1190377 and https://github.com/mlichvar/timedatex/pull/1 , the actual C library does seem to behave the same. So:

[adamw@adam selinux (master)]$ ls /etc/selinux/targeted/contexts/files/*.bin
/etc/selinux/targeted/contexts/files/file_contexts.bin
/etc/selinux/targeted/contexts/files/file_contexts.homedirs.bin
/etc/selinux/targeted/contexts/files/file_contexts.local.bin
[adamw@adam selinux (master)]$ python
Python 2.7.9 (default, Apr 15 2015, 12:08:00) 
[GCC 5.0.0 20150319 (Red Hat 5.0.0-0.21)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import selinux
>>> selinux.matchpathcon_init_prefix(None, '/foo/bar/monkeys')
0
>>> selinux.matchpathcon('/bin/bash', 0)
[0, 'system_u:object_r:shell_exec_t:s0']
>>> selinux.matchpathcon_init_prefix(None, 'ilikehats')
0
>>> selinux.matchpathcon('/bin/bash', 0)
[0, 'system_u:object_r:shell_exec_t:s0']
>>> 

If sefcontext_compile has *not* run - the case where we ran into the initial bug that led me to discover this stuff, that's #1190377 - it seems that the value you pass does have *some* significance, but it still doesn't work as advertised. To get a system in this situation for testing, you can install from a live image: until you update an selinux package or do something else that'd trigger an 'semodule' call or something, the system will be in this state.

In this case, what happens seems to be that so long as the value you pass matches *any* of the regexps in one of the files in /etc/selinux/targeted/contexts/files , *all* of the entries in that file will be loaded. So if you pass a string that matches *any* of the regexps in /etc/selinux/targeted/contexts/files/file_contexts , *all* of the contexts in that file will be loaded - but not any of the ones in /etc/selinux/targeted/contexts/files/file_contexts.homedirs . If you pass a string that doesn't match any regexes in any of the files, no contexts will be loaded.

So if we use /usr/lib/nagios/cgi/fooagsa (because it matches the regex /usr/lib/nagios/cgi(/.*)? ), we can then successfully do matchpathcon against a file that matches a completely *different* regex in that same file:

>>> selinux.matchpathcon_init_prefix(None, '/usr/lib/nagios/cgi/fooagsa')
0
>>> selinux.matchpathcon('/bin/bash', 0)
[0, 'system_u:object_r:shell_exec_t:s0']

But we can't do matchpathcon against /home/test:

>>> selinux.matchpathcon('/home/test', 0)
[0, 'system_u:object_r:default_t:s0']

because that's in file_contexts.homedirs. If we pass a string that just doesn't match any regex, all matchpathcons return default:

>>> selinux.matchpathcon_init_prefix(None, 'ilikehats')
0
>>> selinux.matchpathcon('/home/adamw', 0)
[0, 'system_u:object_r:default_t:s0']
>>> selinux.matchpathcon('/bin/bash', 0)
[0, 'system_u:object_r:default_t:s0']
>>> 

This behaviour still doesn't match the documentation, to me.

Note that fixing this might break users which have been using matchpathcon_init_prefix() wrongly but getting away with it because of the bug...like timedatex.

Comment 1 Adam Williamson 2015-05-08 02:17:49 UTC
OK, now it's just being weird at me.

This I *can* kinda understand on the basis that the entry for /etc/localtime is not a regex:

>>> selinux.matchpathcon_init_prefix(None, '/etc/localtime')
0
>>> selinux.matchpathcon('/etc/localtime', 0)
[0, 'system_u:object_r:default_t:s0']
>>>

But this, I just can't come up with a convincing explanation for other than gremlins:

>>> selinux.matchpathcon_init_prefix(None, '/etc/localtime')
0
>>> selinux.matchpathcon('/etc/resolv.conf', 0)
[0, 'system_u:object_r:net_conf_t:s0']
>>>
>>> selinux.matchpathcon_init_prefix(None, '/usr/share/zoneinfo')
0
>>> selinux.matchpathcon('/etc/resolv.conf', 0)
[0, 'system_u:object_r:default_t:s0']
>>> selinux.matchpathcon('/etc/localtime', 0)
[0, 'system_u:object_r:locale_t:s0']
>>>

So...init_prefix /etc/localtime somehow loads the context for /etc/resolv.conf (but not /etc/localtime, as we saw above), while init_prefix /usr/share/zoneinfo loads the context for /etc/localtime (and lots of other things, like /bin/bash and so on) but somehow *not* the context for /etc/resolv.conf? What the hell.

Comment 2 Stephen Smalley 2015-05-08 15:18:48 UTC
Does seem to be a bug.  However:
1) matchpathcon* are all deprecated interfaces, all users should have long since migrated to selabel*.
2) IIRC, matchpathcon_init_prefix() was purely added as an optimization for udev, so that it could prune loading of file_contexts entries to only /dev entries and speed up initialization.  With the introduction of the precompiled file_contexts.bin file, this became entirely irrelevant.

Comment 3 Stephen Smalley 2015-05-08 15:40:18 UTC
Is there something unique about the python bindings?  Because I see different results via python vs C when using matchpathcon_init_prefix:
# rm -f /etc/selinux/targeted/contexts/files/*.bin
# matchpathcon -p /etc /etc/localtime
/etc/localtime	system_u:object_r:locale_t:s0
# python
Python 2.7.8 (default, Apr 15 2015, 09:26:43) 
[GCC 4.9.2 20150212 (Red Hat 4.9.2-6)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import selinux
>>> selinux.matchpathcon_init_prefix(None, '/etc')
0
>>> selinux.matchpathcon('/etc/localtime', 0)
[0, 'system_u:object_r:default_t:s0']

Comment 4 Adam Williamson 2015-05-08 15:52:24 UTC
Well, I took a quick look and the bindings seemed very light. There's really only https://github.com/SELinuxProject/selinux/blob/master/libselinux/src/selinuxswig_python.i , isn't there?

I can confirm the same thing you see, though. Odd. 'matchpathcon -p' seems to behave a lot more 'as expected' than the python case. *BUT*, when I tried patching timedatex to use matchpathcon_init_prefix(NULL, LOCALTIME_PATH) - which should be the same as matchpathcon -p /etc/localtime - it didn't work!

Comment 5 Stephen Smalley 2015-05-08 15:59:11 UTC
IMHO, we should just stop using matchpathcon_init_prefix() altogether; it was really only for udev and only ever called with a stem (e.g. "/dev"), not a full path.  For that matter, the matchpathcon() family of functions was obsoleted by selabel* and users should be updated to use selabel* instead.
matchpathcon utility might differ in another way though; it might be passing the file mode rather than 0 as the mode argument.  Why that would make a difference though since the file_context's file does not specify a file mode for that entry I do not see...

Comment 6 Adam Williamson 2015-05-08 16:09:33 UTC
Sure, we're already discussing exactly what timedatex should be doing instead of using matchpathcon* in the other bug. But even if timedatex stops using it, the odd behaviour of matchpathcon_init_prefix() doesn't go away, it still seems worth doing something about - even if that 'something' is to deprecate the function.

There's definitely some kind of difference between command line 'matchpathcon -p' and C compiled matchpathcon_init_prefix(). timedatex git master as it stands actually winds up doing this:

security_context_t con;
matchpathcon_init_prefix(NULL, '../usr/share/zoneinfo/some/time/zone');
matchpathcon('/etc/localtime', 0, &con);
lsetfilecon('/etc/localtime13236236', con);

which is basically ridiculous, right? And it doesn't work. Obviously. But it *does* work if you do this - the only difference being to remove the '../' from the init_prefix call:

security_context_t con;
matchpathcon_init_prefix(NULL, '/usr/share/zoneinfo/some/time/zone');
matchpathcon('/etc/localtime', 0, &con);
lsetfilecon('/etc/localtime13236236', con);

doing that is enough to fix the initial bug here. Even though it looks completely wrong. You can do matchpathcon on /etc/localtime after doing matchpathcon_init_prefix on /usr/share/zoneinfo/blah/blah, and you get the correct value. It really really does work that way, I tested it several times. So, that's what first caused me to report this bug.

Comment 7 Adam Williamson 2015-05-08 16:29:37 UTC
So I wrote a quick test program to demonstrate this bug, and another one I found: lgetfilecon() doesn't work if the .bin files haven't been created. Here it is:

#include <selinux/selinux.h>
#include <stdio.h>

#define LOCALTIME_PATH "/etc/localtime"
#define ZONEINFO_PATH "/usr/share/zoneinfo"

void main() {
    security_context_t con;
    lgetfilecon(LOCALTIME_PATH, &con);
    printf("lgetfilecon: %s\n", con);

    matchpathcon_init_prefix(NULL, LOCALTIME_PATH);
    matchpathcon(LOCALTIME_PATH, 0, &con);
    printf("matchpathcon (%s): %s\n", LOCALTIME_PATH, con);

    matchpathcon_init_prefix(NULL, ZONEINFO_PATH);
    matchpathcon(LOCALTIME_PATH, 0, &con);
    printf("matchpathcon (%s): %s\n", ZONEINFO_PATH, con);
}

compile with 'gcc -lselinux'. On a system with the .bin files, you get:

lgetfilecon: system_u:object_r:locale_t:s0
matchpathcon (/etc/localtime): system_u:object_r:locale_t:s0
matchpathcon (/usr/share/zoneinfo): system_u:object_r:locale_t:s0

on a system without the .bin files, you get:

lgetfilecon: system_u:object_r:default_t:s0
matchpathcon (/etc/localtime): system_u:object_r:default_t:s0
matchpathcon (/usr/share/zoneinfo): system_u:object_r:locale_t:s0

But note, 'matchpathcon -p' does not behave the same on the no-.bin-files system:

[test@localhost ~]$ matchpathcon -p /etc/localtime /etc/localtime
/etc/localtime	system_u:object_r:locale_t:s0
[test@localhost ~]$ matchpathcon -p /usr/share/zoneinfo /etc/localtime
/etc/localtime	system_u:object_r:default_t:s0

Comment 8 Stephen Smalley 2015-05-08 16:35:47 UTC
> lgetfilecon: system_u:object_r:default_t:s0
That can't be right.  lgetfilecon just calls lgetxattr() on the file; it has nothing to do with file_contexts.bin or matchpathcon at all.

I do see the difference in matchpathcon(/etc/localtime) output when I run it locally though.

matchpathcon -p appears to be calling lstat() on the file and passing the mode to matchpathcon() rather than passing 0, but passing 0 is supposed to match any mode.

Comment 9 Adam Williamson 2015-05-08 16:52:27 UTC
"That can't be right.  lgetfilecon just calls lgetxattr() on the file; it has nothing to do with file_contexts.bin or matchpathcon at all."

OK, so talking about the .bin files is kind of wrong. It's probably something *else* that usually happens but hasn't happened yet, right after you install from a live image.

There's a bunch of detail to look into there and I don't know how far you want to get into it, but one simple element I can tell you is that selinux-policy has this:

%triggerin -- pcre
selinuxenabled && semodule -nB
exit 0

%triggerin is interesting, but the upshot is that that *doesn't* run on live image creation (because selinux-policy gets installed before pcre) and hence when you install from the live image, it has not run, but it *does* run if you update or reinstall selinux-policy once you've installed the live image.

Running semodule -nB produces the .bin files - but maybe it does something else too.

Anyway, it's relatively easy to play with this if you want to - just grab an F22 live image, e.g. https://dl.fedoraproject.org/pub/alt/stage/22_TC3/Workstation/x86_64/iso/Fedora-Live-Workstation-x86_64-22-TC3.iso , install from it, and poke around. You obviously know selinux much better than me :), so maybe you can spot what else is different between the 'immediately post-live install' environment and a 'typical' environment that would change lgetfilecon's behaviour. But still, if you're interested in looking into that one, I guess I should file it separately, as it's clearly not the same thing as this.

Comment 10 Stephen Smalley 2015-05-08 16:59:27 UTC
Sorry, what I mean is that merely removing the .bin files won't change lgetfilecon() results, as those merely reflect what was actually set on the file.  But, since removing the .bin files does change the matchpathcon() result, and since the program is setting the file context based on the matchpathcon() result, it makes sense that running lgetfilecon() after an install that did not include the .bin files would have a different result.  Anyway, the issue is the difference in matchpathcon() result.

Comment 11 Stephen Smalley 2015-05-08 17:23:10 UTC
Added some instrumentation to libselinux:

matchpathcon:  Lookup /etc/localtime
selabel_lookup_common:  Lookup /usr/share/zoneinfo/America/New_York
lookup_common:  Lookup /usr/share/zoneinfo/America/New_York
Matched /.* on /usr/share/zoneinfo/America/New_York (stem none)
matchpathcon (/etc/localtime): system_u:object_r:default_t:s0
matchpathcon:  Lookup /etc/localtime
selabel_lookup_common:  Lookup /usr/share/zoneinfo/America/New_York
lookup_common:  Lookup /usr/share/zoneinfo/America/New_York
Matched /usr/share/zoneinfo(/.*)? on /share/zoneinfo/America/New_York (stem /usr)
matchpathcon (/usr/share/zoneinfo): system_u:object_r:locale_t:s0

At some point RH helpfully added code to libselinux matchpathcon() to call realpath() on the pathname?

commit 7df397d3d916e7018981b9fcf8062f992b4cec49
Author: Eric Paris <eparis>
Date:   Wed Aug 17 11:24:25 2011 -0400

    libselinux: move realpath helper to matchpathcon library
    
    Instead of only doing path simplification and symlink following for the
    matchpathcon helper instead do it in the library potion.  This was an
    issue when in python some called selinux.matchpatchcon("//lib64", 0) and
    got the wrong answer (because the // wasn't being dealt with)
    
    Signed-off-by: Eric Paris <eparis>
    Acked-by: Dan Walsh <dwalsh>

Comment 12 Adam Williamson 2015-05-08 18:50:22 UTC
oh, duh, yeah, the lgetfilecon() is just getting its actual context, not its expected context according to policy. silly me. Let's drop that out entirely.

So, what you're saying is that the matchpathcon() call resolves the symlink and tries to find the correct context for the target, right, rather than the correct context for the symlink itself?

So that almost explains it, except check this:

#include <selinux/selinux.h>
#include <stdio.h>

#define LOCALTIME_PATH "/etc/localtime"
#define ZONEINFO_PATH "/usr/share/zoneinfo"
#define SOME_OTHER_PATH "/usr/lib/nagios/cgi"

void main() {
    security_context_t con;

    matchpathcon_init_prefix(NULL, LOCALTIME_PATH);
    matchpathcon(LOCALTIME_PATH, 0, &con);
    printf("matchpathcon (prefix %s): %s\n", LOCALTIME_PATH, con);
    matchpathcon_fini();

    matchpathcon_init_prefix(NULL, ZONEINFO_PATH);
    matchpathcon(LOCALTIME_PATH, 0, &con);
    printf("matchpathcon (prefix %s): %s\n", ZONEINFO_PATH, con);
    matchpathcon_fini();

    matchpathcon_init_prefix(NULL, SOME_OTHER_PATH);
    matchpathcon(LOCALTIME_PATH, 0, &con);
    printf("matchpathcon (prefix %s): %s\n", SOME_OTHER_PATH, con);
    matchpathcon_fini();
}

[test@localhost ~]$ ./selinux_test 
matchpathcon (prefix /etc/localtime): system_u:object_r:default_t:s0
matchpathcon (prefix /usr/share/zoneinfo): system_u:object_r:locale_t:s0
matchpathcon (prefix /usr/lib/nagios/cgi): system_u:object_r:locale_t:s0

why does it work with the /usr/lib/nagios/cgi prefix? Note, however, it doesn't work if I use some path under /bin or /var. Is it that it loads all contexts that start with /usr , so when you ask it to matchpathcon /etc/localtime it resolves it to /usr/share/zoneinfo/(whatever) and it has that loaded? I admit I don't quite understand what the phrase "entries  with  regular  expressions  that have stems prefixed by prefix" is intended to mean, but it doesn't read like it can possibly mean "when the prefix is /usr/share/zoneinfo, load all entries that start with /usr".

Comment 13 Stephen Smalley 2015-05-08 19:17:23 UTC
That appears to be what it is doing.  Remember that this was originally for udev to skip everything but /dev entries when loading file_contexts due to the cost of regex compiling all the file_contexts entries.

Aside from being poorly documented (or poorly implemented, take your pick), matchpathcon_init_prefix() is obsolete given file_contexts.bin support; that avoids the overhead associated with loading all the file_contexts entries, particularly the regex compilation time.

matchpathcon() should never have been changed to call realpath() internally IMHO.
selabel_lookup() does not do that.  Obviously the caller can do that if it is appropriate to do so, but it shouldn't be assumed that the caller wants to label the target and not the symlink itself.

And all of matchpathcon*() was supposed to be obsoleted by selabel*().

Comment 14 Adam Williamson 2015-05-08 19:22:19 UTC
OK, thanks. So at least we understand what's going on now. I guess it's up to you what the sensible thing to do in response to this in libselinux is :)

We have some possible directions for timedatex to go in too in the other bug, so I think we're fine.

Comment 15 Stephen Smalley 2015-05-08 19:59:35 UTC
Also, it appears that the realpath() is not applied to any terminal symlink in the pathname if you pass in S_IFLNK as the mode argument to matchpathcon().  It seems that they assumed everyone would lstat() the file and pass in the mode or otherwise specify the actual mode rather than passing the mode 0 (match anything) value when they added this realpath() support.

As noted elsewhere though, selinux.restorecon('/etc/localtime') would work after rename.

Comment 16 Adam Williamson 2015-05-08 20:17:50 UTC
Aha - indeed, if I change the test program to pass S_IFLNK instead of 0, it behaves as I would have expected and as the command-line utility does.

Comment 17 Stephen Smalley 2015-05-11 15:03:25 UTC
Created attachment 1024252 [details]
matchpathcon/selabel_file:  Fix man pages

Upstream commit 26e05da0fc2d0a4bd274320968a88f8acbb3b6a6.
Just updates the man pages to match reality.
Whether or not we can change the behavior to be saner while preserving compatibility is another question.

Comment 18 Adam Williamson 2015-05-11 16:24:12 UTC
Looks good, except you might want to note that _init() apparently loads all contexts if the .bin files exist?

Comment 19 Stephen Smalley 2015-05-11 16:30:29 UTC
You don't think that's covered by "However, this optimization is no longer necessary due to the use of file_contexts.bin files with precompiled regular expressions, so use of this interface is deprecated."?

Comment 20 Adam Williamson 2015-05-11 17:15:16 UTC
not entirely, it doesn't cover what actually happens if you *do* use it in that case. though it seems like a minor point, it seems unlikely that anyone's actually relying on some contexts *not* being loaded.

Comment 21 Fedora Update System 2015-05-25 08:43:39 UTC
libselinux-2.3-10.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/libselinux-2.3-10.fc22

Comment 22 Fedora Update System 2015-05-25 08:44:00 UTC
libselinux-2.3-10.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/libselinux-2.3-10.fc21

Comment 23 Fedora Update System 2015-05-27 16:10:18 UTC
Package libselinux-2.3-10.fc22:
* should fix your issue,
* was pushed to the Fedora 22 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing libselinux-2.3-10.fc22'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2015-8872/libselinux-2.3-10.fc22
then log in and leave karma (feedback).

Comment 24 Fedora Update System 2015-06-01 17:01:14 UTC
libselinux-2.3-10.fc22 has been pushed to the Fedora 22 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2015-07-29 01:57:06 UTC
libselinux-2.3-10.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.