From: dahchanson@gmail.com (Chad Hanson)
Date: Tue, 12 Dec 2017 00:49:07 -0500
Subject: [refpolicy] [PATCH] Fix implementation of MLS file relabel
 attributes
In-Reply-To: <b4fa0b22-860c-c8f4-5b25-770bb4a8bf95@ieee.org>
References: <20171211040215.53324-1-dahchanson@gmail.com>
	<b4fa0b22-860c-c8f4-5b25-770bb4a8bf95@ieee.org>
Message-ID: <20171212054907.GA117841@localhost.localdomain>
To: refpolicy@oss.tresys.com
List-Id: refpolicy.oss.tresys.com

On Mon, Dec 11, 2017 at 07:01:18PM -0500, Chris PeBenito wrote:
> On 12/10/2017 11:02 PM, Chad Hanson via refpolicy wrote:
> >This patch properly completes the implementation of the MLS file relabel attributes. In the previous patch [http://oss.tresys.com/pipermail/refpolicy/2016-July/008038.html], a new attribute, mlsfilerelabetoclr, was created. There should have been a second attribute, mlsfilerelabel, created instead of overloading mlsfilewrite for this privilege. I concur with creating new attributes for this situation. I have created the patch below.
> >
> >Signed-off-by: Chad Hanson <dahchanson@gmail.com>
> >---
> >  policy/mls                   |  2 +-
> >  policy/modules/kernel/mls.if | 28 ++++++++++++++++++++++++----
> >  policy/modules/kernel/mls.te |  3 ++-
> >  3 files changed, 27 insertions(+), 6 deletions(-)
> >
> >diff --git a/policy/mls b/policy/mls
> >index 2dadd205..73ff301b 100644
> >--- a/policy/mls
> >+++ b/policy/mls
> >@@ -72,7 +72,7 @@ mlsconstrain { file lnk_file fifo_file } { create relabelto }
> >  mlsconstrain { dir file lnk_file chr_file blk_file sock_file fifo_file } relabelto
> >  	(( h1 dom h2 ) or
> >  	(( t1 == mlsfilerelabeltoclr ) and ( h1 dom l2 )) or
> >-	( t1 == mlsfilewrite ));
> >+	( t1 == mlsfilerelabel ));
> 
> It seems like both conditions should actually be included, not the
> mlsfilerelabel replacing the mlsfilewrite.  That way, if you use the
> latter, it's still comprehensive.
> 

I would normally agree, I just don't think mlsfilewrite should ever been
allowed for "relabelto" operations. The combination of a new
mlsfilerelabeltoclr combined with mlsfilewrite doesn't make sense. It
should either have been mlsfilewrite[toclr] or mlsfilerelabel[toclr]. My
personal opinion is that relabel is much different than write. In MLS, a
process can write to a file without having read access. A relabel can
lead to new accesses since this is a file attribute change. This wasn't
bypassable originally, that is why a new attribute seems to make sense.

> >  # the file "read" ops (note the check is dominance of the low level)
> >  mlsconstrain { dir file lnk_file chr_file blk_file sock_file fifo_file } { read getattr execute }
> >diff --git a/policy/modules/kernel/mls.if b/policy/modules/kernel/mls.if
> >index b09c0a5a..2e2bebc2 100644
> >--- a/policy/modules/kernel/mls.if
> >+++ b/policy/modules/kernel/mls.if
> >@@ -71,6 +71,26 @@ interface(`mls_file_write_to_clearance',`
> >  	typeattribute $1 mlsfilewritetoclr;
> >  ')
> >+########################################
> >+## <summary>
> >+##	Make specified domain MLS trusted
> >+##	for writing to files at all levels.
> >+## </summary>
> >+## <param name="domain">
> >+##	<summary>
> >+##	Domain allowed access.
> >+##	</summary>
> >+## </param>
> >+## <rolecap/>
> >+#
> >+interface(`mls_file_write_all_levels',`
> >+	gen_require(`
> >+		attribute mlsfilewrite;
> >+	')
> >+
> >+	typeattribute $1 mlsfilewrite;
> >+')
> >+
> >  ########################################
> >  ## <summary>
> >  ##	Make specified domain MLS trusted
> >@@ -94,7 +114,7 @@ interface(`mls_file_relabel_to_clearance',`
> >  ########################################
> >  ## <summary>
> >  ##	Make specified domain MLS trusted
> >-##	for writing to files at all levels.
> >+##	for relabelto to files at all levels.
> >  ## </summary>
> >  ## <param name="domain">
> >  ##	<summary>
> >@@ -103,12 +123,12 @@ interface(`mls_file_relabel_to_clearance',`
> >  ## </param>
> >  ## <rolecap/>
> >  #
> >-interface(`mls_file_write_all_levels',`
> >+interface(`mls_file_relabel',`
> >  	gen_require(`
> >-		attribute mlsfilewrite;
> >+		attribute mlsfilerelabel;
> >  	')
> >-	typeattribute $1 mlsfilewrite;
> >+	typeattribute $1 mlsfilerelabel;
> >  ')
> 
> Interfaces shouldn't be removed.  Instead they should be
> deprecated.... assuming we go forward with the above change.
> 

I didn't remove an interface here, I just moved one of the new
interfaces to have them together after the mlsfilewrite interfaces
instead of interspersed within. The diff just doesn't reflect that
very well.
 
> >  ########################################
> >diff --git a/policy/modules/kernel/mls.te b/policy/modules/kernel/mls.te
> >index ad74e81f..7c50e75c 100644
> >--- a/policy/modules/kernel/mls.te
> >+++ b/policy/modules/kernel/mls.te
> >@@ -10,9 +10,10 @@ attribute mlsfilereadtoclr;
> >  attribute mlsfilewrite;
> >  attribute mlsfilewritetoclr;
> >  attribute mlsfilewriteinrange;
> >+attribute mlsfilerelabel;
> >+attribute mlsfilerelabeltoclr;
> >  attribute mlsfileupgrade;
> >  attribute mlsfiledowngrade;
> >-attribute mlsfilerelabeltoclr;
> >  attribute mlsnetread;
> >  attribute mlsnetreadtoclr;
> >
> 
> 
> -- 
> Chris PeBenito