[PATCH 1/6] semodule_package: do not leak memory when using -u or -s

[PATCH 1/6] semodule_package: do not leak memory when using -u or -s

[Nicolas Iooss]

When using -u and -s options, semodule_package's main() allocates
user_extra and seusers to hold the argument values. These allocated
memory blocks are not freed when main() exits, which leads gcc's Address
Sanitizer to report a memory leak. This occurs for example when building
refpolicy base.pp module.

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

diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c
index e472054826a3..a25daf59f9e7 100644
--- a/semodule-utils/semodule_package/semodule_package.c
+++ b/semodule-utils/semodule_package/semodule_package.c
@@ -257,5 +257,7 @@ int main(int argc, char **argv)
 	free(file_contexts);
 	free(outfile);
 	free(module);
+	free(seusers);
+	free(user_extra);
 	exit(0);
 }
-- 
2.11.1

[PATCH 2/6] libsepol/cil: do not dereference args before checking it was not null

[Nicolas Iooss]

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

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 5e5298d98e61..87817ca29a5f 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -3313,11 +3313,12 @@ int __cil_resolve_ast_node(struct cil_tree_node *node, void *extra_args)
 	int rc = SEPOL_OK;
 	struct cil_args_resolve *args = extra_args;
 	enum cil_pass pass = 0;
-	struct cil_list *ins = args->in_list;
+	struct cil_list *ins;
 
 	if (node == NULL || args == NULL) {
 		goto exit;
 	}
