Re: [PATCH] rtld: Reject overly long LD_AUDIT path elements

[Andreas Schwab <schwab at suse dot de>]

On Jun 19 2017, fweimer@redhat.com (Florian Weimer) wrote:

> diff --git a/elf/rtld.c b/elf/rtld.c
> index c801ee5..74147c3 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -116,13 +116,91 @@ dso_name_valid_for_suid (const char *p)
>    return *p != '\0';
>  }
>  
> -/* List of auditing DSOs.  */
> +/* LD_AUDIT variable contents.  Must be processed before the
> +   audit_list below.  */
> +const char *audit_list_string;
> +
> +/* Cyclic list of auditing DSOs.  audit_list->next is the first
> +   element.  */
>  static struct audit_list
>  {
>    const char *name;
>    struct audit_list *next;
>  } *audit_list;
>  
> +/* Iterator for audit_list_string followed by audit_list.  */
> +struct audit_list_iter
> +{
> +  /* Tail of audit_list_string still needing processing, or NULL.  */
> +  const char *audit_list_tail;
> +
> +  /* The list element returned in the previous iteration.  NULL before
> +     the first element.  */
> +  struct audit_list *previous;
> +
> +  /* Scratch buffer for returning a name which is part of
> +     audit_list_string.  */
> +  char fname[PATH_MAX];
> +};
> +
> +/* Initialize an audit list iterator.  */
> +static void
> +audit_list_iter_init (struct audit_list_iter *iter)
> +{
> +  iter->audit_list_tail = audit_list_string;
> +  iter->previous = NULL;
> +}
> +
> +/* Iterate through both audit_list_string and audit_list.  */
> +static const char *
> +audit_list_iter_next (struct audit_list_iter *iter)
> +{
> +  if (iter->audit_list_tail != NULL)
> +    {
> +      /* First iterate over audit_list_string.  */
> +      while (*iter->audit_list_tail != '\0')
> +	{
> +	  /* Split audit list at colon.  */
> +	  size_t len = strcspn (iter->audit_list_tail, ":");
> +	  if (len > 0 && len < PATH_MAX)
> +	    {
> +	      memcpy (iter->fname, iter->audit_list_tail, len);
> +	      iter->fname[len] = '\0';
> +	    }
> +	  else
> +	    /* Do not return this name to the caller.  */
> +	    iter->fname[0] = '\0';
> +
> +	  /* Skip over the substring and the following delimiter.  */
> +	  iter->audit_list_tail += len;
> +	  if (*iter->audit_list_tail == ':')
> +	    ++iter->audit_list_tail;
> +
> +	  /* If the name is valid, return it.  */
> +	  if (dso_name_valid_for_suid (iter->fname))
> +	    return iter->fname;
> +	  /* Otherwise, wrap around and try the next name.  */
> +	}
> +      /* Fall through to the procesing of audit_list.  */
> +    }
> +
> +  if (iter->previous == NULL)
> +    {
> +      if (audit_list == NULL)
> +	/* No pre-parsed audit list.  */
> +	return NULL;
> +      /* Start of audit list.  The first list element is at
> +	 audit_list->next (cyclic list).  */
> +      iter->previous = audit_list->next;
> +      return iter->previous->name;
> +    }
> +  if (iter->previous == audit_list)
> +    /* Cyclic list wrap-around.  */
> +    return NULL;
> +  iter->previous = iter->previous->next;
> +  return iter->previous->name;
> +}
> +

Why do you need all that complexity?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."