[PATCH 5/7] libsemanage: do not close uninitialized file descriptors

2017-03-28 Thread Nicolas Iooss 
When pipe() fails in semanage_pipe_data(), this function closes all file
descriptors in variables output_fd, err_fd and input_fd even when they
have not been initialized. Fix this by initializing the file descriptors
to -1.

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss 
---
 libsemanage/src/direct_api.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index c23494bb4270..568732355f54 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -705,9 +705,9 @@ static int read_from_pipe_to_data(semanage_handle_t *sh, 
size_t initial_len, int
 
 static int semanage_pipe_data(semanage_handle_t *sh, char *path, char 
*in_data, size_t in_data_len, char **out_data, size_t *out_data_len, char 
**err_data, size_t *err_data_len)
 {
-   int input_fd[2];
-   int output_fd[2];
-   int err_fd[2];
+   int input_fd[2] = {-1, -1};
+   int output_fd[2] = {-1, -1};
+   int err_fd[2] = {-1, -1};
pid_t pid;
char *data_read = NULL;
char *err_data_read = NULL;
-- 
2.12.0

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH 1/7] libsepol: do not dereference a NULL pointer when stack_init() fails

2017-03-28 Thread Nicolas Iooss 
In cond_expr_to_cil() when stack_init() fails, stack is set to
NULL and the execution flow jumps to label "exit". This triggers a call
to stack_pop(stack) which dereferences a NULL pointer in "if (stack->pos
== -1)".

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss 
---
 libsepol/src/module_to_cil.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 308ada4f1381..5c98c29bcf13 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -1363,11 +1363,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_destroy();
-
return rc;
 }
 
-- 
2.12.0

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH 2/7] libsepol: make process_boolean() fail on invalid lines

2017-03-28 Thread Nicolas Iooss 
When load_booleans() calls process_boolean() to parse a boolean
definition, process_boolean() returns a successful value when it fails
to use strtok_r() (e.g. when there is no "=" in the parsed line). This
leads load_booleans() to use uninitialized name and/or val when setting
the boolean into the policy.

Rework process_boolean() in order to report errors when a boolean
definition is incorrect.

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss 
---
 libsepol/src/genbools.c | 59 +
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/libsepol/src/genbools.c b/libsepol/src/genbools.c
index c1f540558bf1..d79433531f76 100644
--- a/libsepol/src/genbools.c
+++ b/libsepol/src/genbools.c
@@ -34,31 +34,42 @@ static int process_boolean(char *buffer, char *name, int 
namesize, int *val)
 {
char name1[BUFSIZ];
char *ptr = NULL;
-   char *tok = strtok_r(buffer, "=", );
-   if (tok) {
-   strncpy(name1, tok, BUFSIZ - 1);
-   strtrim(name, name1, namesize - 1);
-   if (name[0] == '#')
-   return 0;
-   tok = strtok_r(NULL, "\0", );
-   if (tok) {
-   while (isspace(*tok))
-   tok++;
-   *val = -1;
-   if (isdigit(tok[0]))
-   *val = atoi(tok);
-   else if (!strncasecmp(tok, "true", sizeof("true") - 1))
-   *val = 1;
-   else if (!strncasecmp
-(tok, "false", sizeof("false") - 1))
-   *val = 0;
-   if (*val != 0 && *val != 1) {
-   ERR(NULL, "illegal value for boolean "
-   "%s=%s", name, tok);
-   return -1;
-   }
+   char *tok;
+
+   /* Skip spaces */
+   while (isspace(buffer[0]))
+   buffer++;
+   /* Ignore comments */
+   if (buffer[0] == '#')
+   return 0;
+
+   tok = strtok_r(buffer, "=", );
+   if (!tok) {
+   ERR(NULL, "illegal boolean definition %s", buffer);
+   return -1;
+   }
+   strncpy(name1, tok, BUFSIZ - 1);
+   strtrim(name, name1, namesize - 1);
 
-   }
+   tok = strtok_r(NULL, "\0", );
+   if (!tok) {
+   ERR(NULL, "illegal boolean definition %s=%s", name, buffer);
+   return -1;
+   }
+
+   while (isspace(*tok))
+   tok++;
+
+   *val = -1;
+   if (isdigit(tok[0]))
+   *val = atoi(tok);
+   else if (!strncasecmp(tok, "true", sizeof("true") - 1))
+   *val = 1;
+   else if (!strncasecmp(tok, "false", sizeof("false") - 1))
+   *val = 0;
+   if (*val != 0 && *val != 1) {
+   ERR(NULL, "illegal value for boolean %s=%s", name, tok);
+   return -1;
}
return 1;
 }
-- 
2.12.0

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] libsepol: In module_to_cil create one attribute for each unique set

