[PATCH v3 0/2] restorecon context validation improvement

[PATCH v3 0/2] restorecon context validation improvement

[Yuli Khodorkovskiy]

In permissive, if a bad label is written to a file_context file,
restorecon will not verify the label before succesfully applying the
context. These patches fix validation of labels during restorecon
while not breaking current behavior of lazy validation.

Changes since V1:
- Continue using lazy validation for restorecon that was broken in V1 of
the patch.
- Add line number tracking for error messages in restorecon.

Changes since V2:
- Fix compiler error caused by unused variable in selabel_validate()

Yuli Khodorkovskiy (2):
  libselinux: verify file_contexts when using restorecon
  libselinux: echo line number of bad label in selabel_fini()

 libselinux/src/label.c                  | 7 +++----
 libselinux/src/label_backends_android.c | 2 +-
 libselinux/src/label_file.c             | 2 +-
 libselinux/src/label_file.h             | 3 ++-
 libselinux/src/label_internal.h         | 7 +++----
 libselinux/src/matchpathcon.c           | 5 ++---
 6 files changed, 12 insertions(+), 14 deletions(-)

-- 
2.14.3

[PATCH v3 1/2] libselinux: verify file_contexts when using restorecon

[Yuli Khodorkovskiy]

In permissive mode, calling restorecon with a bad label in file_contexts
does not verify the label's existence in the loaded policy. This
results in any label successfully applying to a file, as long as the
file exists.

This issue has two assumptions:

1) file_contexts must be manually updated with the invalid label.
Running `semanage fcontext` will error when attempting to add
an invalid label to file_contexts.
2) the system must be in permissive. Although applying an invalid label
in enforcing gives an error and fails, successfully labeling a file with a
bad label could cause issues during policy development in permissive.

Instead, as each context is used, verify it is valid before blindly
applying the label. If an error with validation occurs in restorecon,
application of remaining valid labels will be uninterrupted as before.

Signed-off-by: Yuli Khodorkovskiy <ykhodo@gmail.com>
---
 libselinux/src/label.c                  | 7 +++----
 libselinux/src/label_backends_android.c | 2 +-
 libselinux/src/label_file.c             | 2 +-
 libselinux/src/label_file.h             | 2 +-
 libselinux/src/label_internal.h         | 6 ++----
 libselinux/src/matchpathcon.c           | 5 ++---
 6 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index 48f4d2d6..c510edc1 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -121,12 +121,11 @@ static inline int selabel_is_validate_set(const struct selinux_opt *opts,
 	return 0;
 }
 
