[PATCH 1/5] libsepol: do not dereference NULL if stack_init fails

[PATCH 1/5] libsepol: do not dereference NULL if stack_init fails

[Nicolas Iooss]

In cond_expr_to_cil(), when stack_init() fails to allocate a stack, the
function calls stack_pop() with stack = NULL. Then stack_pop()
dereferences the pointer ("if (stack->pos == -1) {"), which is NULL.

Fix this by moving the stack cleaning loop in a "if (stack != NULL)"
block.

This issue is reported by clang's static analyzer with the following
message:

    module_to_cil.c:463:6: warning: Access to field 'pos' results in a
    dereference of a null pointer (loaded from variable 'stack')
        if (stack->pos == -1) {
            ^~~~~~~~~~

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/module_to_cil.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 5b8ed19eaa14..c6f1659c84ef 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -1917,10 +1917,12 @@ exit:
 	free(new_val);
 	free(val1);
 	free(val2);
-	while ((val1 = stack_pop(stack)) != NULL) {
-		free(val1);
+	if (stack != NULL) {
+		while ((val1 = stack_pop(stack)) != NULL) {
+			free(val1);
+		}
+		stack_destroy(&stack);
 	}
-	stack_destroy(&stack);
 
 	return rc;
 }
-- 
2.17.0

[PATCH 2/5] libsepol: ensure the level context is not empty

[Nicolas Iooss]

When load_users() parses an invalid line with an empty level context
(ie. nothing between "level" and "range" keywords), it allocates memory
with malloc(0) and uses it. The behavior of malloc() in this case is
an unspecified behavior: it might return NULL, which would lead to a
segmentation fault.

Fix this issue by reporting the invalid entry instead. While at it,
ensure that the character before "range" is a space, and change the
logic slightly in order to avoid using "--p; ... p++;".

This issue is reported by clang's static analyzer with the following
message:

    genusers.c:222:11: warning: Use of zero-allocated memory
                                            *r++ = *s;
                                                 ^
    genusers.c:225:7: warning: Use of zero-allocated memory
                            *r = 0;
                               ^

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/src/genusers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsepol/src/genusers.c b/libsepol/src/genusers.c
index 556821066e3e..9bea83fd8304 100644
--- a/libsepol/src/genusers.c
+++ b/libsepol/src/genusers.c
@@ -201,11 +201,11 @@ static int load_users(struct policydb *policydb, const char *path)
 			if (!(*p))
 				BADLINE();
 			q = p;
-			while (*p && strncasecmp(p, "range", 5))
+			while (*p && (!isspace(*p) || strncasecmp(p + 1, "range", 5)))
 				p++;
-			if (!(*p))
+			if (!(*p) || p == q)
 				BADLINE();
-			*--p = 0;
+			*p = 0;
 			p++;
 
 			scontext = malloc(p - q);
-- 
2.17.0

[PATCH 3/5] libselinux: label_file: fix memory management in store_stem()

[Nicolas Iooss]

If store_stem() fails to expand the memory allocated on data->stem_arr,
some things go wrong:
* the memory referenced by "buf" is leaked,
* data->alloc_stems has been increased without data->stem_arr having
  been expanded. So the next time store_stem() is called, the function
  will behave as if the buffer holds enough space, and will write data
  after the end of data->stem_arr.

The first issue is being spotted by clang's static analyzer, which warns
about leaking variable "stem" in find_stem_from_spec() (this function
calls store_stem()).

This both issues by freeing buf when realloc(data->stem_arr) fails, and
by not increasing data->alloc_stems when this happens.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/src/label_file.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index 3f9ce53b7ffe..1ab139e962f2 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -278,12 +278,14 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)
 
 	if (data->alloc_stems == num) {
 		struct stem *tmp_arr;
-
-		data->alloc_stems = data->alloc_stems * 2 + 16;
+		int alloc_stems = data->alloc_stems * 2 + 16;
 		tmp_arr = realloc(data->stem_arr,
-				  sizeof(*tmp_arr) * data->alloc_stems);
-		if (!tmp_arr)
+				  sizeof(*tmp_arr) * alloc_stems);
+		if (!tmp_arr) {
+			free(buf);
 			return -1;
+		}
+		data->alloc_stems = alloc_stems;
 		data->stem_arr = tmp_arr;
 	}
 	data->stem_arr[num].len = stem_len;
-- 
2.17.0

[PATCH 4/5] libselinux: fix memory leak in getconlist

[Nicolas Iooss]

In getconlist.c's main(), "level" is duplicated from an optional
argument without being ever freed. clang's static analyzer warns about
this memory leak.