2017-03-28 Thread Nicolas Iooss 
On Tue, Mar 28, 2017 at 7:28 PM, James Carter  wrote:
> CIL does not allow type or role sets in certain rules (such as allow
> rules). It does, however, allow sets in typeattributeset and
> roleattributeset statements. Because of this, when module_to_cil
> translates a policy into CIL, it creates a new attribute for each
> set that it encounters. But often the same set is used multiple times
> which means that more attributes are created then necessary. As the
> number of attributes increases the time required for the kernel to
> make each policy decision increases which can be a problem.
>
> To help reduce the number of attributes in a kernel policy,
> when module_to_cil encounters a role or type set search to see if the
> set was encountered already and, if it was, use the previously
> generated attribute instead of creating a new one.
>
> Testing on Android and Refpolicy policies show that this reduces the
> number of attributes generated by about 40%.
>
> Signed-off-by: James Carter 
> ---
>  libsepol/src/module_to_cil.c | 593 
> +--
>  1 file changed, 283 insertions(+), 310 deletions(-)
>
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 6c33b94..4ea8a83 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
>
> [...]
>
> +static char *get_new_attr_name(struct policydb *pdb, int is_type)
>  {
> static unsigned int num_attrs = 0;
> -   int rc = -1;
> int len, rlen;
> -   const char *attr_infix;
> -   char *attr;
> +   char *infix;
> +   char *attr_name = NULL;

Why is infix "char *" instead of "const char *", like attr_infix was?
I am seeing a compiler warning with -Wwrite-strings ("error:
assignment discards ‘const’ qualifier from pointer target type" on
"infix = TYPEATTR_INFIX").

Cheers,
Nicolas


___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] mcstrans: fix typo in mcstransd.8 man page

2017-03-28 Thread James Carter 

On 03/24/2017 10:27 AM, Nikola Forró wrote:

Signed-off-by: Nikola Forró 


Applied.

Thanks,
Jim


---
 mcstrans/man/man8/mcstransd.8 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mcstrans/man/man8/mcstransd.8 b/mcstrans/man/man8/mcstransd.8
index c1dc483..64774a5 100644
--- a/mcstrans/man/man8/mcstransd.8
+++ b/mcstrans/man/man8/mcstransd.8
@@ -23,7 +23,7 @@ Output a short summary of available command line options\&.
 .SH "AUTHOR"
 This man page was written by Dan Walsh .
 The program was originally written by Dan Walsh .
-The program was enhanced/rwwritten by Joe Nall .
+The program was enhanced/rewritten by Joe Nall .

 .SH "FILES"
 /etc/selinux/{SELINUXTYPE}/setrans.conf




--
James Carter 
National Security Agency
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] libsepol/cil: Add hexadecimal support for Xen ioportcon statements

2017-03-28 Thread James Carter 

On 03/22/2017 03:01 PM, James Carter wrote:

Add hexadecimal support for Xen ioportcon statements which was
left out of commit c408c70.

Signed-off-by: James Carter 


This has been applied.

