[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pkg-shadow-devel
Subject:    Re: [Pkg-shadow-devel] [PATCH 05/11] Implement find_new_sub_uids find_new_sub_gids
From:       Seth Arnold <seth.arnold () canonical ! com>
Date:       2013-06-15 0:15:31
Message-ID: 20130615001531.GB32705 () hunt
[Download RAW message or body]


My apologies for the lateness of my reply; I have a few small thoughts
about these patches.

On Tue, Jan 22, 2013 at 01:15:05AM -0800, Eric W. Biederman wrote:
> Functions for finding new subordinate uid and gids ranges for use
> with useradd.
> 
> [...]
>
> +int find_new_sub_gids (const char *owner,
> +		       gid_t *range_start, unsigned long *range_count)
> +{
> +	unsigned long min, max;
> +	unsigned long count;
> +	gid_t start;
> +
> +	assert (range_start != NULL);
> +	assert (range_count != NULL);
> +
> +	min = getdef_ulong ("SUB_GID_MIN", 100000UL);
> +	max = getdef_ulong ("SUB_GID_MAX", 600100000UL);
> +	count = getdef_ulong ("SUB_GID_COUNT", 10000);


I'd like to see a quick


	if (min >= max || count >= max || (min + count) >= max) {
		/* fail loudly */
	}

right here. The sanity of the rest of the function depends upon these
invariants.

> +
> +	/* Is there a preferred range that works? */
> +	if ((*range_count != 0) &&
> +	    (*range_start >= min) &&
> +	    (((*range_start) + (*range_count) - 1) <= max) &&
> +	    is_sub_gid_range_free(*range_start, *range_count)) {
> +		return 0;
> +	}
> +
> +	if (max < (min + count)) {
> +		(void) fprintf (stderr,
> +				_("%s: Invalid configuration: SUB_GID_MIN (%lu), SUB_GID_MAX (%lu)\n"),
> +			Prog, min, max);
> +		return -1;
> +	}
> +	start = sub_gid_find_free_range(min, max, count);
> +	if (start == (gid_t)-1) {
> +		fprintf (stderr,
> +		         _("%s: Can't get unique secondary GID range\n"),
> +		         Prog);
> +		SYSLOG ((LOG_WARN, "no more available secondary GIDs on the system"));
> +		return -1;
> +	}
> +	*range_start = start;
> +	*range_count = count;
> +	return 0;
> +}
> +
> diff --git a/libmisc/find_new_sub_uids.c b/libmisc/find_new_sub_uids.c
> new file mode 100644
> index 0000000..f1720f9
> --- /dev/null
> +++ b/libmisc/find_new_sub_uids.c
> 
> [...]
> 
> +/*
> + * find_new_sub_uids - Find a new unused range of UIDs.
> + *
> + * If successful, find_new_sub_uids provides a range of unused
> + * user IDs in the [SUB_UID_MIN:SUB_UID_MAX] range.
> + * 
> + * Return 0 on success, -1 if no unused UIDs are available.
> + */
> +int find_new_sub_uids (const char *owner,
> +		       uid_t *range_start, unsigned long *range_count)
> +{
> +	unsigned long min, max;
> +	unsigned long count;
> +	uid_t start;
> +
> +	assert (range_start != NULL);
> +	assert (range_count != NULL);
> +
> +	min = getdef_ulong ("SUB_UID_MIN", 100000UL);
> +	max = getdef_ulong ("SUB_UID_MAX", 600100000UL);
> +	count = getdef_ulong ("SUB_UID_COUNT", 10000);

Same here, of course.

> +	/* Is there a preferred range that works? */
> +	if ((*range_count != 0) &&
> +	    (*range_start >= min) &&
> +	    (((*range_start) + (*range_count) - 1) <= max) &&
> +	    is_sub_uid_range_free(*range_start, *range_count)) {
> +		return 0;
> +	}
> +
> +	if (max < (min + count)) {
> +		(void) fprintf (stderr,
> +				_("%s: Invalid configuration: SUB_UID_MIN (%lu), SUB_UID_MAX (%lu)\n"),
> +			Prog, min, max);
> +		return -1;
> +	}
> +	start = sub_uid_find_free_range(min, max, count);
> +	if (start == (uid_t)-1) {
> +		fprintf (stderr,
> +		         _("%s: Can't get unique secondary UID range\n"),
> +		         Prog);
> +		SYSLOG ((LOG_WARN, "no more available secondary UIDs on the system"));
> +		return -1;
> +	}
> +	*range_start = start;
> +	*range_count = count;
> +	return 0;
> +}

On Tue, Jan 22, 2013 at 01:20:07AM -0800, Eric W. Biederman wrote:
> +struct map_range *get_map_ranges(int ranges, int argc, char **argv)
> +{
> +	struct map_range *mappings, *mapping;
> +	int idx, argidx;
> +
> +	if ((ranges * 3) > argc) {
> +		fprintf(stderr, "ranges: %u argc: %d\n",
> +			ranges, argc);
> +		fprintf(stderr,
> +			_( "%s: Not enough arguments to form %u mappings\n"),
> +			Prog, ranges);
> +		return NULL;
> +	}

If 'ranges' is large enough, 3*ranges can wrap around to be less than
'argc', pass this test, and continue to the for loop below, which could
scribble all over memory until running into an unmapped page.

The current use appears safe, and I don't see how this could be exploited
even if it weren't currently safe, but I'd really like to see this test
re-written in a way that does not allow wraparound.

> +	mappings = calloc(ranges, sizeof(*mappings));
> +	if (!mappings) {
> +		fprintf(stderr, _( "%s: Memory allocation failure\n"),
> +			Prog);
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	/* Gather up the ranges from the command line */
> +	mapping = mappings;
> +	for (idx = 0; idx < ranges; idx++, argidx += 3, mapping++) {
> +		if (!getulong(argv[argidx + 0], &mapping->upper))
> +			return NULL;
> +		if (!getulong(argv[argidx + 1], &mapping->lower))
> +			return NULL;
> +		if (!getulong(argv[argidx + 2], &mapping->count))
> +			return NULL;
> +	}
> +	return mappings;
> +}

Thanks,

Seth

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]