Bug 891547

Summary: Labelling of postgresql_log_t does not cause correct labels right away
Product: [Fedora] Fedora Reporter: Jan Pazdziora <jpazdziora>
Component: selinux-policyAssignee: Miroslav Grepl <mgrepl>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 18CC: devrim, dominick.grift, dwalsh, hhorak, jpazdziora, mgrepl
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-01-02 17:35:52 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Jan Pazdziora 2013-01-03 08:51:50 UTC
Description of problem:

While working on completely unrelated issue, I did run

# restorecon -rvvn /var/lib

The output was

restorecon reset /var/lib/pgsql/data/pg_log/postgresql-Thu.log context system_u:object_r:postgresql_db_t:s0->system_u:object_r:postgresql_log_t:s0

This is strange as I did not manually tweak the labels. I believe that in the course of normal PostgreSQL server operation, it should not create files that don't match the defined file contexts.

Version-Release number of selected component (if applicable):

# rpm -q postgresql-server selinux-policy-targeted
postgresql-server-9.1.7-1.fc17.i686
selinux-policy-targeted-3.10.0-161.fc17.noarch
#

How reproducible:

Seen once, looking at /etc/selinux/targeted/contexts/files/file_contexts it's deterministic.

Steps to Reproduce:
1. # service postgresql start
2. # sleep 5
3. # restorecon -rvvn /var/lib/pgsql
  
Actual results:

# service postgresql start
Redirecting to /bin/systemctl start  postgresql.service
# sleep 5
# restorecon -rvvn /var/lib/pgsql
restorecon reset /var/lib/pgsql/data/pg_log/postgresql-Thu.log context system_u:object_r:postgresql_db_t:s0->system_u:object_r:postgresql_log_t:s0
#

Expected results:

# service postgresql start
Redirecting to /bin/systemctl start  postgresql.service
# sleep 5
# restorecon -rvvn /var/lib/pgsql
#

Additional info:

My proposed solution would be to label the directory

# ls -laZ /var/lib/pgsql/data/pg_log
drwx------. postgres postgres unconfined_u:object_r:postgresql_db_t:s0 .
drwx------. postgres postgres system_u:object_r:postgresql_db_t:s0 ..
-rw-------. postgres postgres system_u:object_r:postgresql_db_t:s0 postgresql-Thu.log

with postgresql_log_t and have the files inherit that type.

The strange thing is that while /var/lib/pgsql/data is owned by the postgresql-server-9.1.7-1.fc17.i686 package, none of the subdirectories are, so rpm would not do the labelling for us. Would it hurt to package the /var/lib/pgsql/data/pg_log by postgresql-server as well? Alternatively, /usr/bin/postgresql-setup could be patched to restorecon the directory after it mkdir-ed (and chown-ed and chmod-ed) it.

Comment 1 Miroslav Grepl 2013-01-03 11:21:08 UTC
Jan,
good catch. Also you are right with the solution

/var/lib/pgsql/data/pg_log

should be owned by postgresql-server.


Also we might want to fix labeling which we define

/var/lib/pgsql/.*\.log          gen_context(system_u:object_r:postgresql_log_t,s0)

Comment 2 Miroslav Grepl 2013-01-03 11:30:58 UTC
Is there another location for log files in the /var/lib/pgsql?