Free the allocated memory properly in order to remove a warning reported
by clang's static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/utils/getconlist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index adec1781658b..abfe2c742bfb 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -40,6 +40,7 @@ int main(int argc, char **argv)
 	if (!is_selinux_enabled()) {
 		fprintf(stderr,
 			"getconlist may be used only on a SELinux kernel.\n");
+		free(level);
 		return 1;
 	}
 
@@ -49,6 +50,7 @@ int main(int argc, char **argv)
 	if (((argc - optind) < 2)) {
 		if (getcon(&cur_context) < 0) {
 			fprintf(stderr, "Couldn't get current context.\n");
+			free(level);
 			return 2;
 		}
 	} else
@@ -68,6 +70,7 @@ int main(int argc, char **argv)
 	}
 
 	free(usercon);
+	free(level);
 
 	return 0;
 }
-- 
2.17.0

[PATCH 5/5] libselinux: remove unused variable usercon

[Nicolas Iooss]

In getconlist.c, main() does not use usercon. Remove this variable.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libselinux/utils/getconlist.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index abfe2c742bfb..5ac0ca85075c 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
 
 int main(int argc, char **argv)
 {
-	char **list, *usercon = NULL, *cur_context = NULL;
+	char **list, *cur_context = NULL;
 	char *user = NULL, *level = NULL;
 	int ret, i, opt;
 
@@ -69,7 +69,6 @@ int main(int argc, char **argv)
 		freeconary(list);
 	}
 
-	free(usercon);
 	free(level);
 
 	return 0;
-- 
2.17.0

Re: [PATCH 5/5] libselinux: remove unused variable usercon

[William Roberts]

In general this series looks fine.

However, checkpatch.pl is complaining about DOS line endings in your patches:

For example:
ERROR: DOS line endings
#325: FILE: libselinux/src/label_file.h:281:
+^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$





On Fri, Apr 13, 2018 at 1:34 PM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> In getconlist.c, main() does not use usercon. Remove this variable.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libselinux/utils/getconlist.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
> index abfe2c742bfb..5ac0ca85075c 100644
> --- a/libselinux/utils/getconlist.c
> +++ b/libselinux/utils/getconlist.c
> @@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
>
>  int main(int argc, char **argv)
>  {
> -       char **list, *usercon = NULL, *cur_context = NULL;
> +       char **list, *cur_context = NULL;
>         char *user = NULL, *level = NULL;
>         int ret, i, opt;
>
> @@ -69,7 +69,6 @@ int main(int argc, char **argv)
>                 freeconary(list);
>         }
>
> -       free(usercon);
>         free(level);
>
>         return 0;
> --
> 2.17.0
>
>

Re: [PATCH 5/5] libselinux: remove unused variable usercon

[Nicolas Iooss]

On Sat, Apr 14, 2018 at 2:40 AM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> In general this series looks fine.
>
> However, checkpatch.pl is complaining about DOS line endings in your patches:
>
> For example:
> ERROR: DOS line endings
> #325: FILE: libselinux/src/label_file.h:281:
> +^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

This is unexpected. On my computer, "file" gives the following output
on the .patch files (there is no "..., with CRLF line terminators"):

  0001-libsepol-do-not-dereference-NULL-if-stack_init-fails.patch:
unified diff output, ASCII text
  0002-libsepol-ensure-the-level-context-is-not-empty.patch:
unified diff output, ASCII text
  0003-libselinux-label_file-fix-memory-management-in-store.patch:
unified diff output, ASCII text
  0004-libselinux-fix-memory-leak-in-getconlist.patch:
unified diff output, ASCII text
  0005-libselinux-remove-unused-variable-usercon.patch:
unified diff output, ASCII text

Nevertheless, my messages seem to use CR+LF line endings. I am
wondering whether this is a new feature of "git send-email" or of my
SMTP server mangling text emails... Anyway, "git am *.patch && git
format-patch origin/master" produces files with LF line endings, so
git seems to handle fine this kind of modification.
By the way, I have also pushed these patches on my Github repository,
https://github.com/fishilico/selinux/commits/92717db1cb9dd6c8faf8fae3325f6a9d4c1002ca
.

Thanks!
Nicolas

Re: [PATCH 5/5] libselinux: remove unused variable usercon

[Stephen Smalley]

On 04/13/2018 08:40 PM, William Roberts wrote:
> In general this series looks fine.
> 
> However, checkpatch.pl is complaining about DOS line endings in your patches:
> 
> For example:
> ERROR: DOS line endings
> #325: FILE: libselinux/src/label_file.h:281:
> +^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

If needed, dos2unix can be used to strip them. However, I think git am takes care of this for you.