Jim


---
 libsepol/cil/src/cil_build_ast.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
index 442f100..8a19df4 100644
--- a/libsepol/cil/src/cil_build_ast.c
+++ b/libsepol/cil/src/cil_build_ast.c
@@ -4689,12 +4689,12 @@ int cil_gen_ioportcon(struct cil_db *db, struct 
cil_tree_node *parse_current, st
if (parse_current->next->cl_head != NULL) {
if (parse_current->next->cl_head->next != NULL &&
parse_current->next->cl_head->next->next == NULL) {
-   rc = cil_fill_integer(parse_current->next->cl_head, 
>ioport_low, 10);
+   rc = cil_fill_integer(parse_current->next->cl_head, 
>ioport_low, 0);
if (rc != SEPOL_OK) {
cil_log(CIL_ERR, "Improper ioport specified\n");
goto exit;
}
-   rc = cil_fill_integer(parse_current->next->cl_head->next, 
>ioport_high, 10);
+   rc = cil_fill_integer(parse_current->next->cl_head->next, 
>ioport_high, 0);
if (rc != SEPOL_OK) {
cil_log(CIL_ERR, "Improper ioport specified\n");
goto exit;
@@ -4705,7 +4705,7 @@ int cil_gen_ioportcon(struct cil_db *db, struct 
cil_tree_node *parse_current, st
goto exit;
}
} else {
-   rc = cil_fill_integer(parse_current->next, 
>ioport_low, 10);
+   rc = cil_fill_integer(parse_current->next, 
>ioport_low, 0);
if (rc != SEPOL_OK) {
cil_log(CIL_ERR, "Improper ioport specified\n");
goto exit;




--
James Carter 
National Security Agency
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH 1/2] policycoreutils: fixfiles should handle path arguments more robustly

2017-03-28 Thread James Carter 

On 03/26/2017 10:35 AM, Alan Jenkins wrote:

E.g. `fixfiles restore -v /usr` - before:

Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
Progress and Verbose mutually exclusive
usage:  /sbin/restorecon [-iFnprRv0] [-e excludedir] pathname...
usage:  /sbin/restorecon [-iFnprRv0] [-e excludedir] -f filename
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
229k

after:

Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
/sbin/restorecon:  lstat(-v) failed:  No such file or directory
Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
229k

This matches the usage shown in the manual page.  While we're in there,
we should handle spaces as well e.g `fixfiles restore "a b"`.  Before:

Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
/sbin/restorecon:  lstat(b) failed:  No such file or directory

After:

Warning: Skipping the following R/O filesystems:
/sys/fs/cgroup
/sbin/restorecon:  lstat(a b) failed:  No such file or directory

Signed-off-by: Alan Jenkins 


Both of these have been applied.

Thanks,
Jim


---
 policycoreutils/scripts/fixfiles | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
index 3896d19..d3a53ba 100755
--- a/policycoreutils/scripts/fixfiles
+++ b/policycoreutils/scripts/fixfiles
@@ -248,7 +248,7 @@ if [ ! -z "$RPMFILES" ]; then
 exit $?
 fi
 if [ ! -z "$FILEPATH" ]; then
-${RESTORECON} $exclude_dirs ${FORCEFLAG} ${VERBOSE} -R $* $FILEPATH 2>&1 | cat 
>> $LOGFILE
+${RESTORECON} $exclude_dirs ${FORCEFLAG} ${VERBOSE} -R $* -- "$FILEPATH" 2>&1 | 
cat >> $LOGFILE
 return
 fi
 if [  -n "${FILESYSTEMSRW}" ]; then
@@ -400,7 +400,7 @@ else
process $command
 else
while [ -n "$1" ]; do
-   FILEPATH=$1
+   FILEPATH="$1"
process $command
shift
done




--
James Carter 
National Security Agency
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH 1/1] libsepol/cil: do not dereference a NULL pointer when calloc() fails

2017-03-28 Thread James Carter 

On 03/25/2017 09:48 AM, Nicolas Iooss wrote:

When list_init() fails to allocate a list with calloc(), it calls
list_destroy() with l = NULL. This functions starts by dereferencing
its argument ("(*list)->head"), which does not work well when it is
NULL.

This bug can be fixed by returning directly in list_init() when calloc()
fails. Doing so allows making list_init() implementation shorter by
removing label "exit" and local variable "rc".

This issue has been found using clang's static analyzer.

Signed-off-by: Nicolas Iooss 


Applied.

Thanks,
Jim


---
 libsepol/src/module_to_cil.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 6c33b94da9d9..308ada4f1381 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -250,19 +250,13 @@ static void attr_list_destroy(struct list **attr_list)

 static int list_init(struct list **list)
 {
-   int rc = -1;
struct list *l = calloc(1, sizeof(*l));
if (l == NULL) {
-   goto exit;
+   return -1;
}

*list = l;
-
return 0;
-
-exit:
-   list_destroy();
-   return rc;
 }

 static int list_prepend(struct list *list, void *data)




--
James Carter 
National Security Agency
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH 1/3] policycoreutils/setfiles: stdout messages don't need program prefix

2017-03-28 Thread James Carter 

On 03/26/2017 12:22 PM, Alan Jenkins wrote:

I suggested that if you run a command for its informational output (by
passing  `-v`), you don't expect it to be prefixed with the program name.
Prefixing is used for error messages, so you can tell where your shell
script blew up :).  If a script is running a command for its informational
output, it's usually the script's responsibility to make sure it's in
context, e.g. providing headers if there are multiple sections of output.

Removing the program name from setfiles/restorecon output is particularly
useful because it generates very long lines.  But also, it actually helps
highlight where there are error messages - the prefix will make them
stand out visually.

Signed-off-by: Alan Jenkins 


All three of these have been applied.

Thanks,
Jim


---
 policycoreutils/setfiles/setfiles.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/policycoreutils/setfiles/setfiles.c 
b/policycoreutils/setfiles/setfiles.c
index 6f69c90..83e0b2a 100644
--- a/policycoreutils/setfiles/setfiles.c
+++ b/policycoreutils/setfiles/setfiles.c
@@ -142,9 +142,15 @@ static int __attribute__ ((format(printf, 2, 3)))
 log_callback(int type, const char *fmt, ...)
 {
int rc;
-   FILE *out = (type == SELINUX_INFO) ? stdout : stderr;
+   FILE *out;
va_list ap;
-   fprintf(out, "%s: ", r_opts.progname);
+
+   if (type == SELINUX_INFO) {
+   out = stdout;
+   } else {
+   out = stderr;
+   fprintf(out, "%s: ", r_opts.progname);
+   }
va_start(ap, fmt);
rc = vfprintf(out, fmt, ap);
va_end(ap);




--
James Carter 
National Security Agency
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] libsepol: In module_to_cil create one attribute for each unique set

2017-03-28 Thread James Carter 
CIL does not allow type or role sets in certain rules (such as allow
rules). It does, however, allow sets in typeattributeset and
roleattributeset statements. Because of this, when module_to_cil
translates a policy into CIL, it creates a new attribute for each
set that it encounters. But often the same set is used multiple times
which means that more attributes are created then necessary. As the
number of attributes increases the time required for the kernel to
make each policy decision increases which can be a problem.

To help reduce the number of attributes in a kernel policy,
when module_to_cil encounters a role or type set search to see if the
set was encountered already and, if it was, use the previously
generated attribute instead of creating a new one.

Testing on Android and Refpolicy policies show that this reduces the
number of attributes generated by about 40%.

Signed-off-by: James Carter 
---
 libsepol/src/module_to_cil.c | 593 +--
 1 file changed, 283 insertions(+), 310 deletions(-)

diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 6c33b94..4ea8a83 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -174,12 +174,9 @@ struct role_list_node {
 };
 
 struct attr_list_node {
-   char *attribute;
+   char *attr_name;
int is_type;
-   union {
-   struct type_set *ts;
-   struct role_set *rs;
-   } set;
+   void *set;
 };
 
 struct list_node {
@@ -237,7 +234,7 @@ static void attr_list_destroy(struct list **attr_list)
while (curr != NULL) {
attr = curr->data;
if (attr != NULL) {
-   free(attr->attribute);
+   free(attr->attr_name);
}
 
free(curr->data);
@@ -717,54 +714,85 @@ static int num_digits(int n)
return num;
 }
 
-static int set_to_cil_attr(struct policydb *pdb, int is_type, char ***names, 
uint32_t *num_names)
+static int ebitmap_to_cil(struct policydb *pdb, struct ebitmap *map, int type)
+{
+   struct ebitmap_node *node;
+   uint32_t i;
+   char **val_to_name = pdb->sym_val_to_name[type];
+
+   ebitmap_for_each_bit(map, node, i) {
+   if (!ebitmap_get_bit(map, i)) {
+   continue;
+   }
+   cil_printf("%s ", val_to_name[i]);
+   }
+
+   return 0;
+}
+
+static char *get_new_attr_name(struct policydb *pdb, int is_type)
 {
static unsigned int num_attrs = 0;
-   int rc = -1;
int len, rlen;
-   const char *attr_infix;
-   char *attr;
+   char *infix;
+   char *attr_name = NULL;
 
num_attrs++;
 
if (is_type) {
-   attr_infix = TYPEATTR_INFIX;
+   infix = TYPEATTR_INFIX;
} else {
-   attr_infix = ROLEATTR_INFIX;
+   infix = ROLEATTR_INFIX;
}
 
-   len = strlen(pdb->name) + strlen(attr_infix) + num_digits(num_attrs) + 
1;
-   attr = malloc(len);
-   if (attr == NULL) {
+   len = strlen(pdb->name) + strlen(infix) + num_digits(num_attrs) + 1;
+   attr_name = malloc(len);
+   if (!attr_name) {
log_err("Out of memory");
-   rc = -1;
goto exit;
}
-   rlen = snprintf(attr, len, "%s%s%i", pdb->name, attr_infix, num_attrs);
+
+   rlen = snprintf(attr_name, len, "%s%s%i", pdb->name, infix, num_attrs);
if (rlen < 0 || rlen >= len) {
log_err("Failed to generate attribute name");
-   rc = -1;
+   free(attr_name);
+   attr_name = NULL;
goto exit;
}
 
-   *names = malloc(sizeof(**names));
-   if (*names == NULL) {
+exit:
+   return attr_name;
+}
+
+static int cil_add_attr_to_list(struct list *attr_list, char *attr_name, int 
is_type, void *set)
+{
+   struct attr_list_node *attr_list_node = NULL;
+   int rc = 0;
+
+   attr_list_node = calloc(1, sizeof(*attr_list_node));
+   if (attr_list_node == NULL) {
log_err("Out of memory");
rc = -1;
goto exit;
}
 
+   rc = list_prepend(attr_list, attr_list_node);
+   if (rc != 0) {
+   goto exit;
+   }
 
-   *names[0] = attr;
-   *num_names = 1;
+   attr_list_node->attr_name = attr_name;
+   attr_list_node->is_type = is_type;
+   attr_list_node->set = set;
 
-   rc = 0;
+   return rc;
 
 exit:
+   free(attr_list_node);
return rc;
 }
 
-static int cil_print_attr_strs(int indent, struct policydb *pdb, int is_type, 
struct ebitmap *pos, struct ebitmap *neg, uint32_t flags, char *attr)
+static int cil_print_attr_strs(int indent, struct policydb *pdb, int is_type, 
void *set, char *attr_name)
 {
// CIL doesn't support anonymous positive/negative/complemented sets.  
So
//

Re: [PATCH] selinux: Use task_alloc hook rather than task_create hook

2017-03-28 Thread Tetsuo Handa 
Stephen Smalley wrote:
> On Tue, 2017-03-28 at 22:12 +0900, Tetsuo Handa wrote:
> > This patch is a preparation for getting rid of task_create hook
> > because
> > task_create hook
> 
> task_alloc hook?

Oops, copy error. Yes, I meant task_alloc hook.

> 
> >  which can do what task_create hook can do was revived.
> > 
> > Creating a new thread is unlikely prohibited by security policy, for
> > fork()/execve()/exit() is fundamental of how processes are managed in
> > Unix. If a program is known to create a new thread, it is likely that
> > permission to create a new thread is given to that program.
> > Therefore,
> > a situation where security_task_create() returns an error is likely
> > that
> > the program was exploited and lost control. Even if SELinux failed to
> > check permission to create a thread at security_task_create(),
> > SELinux
> > can later check it at security_task_alloc(). Since the new thread is
> > not
> > yet visible from the rest of the system, nobody can do bad things
> > using
> > the new thread. What we waste will be limited to some initialization
> > steps such as dup_task_struct(), copy_creds() and audit_alloc() in
> > copy_process(). We can tolerate these overhead for unlikely
> > situation.
> > 
> > Therefore, this patch changes SELinux to use task_alloc hook rather
> > than
> > task_create hook so that we can remove task_create hook.
> 
> Aside from the nit on the patch description above,
> 
> Acked-by: Stephen Smalley 

Thank you.

>From b43bd0fc0cc267b91f51ad118f6fabd13efb921e Mon Sep 17 00:00:00 2001
From: Tetsuo Handa 
Date: Tue, 28 Mar 2017 22:09:38 +0900
Subject: [PATCH v2] selinux: Use task_alloc hook rather than task_create hook

This patch is a preparation for getting rid of task_create hook because
task_alloc hook which can do what task_create hook can do was revived.

Creating a new thread is unlikely prohibited by security policy, for
fork()/execve()/exit() is fundamental of how processes are managed in
Unix. If a program is known to create a new thread, it is likely that
permission to create a new thread is given to that program. Therefore,
a situation where security_task_create() returns an error is likely that
the program was exploited and lost control. Even if SELinux failed to
check permission to create a thread at security_task_create(), SELinux
can later check it at security_task_alloc(). Since the new thread is not
yet visible from the rest of the system, nobody can do bad things using
the new thread. What we waste will be limited to some initialization
steps such as dup_task_struct(), copy_creds() and audit_alloc() in
copy_process(). We can tolerate these overhead for unlikely situation.

Therefore, this patch changes SELinux to use task_alloc hook rather than
task_create hook so that we can remove task_create hook.

Signed-off-by: Tetsuo Handa 
Acked-by: Stephen Smalley 
---
 security/selinux/hooks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..d850b7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3710,7 +3710,8 @@ static int selinux_file_open(struct file *file, const 
struct cred *cred)
 
 /* task security operations */
 
-static int selinux_task_create(unsigned long clone_flags)
+static int selinux_task_alloc(struct task_struct *task,
+ unsigned long clone_flags)
 {
u32 sid = current_sid();
 
@@ -6205,7 +6206,7 @@ static int selinux_key_getsecurity(struct key *key, char 
**_buffer)
 
LSM_HOOK_INIT(file_open, selinux_file_open),
 
-   LSM_HOOK_INIT(task_create, selinux_task_create),
+   LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
LSM_HOOK_INIT(cred_free, selinux_cred_free),
LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
-- 
1.8.3.1
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] selinux: Use task_alloc hook rather than task_create hook

2017-03-28 Thread Stephen Smalley 
On Tue, 2017-03-28 at 22:12 +0900, Tetsuo Handa wrote:
> This patch is a preparation for getting rid of task_create hook
> because
> task_create hook

task_alloc hook?

>  which can do what task_create hook can do was revived.
> 
> Creating a new thread is unlikely prohibited by security policy, for
> fork()/execve()/exit() is fundamental of how processes are managed in
> Unix. If a program is known to create a new thread, it is likely that
> permission to create a new thread is given to that program.
> Therefore,
> a situation where security_task_create() returns an error is likely
> that
> the program was exploited and lost control. Even if SELinux failed to
> check permission to create a thread at security_task_create(),
> SELinux
> can later check it at security_task_alloc(). Since the new thread is
> not
> yet visible from the rest of the system, nobody can do bad things
> using
> the new thread. What we waste will be limited to some initialization
> steps such as dup_task_struct(), copy_creds() and audit_alloc() in
> copy_process(). We can tolerate these overhead for unlikely
> situation.
> 
> Therefore, this patch changes SELinux to use task_alloc hook rather
> than
> task_create hook so that we can remove task_create hook.

Aside from the nit on the patch description above,

Acked-by: Stephen Smalley 

> 
> Signed-off-by: Tetsuo Handa 
> ---
>  security/selinux/hooks.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d37a723..d850b7f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3710,7 +3710,8 @@ static int selinux_file_open(struct file *file,
> const struct cred *cred)
>  
>  /* task security operations */
>  
> -static int selinux_task_create(unsigned long clone_flags)
> +static int selinux_task_alloc(struct task_struct *task,
> +   unsigned long clone_flags)
>  {
>   u32 sid = current_sid();
>  
> @@ -6205,7 +6206,7 @@ static int selinux_key_getsecurity(struct key
> *key, char **_buffer)
>  
>   LSM_HOOK_INIT(file_open, selinux_file_open),
>  
> - LSM_HOOK_INIT(task_create, selinux_task_create),
> + LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
>   LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
>   LSM_HOOK_INIT(cred_free, selinux_cred_free),
>   LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] selinux: Use task_alloc hook rather than task_create hook

2017-03-28 Thread Tetsuo Handa 
This patch is a preparation for getting rid of task_create hook because
task_create hook which can do what task_create hook can do was revived.

Creating a new thread is unlikely prohibited by security policy, for
fork()/execve()/exit() is fundamental of how processes are managed in
Unix. If a program is known to create a new thread, it is likely that
permission to create a new thread is given to that program. Therefore,
a situation where security_task_create() returns an error is likely that
the program was exploited and lost control. Even if SELinux failed to
check permission to create a thread at security_task_create(), SELinux
can later check it at security_task_alloc(). Since the new thread is not
yet visible from the rest of the system, nobody can do bad things using
the new thread. What we waste will be limited to some initialization
steps such as dup_task_struct(), copy_creds() and audit_alloc() in
copy_process(). We can tolerate these overhead for unlikely situation.

Therefore, this patch changes SELinux to use task_alloc hook rather than
task_create hook so that we can remove task_create hook.

Signed-off-by: Tetsuo Handa 
---
 security/selinux/hooks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..d850b7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3710,7 +3710,8 @@ static int selinux_file_open(struct file *file, const 
struct cred *cred)
 
 /* task security operations */
 
-static int selinux_task_create(unsigned long clone_flags)
+static int selinux_task_alloc(struct task_struct *task,
+ unsigned long clone_flags)
 {
u32 sid = current_sid();
 
@@ -6205,7 +6206,7 @@ static int selinux_key_getsecurity(struct key *key, char 
**_buffer)
 
LSM_HOOK_INIT(file_open, selinux_file_open),
 
-   LSM_HOOK_INIT(task_create, selinux_task_create),
+   LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
LSM_HOOK_INIT(cred_free, selinux_cred_free),
LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
-- 
1.8.3.1

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.