All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] restorecon context validation improvement
@ 2018-03-29  3:40 Yuli Khodorkovskiy
  2018-03-29  3:40 ` [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon Yuli Khodorkovskiy
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Yuli Khodorkovskiy @ 2018-03-29  3:40 UTC (permalink / raw)
  To: selinux

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.

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

 libselinux/src/label.c          | 4 ++--
 libselinux/src/label_file.h     | 1 +
 libselinux/src/label_internal.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.14.3

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon
  2018-03-29  3:40 [PATCH v2 0/2] restorecon context validation improvement Yuli Khodorkovskiy
@ 2018-03-29  3:40 ` Yuli Khodorkovskiy
  2018-03-29 12:37   ` Stephen Smalley
  2018-03-29  3:40 ` [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini() Yuli Khodorkovskiy
  2018-03-29  5:00 ` [PATCH v2 0/2] restorecon context validation improvement William Roberts
  2 siblings, 1 reply; 10+ messages in thread
From: Yuli Khodorkovskiy @ 2018-03-29  3:40 UTC (permalink / raw)
  To: selinux

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libselinux/src/label.c b/libselinux/src/label.c
index 48f4d2d6..e642a97b 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -126,7 +126,7 @@ int selabel_validate(struct selabel_handle *rec,
 {
 	int rc = 0;
 
-	if (!rec->validating || contexts->validated)
+	if (contexts->validated)
 		goto out;
 
 	rc = selinux_validate(&contexts->ctx_raw);
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini()
  2018-03-29  3:40 [PATCH v2 0/2] restorecon context validation improvement Yuli Khodorkovskiy
  2018-03-29  3:40 ` [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon Yuli Khodorkovskiy
@ 2018-03-29  3:40 ` Yuli Khodorkovskiy
  2018-03-29 13:49   ` Stephen Smalley
  2018-03-29  5:00 ` [PATCH v2 0/2] restorecon context validation improvement William Roberts
  2 siblings, 1 reply; 10+ messages in thread
From: Yuli Khodorkovskiy @ 2018-03-29  3:40 UTC (permalink / raw)
  To: selinux

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 e642a97b..d9a58ce9 100644
--- a/libselinux/src/label.c
+++ b/libselinux/src/label.c
@@ -143,7 +143,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(rec, 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 aa576d8e..4780ae48 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 c55efb75..0e020557 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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/2] restorecon context validation improvement
  2018-03-29  3:40 [PATCH v2 0/2] restorecon context validation improvement Yuli Khodorkovskiy
  2018-03-29  3:40 ` [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon Yuli Khodorkovskiy
  2018-03-29  3:40 ` [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini() Yuli Khodorkovskiy
@ 2018-03-29  5:00 ` William Roberts
  2 siblings, 0 replies; 10+ messages in thread
From: William Roberts @ 2018-03-29  5:00 UTC (permalink / raw)
  To: Yuli Khodorkovskiy; +Cc: selinux

On Wed, Mar 28, 2018 at 8:40 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.
>
> Yuli Khodorkovskiy (2):
>   libselinux: verify file_contexts when using restorecon
>   libselinux: echo line number of bad label in selabel_fini()
>
>  libselinux/src/label.c          | 4 ++--
>  libselinux/src/label_file.h     | 1 +
>  libselinux/src/label_internal.h | 1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> --
> 2.14.3
>
>

ack, LGTM.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon
  2018-03-29  3:40 ` [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon Yuli Khodorkovskiy
@ 2018-03-29 12:37   ` Stephen Smalley
  2018-03-29 16:17     ` William Roberts
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2018-03-29 12:37 UTC (permalink / raw)
  To: Yuli Khodorkovskiy, selinux

On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote:
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libselinux/src/label.c b/libselinux/src/label.c
> index 48f4d2d6..e642a97b 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -126,7 +126,7 @@ int selabel_validate(struct selabel_handle *rec,
>  {
>  	int rc = 0;
>  
> -	if (!rec->validating || contexts->validated)
> +	if (contexts->validated)
>  		goto out;
>  
>  	rc = selinux_validate(&contexts->ctx_raw);
> 

label.c: In function ‘selabel_validate’:
label.c:124:45: error: unused parameter ‘rec’ [-Werror=unused-parameter]
 int selabel_validate(struct selabel_handle *rec,
                                             ^~~
Need to drop the rec argument to selabel_validate() since it is no longer used.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini()
  2018-03-29  3:40 ` [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini() Yuli Khodorkovskiy
@ 2018-03-29 13:49   ` Stephen Smalley
  2018-03-29 15:48     ` Yuli Khodorkovskiy
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2018-03-29 13:49 UTC (permalink / raw)
  To: Yuli Khodorkovskiy, selinux

On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote:
> 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 e642a97b..d9a58ce9 100644
> --- a/libselinux/src/label.c
> +++ b/libselinux/src/label.c
> @@ -143,7 +143,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(rec, 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 aa576d8e..4780ae48 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 c55efb75..0e020557 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 {
> 

I think this is ok, but wanted to double check: does this work correctly when file contexts are loaded from
file_contexts.bin instead?  It looks to me as if the lineno will be left as 0 in that case and the
code will handle that correctly.

The other question is whether we correctly report the file name when the entry comes from a file other
than file_contexts itself, e.g. file_contexts.local, .homedirs, ...  Not your bug if we don't but wondered.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini()
  2018-03-29 13:49   ` Stephen Smalley
@ 2018-03-29 15:48     ` Yuli Khodorkovskiy
  2018-03-29 16:35       ` Stephen Smalley
  0 siblings, 1 reply; 10+ messages in thread
From: Yuli Khodorkovskiy @ 2018-03-29 15:48 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: selinux

[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]

On Thu, Mar 29, 2018 at 9:49 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:

> On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote:
> > 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 e642a97b..d9a58ce9 100644
> > --- a/libselinux/src/label.c
> > +++ b/libselinux/src/label.c
> > @@ -143,7 +143,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(rec, 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 aa576d8e..4780ae48 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 c55efb75..0e020557 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 {
> >
>
> I think this is ok, but wanted to double check: does this work correctly
> when file contexts are loaded from
> file_contexts.bin instead?  It looks to me as if the lineno will be left
> as 0 in that case and the
> code will handle that correctly.
>

Compiling a file_contexts.bin with sefcontext_compile will give you the
line number:

sefcontext_compile /etc/selinux/targeted/contexts/files/file_contexts -o
file_contexts.bin -p /etc/selinux/targeted/policy/policy.31
libsepol.sepol_context_from_string: malformed context "test"
libsepol.sepol_context_from_string: could not construct context from string
libsepol.context_from_string: could not create context structure
libsepol.sepol_context_to_sid: could not convert test to sid
/etc/selinux/targeted/contexts/files/file_contexts: line 6132 has invalid
context test
sefcontext_compile: process_file failed

Using file_contexts.bin for relabeling that I generated with no validation
will not report a line number:

restorecon: /etc/selinux/targeted/contexts/files/file_contexts: has invalid
context test

I'll see if I can associate the line number with each regex in
sefcontext_compile.



>
> The other question is whether we correctly report the file name when the
> entry comes from a file other
> than file_contexts itself, e.g. file_contexts.local, .homedirs, ...  Not
> your bug if we don't but wondered.
>

It is reporting the line number correctly, but the filename is incorrect.
I'll update this.

[-- Attachment #2: Type: text/html, Size: 4789 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon
  2018-03-29 12:37   ` Stephen Smalley
@ 2018-03-29 16:17     ` William Roberts
  0 siblings, 0 replies; 10+ messages in thread
From: William Roberts @ 2018-03-29 16:17 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Yuli Khodorkovskiy, selinux

On Thu, Mar 29, 2018 at 5:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote:
>> 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 | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libselinux/src/label.c b/libselinux/src/label.c
>> index 48f4d2d6..e642a97b 100644
>> --- a/libselinux/src/label.c
>> +++ b/libselinux/src/label.c
>> @@ -126,7 +126,7 @@ int selabel_validate(struct selabel_handle *rec,
>>  {
>>       int rc = 0;
>>
>> -     if (!rec->validating || contexts->validated)
>> +     if (contexts->validated)
>>               goto out;
>>
>>       rc = selinux_validate(&contexts->ctx_raw);
>>
>
> label.c: In function ‘selabel_validate’:
> label.c:124:45: error: unused parameter ‘rec’ [-Werror=unused-parameter]
>  int selabel_validate(struct selabel_handle *rec,
>                                              ^~~
> Need to drop the rec argument to selabel_validate() since it is no longer used.

I figured with -Wall -Werror we would be good. For some reason, I
can't reproduce with my
ancient version of gcc. gcc versions 5 and above properly reports this. Weird.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini()
  2018-03-29 15:48     ` Yuli Khodorkovskiy
@ 2018-03-29 16:35       ` Stephen Smalley
  2018-03-29 16:37         ` Yuli Khodorkovskiy
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2018-03-29 16:35 UTC (permalink / raw)
  To: Yuli Khodorkovskiy; +Cc: selinux

On 03/29/2018 11:48 AM, Yuli Khodorkovskiy wrote:
> 
> 
> On Thu, Mar 29, 2018 at 9:49 AM, Stephen Smalley <sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>> wrote:
> 
>     On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote:
>     > 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 <mailto: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 e642a97b..d9a58ce9 100644
>     > --- a/libselinux/src/label.c
>     > +++ b/libselinux/src/label.c
>     > @@ -143,7 +143,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(rec, 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 aa576d8e..4780ae48 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 c55efb75..0e020557 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 {
>     >
> 
>     I think this is ok, but wanted to double check: does this work correctly when file contexts are loaded from
>     file_contexts.bin instead?  It looks to me as if the lineno will be left as 0 in that case and the
>     code will handle that correctly.
> 
> 
> Compiling a file_contexts.bin with sefcontext_compile will give you the line number:
> 
> sefcontext_compile /etc/selinux/targeted/contexts/files/file_contexts -o file_contexts.bin -p /etc/selinux/targeted/policy/policy.31
> libsepol.sepol_context_from_string: malformed context "test"
> libsepol.sepol_context_from_string: could not construct context from string
> libsepol.context_from_string: could not create context structure
> libsepol.sepol_context_to_sid: could not convert test to sid
> /etc/selinux/targeted/contexts/files/file_contexts: line 6132 has invalid context test
> sefcontext_compile: process_file failed
> 
> Using file_contexts.bin for relabeling that I generated with no validation will not report a line number:
> 
> restorecon: /etc/selinux/targeted/contexts/files/file_contexts: has invalid context test
> 
> I'll see if I can associate the line number with each regex in sefcontext_compile.

That's fine if you want to do it but not necessary for these patches to be merged; I just wanted to ensure that we don't have garbage output as a result of these patches.  I'd do it as a separate patch regardless since it likely requires a new binary format version for the file_contexts.bin files. 

>     The other question is whether we correctly report the file name when the entry comes from a file other
>     than file_contexts itself, e.g. file_contexts.local, .homedirs, ...  Not your bug if we don't but wondered.
> 
> 
> It is reporting the line number correctly, but the filename is incorrect. I'll update this.

Again, not strictly necessary for these patches since you didn't introduce the bug, and probably ought to be its own separate patch.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini()
  2018-03-29 16:35       ` Stephen Smalley
@ 2018-03-29 16:37         ` Yuli Khodorkovskiy
  0 siblings, 0 replies; 10+ messages in thread
From: Yuli Khodorkovskiy @ 2018-03-29 16:37 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: selinux

Alright, then I'll resubmit with a fix for the compiler warnings and
do the rest of the enhancements as a separate patch set.

On Thu, Mar 29, 2018 at 12:35 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 03/29/2018 11:48 AM, Yuli Khodorkovskiy wrote:
>>
>>
>> On Thu, Mar 29, 2018 at 9:49 AM, Stephen Smalley <sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>> wrote:
>>
>>     On 03/28/2018 11:40 PM, Yuli Khodorkovskiy wrote:
>>     > 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 <mailto: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 e642a97b..d9a58ce9 100644
>>     > --- a/libselinux/src/label.c
>>     > +++ b/libselinux/src/label.c
>>     > @@ -143,7 +143,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(rec, 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 aa576d8e..4780ae48 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 c55efb75..0e020557 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 {
>>     >
>>
>>     I think this is ok, but wanted to double check: does this work correctly when file contexts are loaded from
>>     file_contexts.bin instead?  It looks to me as if the lineno will be left as 0 in that case and the
>>     code will handle that correctly.
>>
>>
>> Compiling a file_contexts.bin with sefcontext_compile will give you the line number:
>>
>> sefcontext_compile /etc/selinux/targeted/contexts/files/file_contexts -o file_contexts.bin -p /etc/selinux/targeted/policy/policy.31
>> libsepol.sepol_context_from_string: malformed context "test"
>> libsepol.sepol_context_from_string: could not construct context from string
>> libsepol.context_from_string: could not create context structure
>> libsepol.sepol_context_to_sid: could not convert test to sid
>> /etc/selinux/targeted/contexts/files/file_contexts: line 6132 has invalid context test
>> sefcontext_compile: process_file failed
>>
>> Using file_contexts.bin for relabeling that I generated with no validation will not report a line number:
>>
>> restorecon: /etc/selinux/targeted/contexts/files/file_contexts: has invalid context test
>>
>> I'll see if I can associate the line number with each regex in sefcontext_compile.
>
> That's fine if you want to do it but not necessary for these patches to be merged; I just wanted to ensure that we don't have garbage output as a result of these patches.  I'd do it as a separate patch regardless since it likely requires a new binary format version for the file_contexts.bin files.
>
>>     The other question is whether we correctly report the file name when the entry comes from a file other
>>     than file_contexts itself, e.g. file_contexts.local, .homedirs, ...  Not your bug if we don't but wondered.
>>
>>
>> It is reporting the line number correctly, but the filename is incorrect. I'll update this.
>
> Again, not strictly necessary for these patches since you didn't introduce the bug, and probably ought to be its own separate patch.
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-03-29 16:37 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-29  3:40 [PATCH v2 0/2] restorecon context validation improvement Yuli Khodorkovskiy
2018-03-29  3:40 ` [PATCH v2 1/2] libselinux: verify file_contexts when using restorecon Yuli Khodorkovskiy
2018-03-29 12:37   ` Stephen Smalley
2018-03-29 16:17     ` William Roberts
2018-03-29  3:40 ` [PATCH v2 2/2] libselinux: echo line number of bad label in selabel_fini() Yuli Khodorkovskiy
2018-03-29 13:49   ` Stephen Smalley
2018-03-29 15:48     ` Yuli Khodorkovskiy
2018-03-29 16:35       ` Stephen Smalley
2018-03-29 16:37         ` Yuli Khodorkovskiy
2018-03-29  5:00 ` [PATCH v2 0/2] restorecon context validation improvement William Roberts

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.