> 
> 
> 
> 
> 
> On Fri, Apr 13, 2018 at 1:34 PM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>> In getconlist.c, main() does not use usercon. Remove this variable.
>>
>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>> ---
>>  libselinux/utils/getconlist.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
>> index abfe2c742bfb..5ac0ca85075c 100644
>> --- a/libselinux/utils/getconlist.c
>> +++ b/libselinux/utils/getconlist.c
>> @@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
>>
>>  int main(int argc, char **argv)
>>  {
>> -       char **list, *usercon = NULL, *cur_context = NULL;
>> +       char **list, *cur_context = NULL;
>>         char *user = NULL, *level = NULL;
>>         int ret, i, opt;
>>
>> @@ -69,7 +69,6 @@ int main(int argc, char **argv)
>>                 freeconary(list);
>>         }
>>
>> -       free(usercon);
>>         free(level);
>>
>>         return 0;
>> --
>> 2.17.0
>>
>>
> 

Re: [PATCH 5/5] libselinux: remove unused variable usercon

[William Roberts]

On Mon, Apr 16, 2018 at 5:34 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 04/13/2018 08:40 PM, William Roberts wrote:
>> In general this series looks fine.
>>
>> However, checkpatch.pl is complaining about DOS line endings in your patches:
>>
>> For example:
>> ERROR: DOS line endings
>> #325: FILE: libselinux/src/label_file.h:281:
>> +^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$
>
> If needed, dos2unix can be used to strip them. However, I think git am takes care of this for you.
>

FYI your patches are staged here:
https://github.com/SELinuxProject/selinux/pull/93

If no one nacks them, ill merge latter this week. Thanks.

>>
>>
>>
>>
>>
>> On Fri, Apr 13, 2018 at 1:34 PM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>>> In getconlist.c, main() does not use usercon. Remove this variable.
>>>
>>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>>> ---
>>>  libselinux/utils/getconlist.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
>>> index abfe2c742bfb..5ac0ca85075c 100644
>>> --- a/libselinux/utils/getconlist.c
>>> +++ b/libselinux/utils/getconlist.c
>>> @@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
>>>
>>>  int main(int argc, char **argv)
>>>  {
>>> -       char **list, *usercon = NULL, *cur_context = NULL;
>>> +       char **list, *cur_context = NULL;
>>>         char *user = NULL, *level = NULL;
>>>         int ret, i, opt;
>>>
>>> @@ -69,7 +69,6 @@ int main(int argc, char **argv)
>>>                 freeconary(list);
>>>         }
>>>
>>> -       free(usercon);
>>>         free(level);
>>>
>>>         return 0;
>>> --
>>> 2.17.0
>>>
>>>
>>
>

Re: [PATCH 5/5] libselinux: remove unused variable usercon

[jwcart2]

On 04/16/2018 11:30 AM, William Roberts wrote:
> On Mon, Apr 16, 2018 at 5:34 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 04/13/2018 08:40 PM, William Roberts wrote:
>>> In general this series looks fine.
>>>
>>> However, checkpatch.pl is complaining about DOS line endings in your patches:
>>>
>>> For example:
>>> ERROR: DOS line endings
>>> #325: FILE: libselinux/src/label_file.h:281:
>>> +^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$
>>
>> If needed, dos2unix can be used to strip them. However, I think git am takes care of this for you.
>>
> 
> FYI your patches are staged here:
> https://github.com/SELinuxProject/selinux/pull/93
> 
> If no one nacks them, ill merge latter this week. Thanks.
> 

These patches look good to me.

Thanks,
Jim

>>>
>>>
>>>
>>>
>>>
>>> On Fri, Apr 13, 2018 at 1:34 PM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>>>> In getconlist.c, main() does not use usercon. Remove this variable.
>>>>
>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>>>> ---
>>>>   libselinux/utils/getconlist.c | 3 +--
>>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
>>>> index abfe2c742bfb..5ac0ca85075c 100644
>>>> --- a/libselinux/utils/getconlist.c
>>>> +++ b/libselinux/utils/getconlist.c
>>>> @@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
>>>>
>>>>   int main(int argc, char **argv)
>>>>   {
>>>> -       char **list, *usercon = NULL, *cur_context = NULL;
>>>> +       char **list, *cur_context = NULL;
>>>>          char *user = NULL, *level = NULL;
>>>>          int ret, i, opt;
>>>>
>>>> @@ -69,7 +69,6 @@ int main(int argc, char **argv)
>>>>                  freeconary(list);
>>>>          }
>>>>
>>>> -       free(usercon);
>>>>          free(level);
>>>>
>>>>          return 0;
>>>> --
>>>> 2.17.0
>>>>
>>>>
>>>
>>
> 
> 


-- 
James Carter <jwcart2@tycho.nsa.gov>
National Security Agency