2016-03-17 1:06 GMT+08:00 Jeff King <peff@xxxxxxxx>:
> 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.
Yes, do you think goto is a good solution for clearup?
>
>> + 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).
>
>> + 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?
Well, I will replace it.
During the greping. I found that I should also wrap my warning strings
with _() for i18n.
>
>> + } 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.
I think so. Thanks.
>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html