+	ins = args->in_list;
 
 	pass = args->pass;
 	switch (pass) {
-- 
2.11.1

[PATCH 3/6] libsemanage: never call memcpy with a NULL value

[Nicolas Iooss]

clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().

Make append_str() do nothing when called with t=NULL.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsemanage/src/semanage_store.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index f468faba4b64..47ec93185e06 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1194,8 +1194,14 @@ static char *append(char *s, char c)
 static char *append_str(char *s, const char *t)
 {
 	size_t s_len = (s == NULL ? 0 : strlen(s));
-	size_t t_len = (t == NULL ? 0 : strlen(t));
-	char *new_s = realloc(s, s_len + t_len + 1);
+	size_t t_len;
+	char *new_s;
+
+	if (t == NULL) {
+		return s;
+	}
+	t_len = strlen(t);
+	new_s = realloc(s, s_len + t_len + 1);
 	if (new_s == NULL) {
 		return NULL;
 	}
-- 
2.11.1

[PATCH 4/6] libsemanage/tests: include libsepol headers from $DESTDIR

[Nicolas Iooss]

When building and running tests on a system without SELinux with a
command similar to "make DESTDIR=/tmp/destdir install test", libsemanage
tests fail to build with the following error:

    In file included from utilities.h:20:0,
                     from utilities.c:24:
    ../src/handle.h:29:26: fatal error: sepol/handle.h: No such file or
    directory
     #include <sepol/handle.h>
                              ^

Fix this by adding the newly-installed directory under $DESTDIR (using
variable $PREFIX) in the search paths of the compiler.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 .travis.yml                | 2 --
 libsemanage/tests/Makefile | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 7d7424459344..fa25d1b336ac 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -105,14 +105,12 @@ script:
   - make all -k
 
   # Set up environment variables for the tests
-  - export CFLAGS="-I$DESTDIR/usr/include"
   - export LD_LIBRARY_PATH="$DESTDIR/usr/lib:$DESTDIR/lib"
   - export PATH="$DESTDIR/usr/sbin:$DESTDIR/usr/bin:$DESTDIR/sbin:$DESTDIR/bin:$PATH"
   - export PYTHONPATH="$PYSITEDIR"
   - export RUBYLIB="$DESTDIR/$($RUBY -e 'puts RbConfig::CONFIG["vendorlibdir"]'):$DESTDIR/$($RUBY -e 'puts RbConfig::CONFIG["vendorarchdir"]')"
 
   # Show variables (to help debugging issues)
-  - echo "$CFLAGS"
   - echo "$LD_LIBRARY_PATH"
   - echo "$PATH"
   - echo "$PYTHONPATH"
diff --git a/libsemanage/tests/Makefile b/libsemanage/tests/Makefile
index 3fe7d16f3e72..885b54430155 100644
--- a/libsemanage/tests/Makefile
+++ b/libsemanage/tests/Makefile
@@ -11,7 +11,7 @@ LIBS = ../src/libsemanage.a -L$(LIBDIR) -lselinux -lsepol
 
 EXECUTABLE = libsemanage-tests
 CFLAGS += -g -O0 -Wall -W -Wundef -Wmissing-noreturn -Wmissing-format-attribute -Wno-unused-parameter
-INCLUDE = -I../src -I../include
+INCLUDE = -I../src -I../include -I$(PREFIX)/include
 LDFLAGS += -lcunit -lbz2 -laudit
 OBJECTS = $(SOURCES:.c=.o) 
 
-- 
2.11.1

[PATCH 5/6] mcstrans: do not dereference color_str if it is NULL

[Nicolas Iooss]

This bug has been found using clang static analyzer.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 mcstrans/src/mcscolor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mcstrans/src/mcscolor.c b/mcstrans/src/mcscolor.c
index e713da9aea1f..cc6174bba9d5 100644
--- a/mcstrans/src/mcscolor.c
+++ b/mcstrans/src/mcscolor.c
@@ -292,7 +292,7 @@ int raw_color(const security_context_t raw, char **color_str) {
 	size_t result_size = (N_COLOR * CHARS_PER_COLOR) + 1;
 	int rc = -1;
 
-	if (!color_str && !*color_str) {
+	if (!color_str || !*color_str) {
 		return -1;
 	}
 
-- 
2.11.1

[PATCH 6/6] libselinux: initialize temp value in SWIG wrapper to prevent freeing garbage

[Nicolas Iooss]

Currently this Python program triggers a segmentation fault in
libselinux SWIG wrapper:

    import selinux
    selinux.get_ordered_context_list()

gdb shows that the segmentation fault occurs when freeing some memory:

    Reading symbols from python...(no debugging symbols found)...done.
    Starting program: /usr/bin/python -c import\
    selinux\;selinux.get_ordered_context_list\(\)
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/usr/lib/libthread_db.so.1".

    Program received signal SIGSEGV, Segmentation fault.
    0x00007ffff789a304 in free () from /usr/lib/libc.so.6
    (gdb) bt
    #0  0x00007ffff789a304 in free () from /usr/lib/libc.so.6
    #1  0x00007ffff6011499 in freeconary (con=0x7ffff6ac5d00) at
    freeconary.c:14
    #2  0x00007ffff6296899 in _wrap_get_ordered_context_list
    (self=<optimized out>, args=<optimized out>) at
    selinuxswig_wrap.c:6185
    #3  0x00007ffff741891f in _PyCFunction_FastCallDict () from
    /usr/lib/libpython3.6m.so.1.0
    ...

SWIG generated the following code for _wrap_get_ordered_context_list():

    char ***arg3 = (char ***) 0 ;
    char **temp3 ;
    arg3 = &temp3;
    if (!PyArg_ParseTuple(args, "OO:get_ordered_context_list",&obj0,&obj1))
        SWIG_fail;
    /* ... */
  fail:
    if (*arg3) freeconary(*arg3);

If PyArg_ParseTuple fails, freeconary() is called on the value of
"temp3", which has not been initialized. Fix this by initializing temp
to NULL in the SWIG template.

A similar issue exists with security_get_boolean_names(). Fix it too.

This issue has been found using clang's static analyzer, on a system
which uses SWIG 3.0.12.

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

diff --git a/libselinux/src/selinuxswig.i b/libselinux/src/selinuxswig.i
index 687c43bc6d7d..dbdb4c3d72d4 100644
--- a/libselinux/src/selinuxswig.i
+++ b/libselinux/src/selinuxswig.i
@@ -18,7 +18,7 @@
 %typedef unsigned mode_t;
 %typedef unsigned pid_t;
 
-%typemap(in, numinputs=0) (char ***names, int *len) (char **temp1, int temp2) {
+%typemap(in, numinputs=0) (char ***names, int *len) (char **temp1=NULL, int temp2) {
 	$1 = &temp1;
 	$2 = &temp2;
 }
@@ -33,7 +33,7 @@
 	}
 }
 
-%typemap(in, numinputs=0) (char ***) (char **temp) {
+%typemap(in, numinputs=0) (char ***) (char **temp=NULL) {
 	$1 = &temp;
 }
 
-- 
2.11.1

Re: [PATCH 3/6] libsemanage: never call memcpy with a NULL value

[William Roberts]

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

On Feb 27, 2017 12:42, "Nicolas Iooss" <nicolas.iooss@m4x.org> wrote:

clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().

Make append_str() do nothing when called with t=NULL.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsemanage/src/semanage_store.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_
store.c
index f468faba4b64..47ec93185e06 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1194,8 +1194,14 @@ static char *append(char *s, char c)
 static char *append_str(char *s, const char *t)
 {
        size_t s_len = (s == NULL ? 0 : strlen(s));
-       size_t t_len = (t == NULL ? 0 : strlen(t));
-       char *new_s = realloc(s, s_len + t_len + 1);
+       size_t t_len;
+       char *new_s;
+
+       if (t == NULL) {
+               return s;
+       }
+       t_len = strlen(t);
+       new_s = realloc(s, s_len + t_len + 1);


Overflow possibility here?

        if (new_s == NULL) {
                return NULL;
        }
--
2.11.1

_______________________________________________
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.

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

Re: [PATCH 3/6] libsemanage: never call memcpy with a NULL value

[William Roberts]

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

On Feb 27, 2017 2:16 PM, "William Roberts" <bill.c.roberts@gmail.com> wrote:



On Feb 27, 2017 12:42, "Nicolas Iooss" <nicolas.iooss@m4x.org> wrote:

clang's static analyzer reports "Argument with 'nonnull' attribute
passed null" in append_str(), because argument t may be NULL but is used
in a call to memcpy().

Make append_str() do nothing when called with t=NULL.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsemanage/src/semanage_store.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_sto
re.c
index f468faba4b64..47ec93185e06 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -1194,8 +1194,14 @@ static char *append(char *s, char c)
 static char *append_str(char *s, const char *t)
 {
        size_t s_len = (s == NULL ? 0 : strlen(s));
-       size_t t_len = (t == NULL ? 0 : strlen(t));
-       char *new_s = realloc(s, s_len + t_len + 1);
+       size_t t_len;
+       char *new_s;
+
+       if (t == NULL) {
+               return s;
+       }
+       t_len = strlen(t);
+       new_s = realloc(s, s_len + t_len + 1);


Overflow possibility here?


I guess since s and t lengths come from strlen() and the architectures we
worry about running code on, overflowing would be pretty impossible here.


        if (new_s == NULL) {
                return NULL;
        }
--
2.11.1

_______________________________________________
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.

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

Re: [PATCH 1/6] semodule_package: do not leak memory when using -u or -s

[James Carter]

On 02/27/2017 03:39 PM, Nicolas Iooss wrote:
> When using -u and -s options, semodule_package's main() allocates
> user_extra and seusers to hold the argument values. These allocated
> memory blocks are not freed when main() exits, which leads gcc's Address
> Sanitizer to report a memory leak. This occurs for example when building
> refpolicy base.pp module.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

I applied all six of these patches.

Thanks,
Jim

> ---
>  semodule-utils/semodule_package/semodule_package.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/semodule-utils/semodule_package/semodule_package.c b/semodule-utils/semodule_package/semodule_package.c
> index e472054826a3..a25daf59f9e7 100644
> --- a/semodule-utils/semodule_package/semodule_package.c
> +++ b/semodule-utils/semodule_package/semodule_package.c
> @@ -257,5 +257,7 @@ int main(int argc, char **argv)
>  	free(file_contexts);
>  	free(outfile);
>  	free(module);
> +	free(seusers);
> +	free(user_extra);
>  	exit(0);
>  }
>


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