-int selabel_validate(struct selabel_handle *rec,
-		     struct selabel_lookup_rec *contexts)
+int selabel_validate(struct selabel_lookup_rec *contexts)
 {
 	int rc = 0;
 
-	if (!rec->validating || contexts->validated)
+	if (contexts->validated)
 		goto out;
 
 	rc = selinux_validate(&contexts->ctx_raw);
@@ -143,7 +142,7 @@ static int selabel_fini(struct selabel_handle *rec,
 			    struct selabel_lookup_rec *lr,
 			    int translating)
 {
-	if (compat_validate(rec, lr, rec->spec_file, 0))
+	if (compat_validate(lr, rec->spec_file, 0))
 		return -1;
 
 	if (translating && !lr->ctx_trans &&
diff --git a/libselinux/src/label_backends_android.c b/libselinux/src/label_backends_android.c
index cb8aae26..49e39ec8 100644
--- a/libselinux/src/label_backends_android.c
+++ b/libselinux/src/label_backends_android.c
@@ -122,7 +122,7 @@ static int process_line(struct selabel_handle *rec,
 		spec_arr[nspec].lr.ctx_raw = context;
 
 		if (rec->validating) {
-			if (selabel_validate(rec, &spec_arr[nspec].lr) < 0) {
+			if (selabel_validate(&spec_arr[nspec].lr) < 0) {
 				selinux_log(SELINUX_ERROR,
 					    "%s:  line %u has invalid context %s\n",
 					    path, lineno, spec_arr[nspec].lr.ctx_raw);
diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index 560d8c3d..169fed70 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -328,7 +328,7 @@ end_arch_check:
 		spec->lr.ctx_raw = str_buf;
 
 		if (strcmp(spec->lr.ctx_raw, "<<none>>") && rec->validating) {
-			if (selabel_validate(rec, &spec->lr) < 0) {
+			if (selabel_validate(&spec->lr) < 0) {
 				selinux_log(SELINUX_ERROR,
 					    "%s: context %s is invalid\n",
 					    path, spec->lr.ctx_raw);
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index aa576d8e..9e52a3c4 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -506,7 +506,7 @@ static inline int process_line(struct selabel_handle *rec,
 	spec_hasMetaChars(&spec_arr[nspec]);
 
 	if (strcmp(context, "<<none>>") && rec->validating)
-		return compat_validate(rec, &spec_arr[nspec].lr, path, lineno);
+		return compat_validate(&spec_arr[nspec].lr, path, lineno);
 
 	return 0;
 }
diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h
index c55efb75..75451858 100644
--- a/libselinux/src/label_internal.h
+++ b/libselinux/src/label_internal.h
@@ -111,8 +111,7 @@ struct selabel_handle {
  * Validation function
  */
 extern int
-selabel_validate(struct selabel_handle *rec,
-		 struct selabel_lookup_rec *contexts) hidden;
+selabel_validate(struct selabel_lookup_rec *contexts) hidden;
 
 /*
  * Compatibility support
@@ -127,8 +126,7 @@ extern void __attribute__ ((format(printf, 1, 2)))
 		selinux_log(type, fmt);
 
 extern int
-compat_validate(struct selabel_handle *rec,
-		struct selabel_lookup_rec *contexts,
+compat_validate(struct selabel_lookup_rec *contexts,
 		const char *path, unsigned lineno) hidden;
 
 /*
diff --git a/libselinux/src/matchpathcon.c b/libselinux/src/matchpathcon.c
index 58b4144a..c66739af 100644
--- a/libselinux/src/matchpathcon.c
+++ b/libselinux/src/matchpathcon.c
@@ -35,8 +35,7 @@ void set_matchpathcon_printf(void (*f) (const char *fmt, ...))
 	myprintf_compat = 1;
 }
 
-int compat_validate(struct selabel_handle *rec,
-		    struct selabel_lookup_rec *contexts,
+int compat_validate(struct selabel_lookup_rec *contexts,
 		    const char *path, unsigned lineno)
 {
 	int rc;
@@ -47,7 +46,7 @@ int compat_validate(struct selabel_handle *rec,
 	else if (mycanoncon)
 		rc = mycanoncon(path, lineno, ctx);
 	else {
-		rc = selabel_validate(rec, contexts);
+		rc = selabel_validate(contexts);
 		if (rc < 0) {
 			if (lineno) {
 				COMPAT_LOG(SELINUX_WARNING,
-- 
2.14.3

[PATCH v3 2/2] libselinux: echo line number of bad label in selabel_fini()

[Yuli Khodorkovskiy]

Keep track of line numbers for each file context in
selabel_handle. If an error occurs in selabel_fini(), the
line number of an invalid file context is echoed to the user.

Signed-off-by: Yuli Khodorkovskiy <ykhodo@gmail.com>
---
 libselinux/src/label.c          | 2 +-
 libselinux/src/label_file.h     | 1 +
 libselinux/src/label_internal.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index c510edc1..591815a7 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -142,7 +142,7 @@ static int selabel_fini(struct selabel_handle *rec,
 			    struct selabel_lookup_rec *lr,
 			    int translating)
 {
-	if (compat_validate(lr, rec->spec_file, 0))
+	if (compat_validate(lr, rec->spec_file, lr->lineno))
 		return -1;
 
 	if (translating && !lr->ctx_trans &&
diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index 9e52a3c4..3f9ce53b 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -472,6 +472,7 @@ static inline int process_line(struct selabel_handle *rec,
 	spec_arr[nspec].mode = 0;
 
 	spec_arr[nspec].lr.ctx_raw = context;
+	spec_arr[nspec].lr.lineno = lineno;
 
 	/*
 	 * bump data->nspecs to cause closef() to cover it in its free
diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h
index 75451858..b0d05882 100644
--- a/libselinux/src/label_internal.h
+++ b/libselinux/src/label_internal.h
@@ -73,6 +73,7 @@ struct selabel_lookup_rec {
 	char * ctx_raw;
 	char * ctx_trans;
 	int validated;
+	unsigned lineno;
 };
 
 struct selabel_handle {
-- 
2.14.3

Re: [PATCH v3 0/2] restorecon context validation improvement

[William Roberts]

On Thu, Mar 29, 2018 at 5:16 PM, Yuli Khodorkovskiy <ykhodo@gmail.com> wrote:
> In permissive, if a bad label is written to a file_context file,
> restorecon will not verify the label before succesfully applying the
> context. These patches fix validation of labels during restorecon
> while not breaking current behavior of lazy validation.
>
> Changes since V1:
> - Continue using lazy validation for restorecon that was broken in V1 of
> the patch.
> - Add line number tracking for error messages in restorecon.
>
> Changes since V2:
> - Fix compiler error caused by unused variable in selabel_validate()
>
> Yuli Khodorkovskiy (2):
>   libselinux: verify file_contexts when using restorecon
>   libselinux: echo line number of bad label in selabel_fini()
>
>  libselinux/src/label.c                  | 7 +++----
>  libselinux/src/label_backends_android.c | 2 +-
>  libselinux/src/label_file.c             | 2 +-
>  libselinux/src/label_file.h             | 3 ++-
>  libselinux/src/label_internal.h         | 7 +++----
>  libselinux/src/matchpathcon.c           | 5 ++---
>  6 files changed, 12 insertions(+), 14 deletions(-)
>
> --
> 2.14.3
>
>

These look good to me and pass all my testing. I have them on
github passing CI as well:
https://github.com/SELinuxProject/selinux/pull/90

ack. Unless someone finds an issue, will merge
on 4/3.

Re: [PATCH v3 0/2] restorecon context validation improvement

[William Roberts]

On Fri, Mar 30, 2018 at 11:59 AM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> On Thu, Mar 29, 2018 at 5:16 PM, Yuli Khodorkovskiy <ykhodo@gmail.com> wrote:
>> In permissive, if a bad label is written to a file_context file,
>> restorecon will not verify the label before succesfully applying the
>> context. These patches fix validation of labels during restorecon
>> while not breaking current behavior of lazy validation.
>>
>> Changes since V1:
>> - Continue using lazy validation for restorecon that was broken in V1 of
>> the patch.
>> - Add line number tracking for error messages in restorecon.
>>
>> Changes since V2:
>> - Fix compiler error caused by unused variable in selabel_validate()
>>
>> Yuli Khodorkovskiy (2):
>>   libselinux: verify file_contexts when using restorecon
>>   libselinux: echo line number of bad label in selabel_fini()
>>
>>  libselinux/src/label.c                  | 7 +++----
>>  libselinux/src/label_backends_android.c | 2 +-
>>  libselinux/src/label_file.c             | 2 +-
>>  libselinux/src/label_file.h             | 3 ++-
>>  libselinux/src/label_internal.h         | 7 +++----
>>  libselinux/src/matchpathcon.c           | 5 ++---
>>  6 files changed, 12 insertions(+), 14 deletions(-)
>>
>> --
>> 2.14.3
>>
>>
>
> These look good to me and pass all my testing. I have them on
> github passing CI as well:
> https://github.com/SELinuxProject/selinux/pull/90
>
> ack. Unless someone finds an issue, will merge
> on 4/3.

merged. Thank you.