Comment 3 Honza Horak 2013-01-03 13:38:42 UTC
(In reply to comment #2)
> Is there another location for log files in the /var/lib/pgsql?

AFAIK no. All log files are located only in /var/lib/pgsql/data/pg_log/*.log .

(In reply to comment #1)
> Jan,
> good catch. Also you are right with the solution
> 
> /var/lib/pgsql/data/pg_log
> 
> should be owned by postgresql-server.

I don't see any connection between RPM ownership of directory and SELinux context. What do I miss?

> Also we might want to fix labeling which we define
> 
> /var/lib/pgsql/.*\.log         
> gen_context(system_u:object_r:postgresql_log_t,s0)

This should be enough to solve the labelling issue, right?

Comment 4 Jan Pazdziora 2013-01-03 13:47:54 UTC
(In reply to comment #3)
> > 
> > /var/lib/pgsql/data/pg_log
> > 
> > should be owned by postgresql-server.
> 
> I don't see any connection between RPM ownership of directory and SELinux
> context. What do I miss?

If the package owns the directory, rpm will create it, and it will label it correctly.

> > Also we might want to fix labeling which we define
> > 
> > /var/lib/pgsql/.*\.log         
> > gen_context(system_u:object_r:postgresql_log_t,s0)
> 
> This should be enough to solve the labelling issue, right?

No. You need to restorecon as well. Either you will restorecon after mkdiring that directory in postgresql-setup, or you make the package own the directory and let rpm do the restorecon for you.

Comment 5 Honza Horak 2013-01-03 14:30:58 UTC
(In reply to comment #4)
> No. You need to restorecon as well. Either you will restorecon after
> mkdiring that directory in postgresql-setup, or you make the package own the
> directory and let rpm do the restorecon for you.

I still don't understand why we need to do relabelling -- if there is correct context definition, mkdir should create directory with right context, shouldn't it?

Comment 6 Miroslav Grepl 2013-01-03 14:40:49 UTC
If you add a directory to rpm payload then rpm takes care about labeling which we define in the policy.



Generally a dir created by mkdir gets the same labeling which has a parent directory.

Of course there are some exceptions. 

So if you just add the /var/lib/pgsql/data/pg_log dir to the spec file and we add a labeling for this dir it will work fine.

Comment 7 Miroslav Grepl 2013-01-03 14:43:24 UTC
I added this change 

commit 56104f254c3801686b3f8ae47c74743a59035261
Author: Miroslav Grepl <mgrepl>
Date:   Thu Jan 3 15:41:24 2013 +0100

    Fix labeling for postgresql log files located in /var/lib/pgsql

diff --git a/policy/modules/services/postgresql.fc b/policy/modules/services/postgresql.fc
index d3cc612..bc10e82 100644
--- a/policy/modules/services/postgresql.fc
+++ b/policy/modules/services/postgresql.fc
@@ -31,7 +31,7 @@ ifdef(`distro_redhat', `
 
 /var/lib/pgsql(/.*)?                   gen_context(system_u:object_r:postgresql_db_t,s0)
 /var/lib/pgsql/logfile(/.*)?           gen_context(system_u:object_r:postgresql_log_t,s0)
-/var/lib/pgsql/.*\.log                 gen_context(system_u:object_r:postgresql_log_t,s0)
+/var/lib/pgsql/data/pg_log(/.*)?       gen_context(system_u:object_r:postgresql_log_t,s0)

to the policy.

Comment 8 Jan Pazdziora 2013-01-03 14:45:04 UTC
(In reply to comment #5)
> 
> I still don't understand why we need to do relabelling -- if there is
> correct context definition, mkdir should create directory with right
> context, shouldn't it?

No, that's not how SELinux works.

Comment 9 Honza Horak 2013-01-03 15:10:33 UTC
(In reply to comment #6)
> Generally a dir created by mkdir gets the same labeling which has a parent
> directory.

This is what I was missing. Thanks for explanation, guys.

Comment 10 Tom Lane 2013-01-03 16:06:42 UTC
(In reply to comment #6)
> So if you just add the /var/lib/pgsql/data/pg_log dir to the spec file and
> we add a labeling for this dir it will work fine.

This is not going to happen.  The data directory is created at initdb time, not at install time, and therefore the pg_log subdirectory is also created at initdb time.

I do notice that postgresql-setup omits to do an explicit restorecon when creating the pg_log directory ... but really, should that be necessary?  I always thought that the restorecons it does for other files were a leftover from broken old versions of selinux.

Comment 11 Jan Pazdziora 2013-01-03 16:25:19 UTC
(In reply to comment #10)
> (In reply to comment #6)
> > So if you just add the /var/lib/pgsql/data/pg_log dir to the spec file and
> > we add a labeling for this dir it will work fine.
> 
> This is not going to happen.  The data directory is created at initdb time,
> not at install time, and therefore the pg_log subdirectory is also created
> at initdb time.

Would initdb choke if the directory already (pre)existed, since rpm installation time? It does not seem to check the exit status of that mkdir.

> I do notice that postgresql-setup omits to do an explicit restorecon when
> creating the pg_log directory ... but really, should that be necessary?  I
> always thought that the restorecons it does for other files were a leftover
> from broken old versions of selinux.

Not sure about broken. Old version of SELinux would inherit the label of the parent directory. New SELinuxes make it possible to create different labels based on the name of the file/directory being created.

Explicit restorecon would certainly feel less hackish because things would behave in deterministic fashion. From long term maintainability point of view, the easiest thing could be to just move the restorecon to the end of the execution of the script, and iron the whole $PGDATA with -r.

Comment 12 Daniel Walsh 2013-01-03 16:55:21 UTC
We have a patch hopefully going into coreutils to do the right thing, 

mkdir -Z /var/lib/pgsql/data/pg_log

==

mkdir /var/lib/pgsql/data/pg_log; restorecon /var/lib/pgsql/data/pg_log

You need to think of SELinux the same way you think of DAC.  If you do something like 

mkdir foobar
chown foobar
or 
chcon foobar
Then from an SELinux point of view the equivalent would be
restorecon foobar

restorecon just says set the label to the system defaults.

In current fedora we can add some better controls for this that say something like

if unconfined_t creates a directory named "pg_log" in a directory labeled postgresql_db_t it will get created as postgresql_log_t, but this does not work in RHEL6.

Comment 13 Tom Lane 2013-01-03 17:01:13 UTC
(In reply to comment #11)
> > This is not going to happen.  The data directory is created at initdb time,
> > not at install time, and therefore the pg_log subdirectory is also created
> > at initdb time.
> 
> Would initdb choke if the directory already (pre)existed, since rpm
> installation time?

Yes, it would, and I will *not* remove initdb's safety check that says the data directory has to be empty.

> Not sure about broken. Old version of SELinux would inherit the label of the
> parent directory. New SELinuxes make it possible to create different labels
> based on the name of the file/directory being created.

It seems pretty darn peculiar that file creation correctly applies the labeling the policy says the file should have, but directory creation doesn't.  If that isn't considered a selinux bug, why not?

> Explicit restorecon would certainly feel less hackish because things would
> behave in deterministic fashion.

I don't see what's deterministic about it.  I hope you don't imagine that every subdirectory that ever will exist under /var/lib/pgsql/data/ will necessarily be made by Fedora-specific code that knows it needs to do a restorecon.  It's pure luck that the particular case here is in a Fedora-supplied script that we can change painlessly.  If we were talking about having to patch a system("restorecon ...") into the C-code guts of the server, I'd be pushing back a whole lot harder than I am (if indeed that would even work, since the server wouldn't be running as root).

The hack here is the need to do restorecon at all.  Why shouldn't we expect the kernel to get it right the first time?  Why is a supposedly guaranteed-secure OS reliant on userspace actions to get files labeled correctly?

Comment 14 Daniel Walsh 2013-01-03 17:14:47 UTC
I just checked in a fix to make this happen in F19/F18 policy by default.

file and directory creation follow the same rules, in that by default they adopt the security label of the parent.  In this case we are saying that the security of the file and directory are different the file is a database file and the directory is a directory which logging data will be stored.  SELinux is attempting to differentiate these, logging tools will be able to do stuff with postgresql_log_t but not allowed to touch postgresql_db_t.

As I have stated we have newer mechanisms to do the "right thing" in latest fedoras and I have updated policy to reflect this.  But as with Ownership/Permissions if we want to separate the permissions on a file/direcory, then the person/tool creating the content needs to fix the permissions by using restorecon/chmod/chown.

Comment 15 Jan Pazdziora 2013-01-03 20:20:32 UTC
(In reply to comment #13)
> 
> > Not sure about broken. Old version of SELinux would inherit the label of the
> > parent directory. New SELinuxes make it possible to create different labels
> > based on the name of the file/directory being created.
> 
> It seems pretty darn peculiar that file creation correctly applies the
> labeling the policy says the file should have,

I'm affraid it does not. The file/directory will get parent's context (type), unless you have transition defined either for invoking process type and parent, or (in Fedoras) for invoking process type and parent and filename (or dirname).

>                                               but directory creation
> doesn't.  If that isn't considered a selinux bug, why not?

Creation of files and of directories behave exactly the same WRT to the context/type.

> > Explicit restorecon would certainly feel less hackish because things would
> > behave in deterministic fashion.
> 
> I don't see what's deterministic about it.  I hope you don't imagine that

It's deterministic because the resulting type (after restorecon) is based on the defined file context, not on the type of the process which creates the file/directory.

> every subdirectory that ever will exist under /var/lib/pgsql/data/ will
> necessarily be made by Fedora-specific code that knows it needs to do a
> restorecon.  It's pure luck that the particular case here is in a

Well, if Fedora-specific means "this particular directory has different type from the parent on Fedora while not on RHEL", then the restorecon -r would easily take care of that -- it will make sure everything is sane. You don't have to remember to do restorecon after individual mkdirs. If you have SUSE or other systems that don't use SELinux in mind, then well -- yes, invocation of restorecon is Fedora/RHEL-specific. But you already have the code in postgresql-setup anyway.

> Fedora-supplied script that we can change painlessly.  If we were talking
> about having to patch a system("restorecon ...") into the C-code guts of the
> server, I'd be pushing back a whole lot harder than I am (if indeed that
> would even work, since the server wouldn't be running as root).

That's why it's probably good to pre-create the directories with correct labels before the postgresql_t processes get started. If you already have pg_log/ with postgresql_log_t, then without any other restorecons, any process which creates file or directory within such directory, will create inode with context/type postgresql_log_t -- inheriting from the parent.

> The hack here is the need to do restorecon at all.  Why shouldn't we expect
> the kernel to get it right the first time?  Why is a supposedly
> guaranteed-secure OS reliant on userspace actions to get files labeled
> correctly?

We are getting into a conceptual discussion of SELinux' merits and tachnology here. ;-) SELinux is based on labels it finds on inodes on the filesystem, not on paths. The default path label => label gets to inodes on disk is an "offline" operation, handled by rpm or restorecon or similar mechanisms.

Comment 16 Jan Pazdziora 2013-01-03 20:29:18 UTC
(In reply to comment #14)
> I just checked in a fix to make this happen in F19/F18 policy by default.

Do you have a commit repo/SHA1?

Comment 17 Daniel Walsh 2013-01-03 21:39:48 UTC
Miroslav will build an RPM when he sees this I am sure, Currently it is just checked into the Fedora SELinux Git Repository.

Comment 18 Tom Lane 2013-01-03 23:44:17 UTC
I stuck a restorecon into postgresql-setup (currently only in F18 and rawhide), but on my F16 work machine that doesn't seem to have any effect: the pg_log subdirectory still ends up with the postgresql_db_t label.  Are we talking about a policy change that was made in F17 or later?

Comment 19 Jan Pazdziora 2013-01-04 07:16:42 UTC
On Fedora 16, 17, the type is defined for

/var/lib/pgsql/.*\.log	system_u:object_r:postgresql_log_t:s0

not for the directory. It's the change from the comment 7 which should make it work right after the mkdir (and then let the files inherit it from the pg_log/).

Comment 20 Miroslav Grepl 2013-01-04 08:30:12 UTC
I backported fixes from F18.

commit 1e6e3d520d9ca0c1bbe0f0b8ee71ce9b53b12bfe
Author: Miroslav Grepl <mgrepl>
Date:   Fri Jan 4 09:27:48 2013 +0100

    Add interface for postgesql_filetrans_name_content to make sure log directories get created with the correct label.


I would like to do a new build today for testing.

Comment 21 Tom Lane 2013-01-04 16:26:55 UTC
I've committed the necessary restorecon call into git for all active Fedora branches, but haven't bothered to rebuild postgresql before F18 since it's of no value until the policy change propagates.  Not sure if we should close this, reassign it back to selinux-policy for that part of the fix, or ...?

Comment 22 Daniel Walsh 2013-01-04 18:01:49 UTC
Lets move this to F18 and selinux-policy.  Once the policy is built it would be great if someone could test it.

Comment 23 Fedora End Of Life 2013-12-21 10:09:52 UTC
This message is a reminder that Fedora 18 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 18. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as WONTFIX if it remains open with a Fedora 
'version' of '18'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version prior to Fedora 18's end of life.

Thank you for reporting this issue and we are sorry that we may not be 
able to fix it before Fedora 18 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior to Fedora 18's end of life.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Comment 24 Jan Pazdziora 2013-12-22 09:34:28 UTC
I've verified on Fedora 20 that the logfile is created with postgresql_log_t. So I assume either CURRENTRELEASE or NEXTRELEASE would be nice?