Re: genhomedircon is broken in libsemanage

[James Antill <james.antill@redhat.com>]

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


 Mostly FYI, although there is one minor error dealing with a malloc()
error case.

On Thu, 2008-01-31 at 10:13 -0500, Todd C. Miller wrote:

> Index: trunk/libsemanage/src/genhomedircon.c
> ===================================================================
> --- trunk/libsemanage/src/genhomedircon.c	(revision 2771)
> +++ trunk/libsemanage/src/genhomedircon.c	(working copy)
> @@ -24,6 +24,8 @@
>  #include <semanage/seusers_policy.h>
>  #include <semanage/users_policy.h>
>  #include <semanage/user_record.h>
> +#include <semanage/fcontext_record.h>
> +#include <semanage/fcontexts_policy.h>
>  #include <sepol/context.h>
>  #include <sepol/context_record.h>
>  #include "semanage_store.h"
> @@ -45,6 +47,7 @@
>  #include <pwd.h>
>  #include <errno.h>
>  #include <unistd.h>
> +#include <regex.h>
>  
>  /* paths used in get_home_dirs() */
>  #define PATH_ETC_USERADD "/etc/default/useradd"
> @@ -101,6 +104,11 @@
>  	const char *replace_with;
>  } replacement_pair_t;
>  
> +typedef struct {
> +	const char *dir;
> +	int matched;
> +} fc_match_handle_t;
> +
>  static semanage_list_t *default_shell_list(void)
>  {
>  	semanage_list_t *list = NULL;
> @@ -150,10 +158,70 @@
>  	return list;
>  }
>  
> +/* Helper function called via semanage_fcontext_iterate() */
> +static int fcontext_matches(const semanage_fcontext_t *fcontext, void *varg)
> +{
> +	const char *oexpr = semanage_fcontext_get_expr(fcontext);
> +	fc_match_handle_t *handp = varg;
> +	struct Ustr *expr;
> +	regex_t re;
> +	size_t n;
> +	int type, retval = -1;
> +
> +	/* Only match ALL or DIR */
> +	type = semanage_fcontext_get_type(fcontext);
> +	if (type != SEMANAGE_FCONTEXT_ALL && type != SEMANAGE_FCONTEXT_ALL)
> +		return 0;
> +
> +	/* Convert oexpr into a Ustr and anchor it at the beginning */
> +	expr = ustr_dup_cstr("^");
> +	if (expr == USTR_NULL)
> +		goto done;
> +	ustr_ins_cstr(&expr, 1, oexpr);

 This works fine, but you can use:

          ustr_add_cstr(&expr, oexpr)

...which appends data, so you don't need to keep track of the offset.

> +	if (expr == USTR_NULL)

 This will never be true, you either want to test the return value or
use the "has had a memory error" flag:

          if (ustr_enomem(expr))

> +		goto done;
> +	n = ustr_len(expr);
> +
> +	/* Strip off trailing ".+" or ".*" */
> +	if (ustr_cmp_suffix_cstr_eq(expr, ".+") ||
> +	    ustr_cmp_suffix_cstr_eq(expr, ".*")) {
> +		if (!ustr_del_subustr(&expr, n - 1, 2))

 This works fine, but you can use:

                  if (!ustr_del(&expr, 2))

...which always removes the last X bytes.

> +			goto done;
> +		n -= 2;
> +	}
> +
> +	/* Strip off trailing "(/.*)?" */
> +	if (ustr_cmp_suffix_cstr_eq(expr, "(/.*)?")) {
> +		if (!ustr_del_subustr(&expr, n - 5, 6))
> +			goto done;
> +		n -= 6;
> +	}
> +
> +	/* Append pattern to eat up trailing slashes */
> +	if (!ustr_ins_cstr(&expr, n, "/*$"))
> +		goto done;
> +
> +	/* Check dir against expr */
> +	if (regcomp(&re, ustr_cstr(expr), REG_EXTENDED) != 0)
> +		goto done;
> +	if (regexec(&re, handp->dir, 0, NULL, 0) == 0)
> +		handp->matched = 1;
> +	regfree(&re);
> +
> +	retval = 0;
> +
> +done:
> +	if (expr)
> +		ustr_free(expr);

 This works fine, but:

 ustr_free(NULL);

...is guaranteed to be a noop, much like libc free(NULL).

> +
> +	return retval;
> +}


-- 
James Antill <james.antill@redhat.com>
Red Hat

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]