Re: [PATCH/RFC/GSoC 1/3] path.c: implement xdg_runtime_dir()

From: Jeff King <peff@peff.net>
Date: Mar 16, 2016, 17:06 UTC
Message-ID: <20160316170623.GB4039@sigill.intra.peff.net>
In-Reply-To: <1458122865-29447-1-git-send-email-huiyiqun@gmail.com>

On Wed, Mar 16, 2016 at 06:07:43PM +0800, Hui Yiqun wrote:

> +	if (runtime_dir && *runtime_dir)
> +		git_runtime_dir = mkpathdup("%s/git/", runtime_dir);
> +	else
> +		git_runtime_dir = mkpathdup("/tmp/git-%d", uid);

Here we allocate the string, but later we may return NULL on error, leaking the allocated memory.

Show 14 quoted lines

> +	if (!lstat(git_runtime_dir, &st)) {
> +		/*
> +		 * As described in XDG base dir spec[1], the subdirectory
> +		 * under $XDG_RUNTIME_DIR or its fallback MUST be owned by
> +		 * the user, and its unix access mode MUST be 0700.
> +		 *
> +		 * Calling chmod or chown silently may cause security
> +		 * problem if somebody chdir to it, sleep, and then, try
> +		 * to open our protected runtime cache or socket.
> +		 * So we just put warning and left it to user to solve.
> +		 *
> +		 * [1]https://specifications.freedesktop.org/basedir-spec/
> +		 * basedir-spec-latest.html
> +		 */

OK. I think these checks should be sufficient to deal with the /tmp race I mentioned elsewhere in the thread (assuming that an attacker cannot flip the uid back and forth in the same way, but that should be true on Unix systems).

Show 13 quoted lines

> +		if ((st.st_mode & 0777) != S_IRWXU) {
> +			fprintf(stderr,
> +					"permission of runtime directory '%s' "
> +					"MUST be 0700 instead of 0%o\n",
> +					git_runtime_dir, (st.st_mode & 0777));
> +			return NULL;
> +		} else if (st.st_uid != uid) {
> +			fprintf(stderr,
> +					"owner of runtime directory '%s' "
> +					"MUST be %d instead of %d\n",
> +					git_runtime_dir, uid, st.st_uid);
> +			return NULL;
> +		}

Should these be using warning(), rather than a raw fprintf?

Show 13 quoted lines

> +	} else {
> +		if (safe_create_leading_directories_const(git_runtime_dir) < 0) {
> +			fprintf(stderr,
> +					"unable to create directories for '%s'\n",
> +					git_runtime_dir);
> +			return NULL;
> +		}
> +		if (mkdir(git_runtime_dir, 0700) < 0) {
> +			fprintf(stderr,
> +					"unable to mkdir '%s'\n", git_runtime_dir);
> +			return NULL;
> +		}
> +	}

And this retains the un-racy mkdir(). Good.

> +	free(git_runtime_dir);
> +	return mkpathdup("%s/%s", git_runtime_dir, filename);

This mkpathdup accesses the string we just freed?

It might be easier to just use a strbuf here, and then you can append to it at the end.

-Peff