[PATCH 2/3] make update-server-info more robust
Since "git update-server-info" may be called automatically
as part of a push or a "gc --auto", we should be robust
against two processes trying to update it simultaneously.
However, we currently use a fixed tempfile, which means that
two simultaneous writers may step on each other's toes and
end up renaming junk into place.
Let's instead switch to using a unique tempfile via mkstemp.
We do not want to use a lockfile here, because it's OK for
two writers to simultaneously update (one will "win" the
rename race, but that's OK; they should be writing the same
information).
While we're there, let's clean up a few other things:
1. Detect write errors. Report them and abort the update
if any are found.
2. Free path memory rather than leaking it (and clean up
the tempfile when necessary).
3. Use the pathdup functions consistently rather than
static buffers or manually calculated lengths.
This last one fixes a potential overflow of "infofile" in
update_info_packs (e.g., by putting large junk into
$GIT_OBJECT_DIRECTORY). However, this overflow was probably
not an interesting attack vector for two reasons:
a. The attacker would need to control the environment to
do this, in which case it was already game-over.
b. During its setup phase, git checks that the directory
actually exists, which means it is probably shorter
than PATH_MAX anyway.
Because both update_info_refs and update_info_packs share
these same failings (and largely duplicate each other), this
patch factors out the improved error-checking version into a
helper function.
Signed-off-by: Jeff King
---
I guess point (b) may not apply on systems that have a really small
PATH_MAX that does not reflect what you can actually create in the
filesystem (Windows?). But I think point (a) still applies, so this is
not really a big deal security-wise (though it is certainly a bugfix for
such systems).
server-info.c | 116 +++---
1 file changed, 71 insertions(+), 45 deletions(-)
diff --git a/server-info.c b/server-info.c
index 9ec744e..d54a3d6 100644
--- a/server-info.c
+++ b/server-info.c
@@ -4,45 +4,80 @@
#include "commit.h"
#include "tag.h"
-/* refs */
-static FILE *info_ref_fp;
+/*
+ * Create the file "path" by writing to a temporary file and renaming
+ * it into place. The contents of the file come from "generate", which
+ * should return non-zero if it encounters an error.
+ */
+static int update_info_file(char *path, int (*generate)(FILE *))
+{
+ char *tmp = mkpathdup("%s_XX", path);
+ int ret = -1;
+ int fd = -1;
+ FILE *fp = NULL;
+
+ safe_create_leading_directories(path);
+ fd = mkstemp(tmp);
+ if (fd < 0)
+ goto out;
+ fp = fdopen(fd, "w");
+ if (!fp)
+ goto out;
+ ret = generate(fp);
+ if (ret)
+ goto out;
+ if (fclose(fp))
+ goto out;
+ if (adjust_shared_perm(tmp) < 0)
+ goto out;
+ if (rename(tmp, path) < 0)
+ goto out;
+ ret = 0;
+
+out:
+ if (ret) {
+ error("unable to update %s: %s", path, strerror(errno));
+ if (fp)
+ fclose(fp);
+ else if (fd >= 0)
+ close(fd);
+ unlink(tmp);
+ }
+ free(tmp);
+ return ret;
+}
static int add_info_ref(const char *path, const unsigned char *sha1, int flag,
void *cb_data)
{
+ FILE *fp = cb_data;
struct object *o = parse_object(sha1);
if (!o)
return -1;
- fprintf(info_ref_fp, "%s%s\n", sha1_to_hex(sha1), path);
+ if (fprintf(fp, "%s %s\n", sha1_to_hex(sha1), path) < 0)
+ return -1;
+
if (o->type == OBJ_TAG) {
o = deref_tag(o, path, 0);
if (o)
- fprintf(info_ref_fp, "%s%s^{}\n",
- sha1_to_hex(o->sha1), path);
+ if (fprintf(fp, "%s %s^{}\n",
+ sha1_to_hex(o->sha1), path) < 0)
+ return -1;
}
return 0;
}
+static int generate_info_refs(FILE *fp)
+{
+ return for_each_ref(add_info_ref, fp);
+}
+
static int update_info_refs(int force)
{
- char *path0 = git_pathdup("info/refs");
- int len = strlen(path0);
- char *path1 = xmalloc(len + 2);
-
- strcpy(path1, path0);
- strcpy(path1 + len, "+");
-
- safe_create_leading_directories(path0);
- info_ref_fp = fopen(path1, "w");
- if (!info_ref_fp)
- return error("unable to update %s", path1);
- for_each_ref(add_info_ref, NULL);
- fclose(info_ref_fp);
- adjust_shared_perm(path1);
- rename(path1, path0);
- free(path0);
- free(path1);
- return 0;
+ char *path = git_pathdup("info/refs");
+
Re: [PATCH 2/3] make update-server-info more robust
Am 13.09.2014 um 22:19 schrieb Jeff King: > Since "git update-server-info" may be called automatically > as part of a push or a "gc --auto", we should be robust > against two processes trying to update it simultaneously. > However, we currently use a fixed tempfile, which means that > two simultaneous writers may step on each other's toes and > end up renaming junk into place. > > Let's instead switch to using a unique tempfile via mkstemp. > We do not want to use a lockfile here, because it's OK for > two writers to simultaneously update (one will "win" the > rename race, but that's OK; they should be writing the same > information). > > While we're there, let's clean up a few other things: > >1. Detect write errors. Report them and abort the update > if any are found. > >2. Free path memory rather than leaking it (and clean up > the tempfile when necessary). > >3. Use the pathdup functions consistently rather than > static buffers or manually calculated lengths. > > This last one fixes a potential overflow of "infofile" in > update_info_packs (e.g., by putting large junk into > $GIT_OBJECT_DIRECTORY). However, this overflow was probably > not an interesting attack vector for two reasons: > >a. The attacker would need to control the environment to > do this, in which case it was already game-over. > >b. During its setup phase, git checks that the directory > actually exists, which means it is probably shorter > than PATH_MAX anyway. > > Because both update_info_refs and update_info_packs share > these same failings (and largely duplicate each other), this > patch factors out the improved error-checking version into a > helper function. > > Signed-off-by: Jeff King > --- > I guess point (b) may not apply on systems that have a really small > PATH_MAX that does not reflect what you can actually create in the > filesystem (Windows?). It's the other way around: PATH_MAX is an actual limit basically only on Windows [1] unless you avoid using the Windows API [2]. Regardless of the security implications, getting rid of more PATH_MAX buffers is a good move. And I looked only briefly at your patch, but I like the three bullet points above. :) René [1] http://insanecoding.blogspot.de/2007/11/pathmax-simply-isnt.html [2] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] make update-server-info more robust
Jeff King writes: > Since "git update-server-info" may be called automatically > as part of a push or a "gc --auto", we should be robust > against two processes trying to update it simultaneously. > However, we currently use a fixed tempfile, which means that > two simultaneous writers may step on each other's toes and > end up renaming junk into place. Thanks. I'll queue these clean-ups but we may want to start thinking about deprecating and removing the dumb http support. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] make update-server-info more robust
On Mon, Sep 15, 2014 at 11:39:12AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > Since "git update-server-info" may be called automatically > > as part of a push or a "gc --auto", we should be robust > > against two processes trying to update it simultaneously. > > However, we currently use a fixed tempfile, which means that > > two simultaneous writers may step on each other's toes and > > end up renaming junk into place. > > Thanks. I'll queue these clean-ups but we may want to start > thinking about deprecating and removing the dumb http support. Yeah, I have often thought about that (especially the push support, which has always been flaky and underused). However, some possible schemes for resumable clone could be easily implemented by shunting the cloner to a dumb-http conversation. So it may be worth keeping at least the fetch side around for the time being. Food for thought. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
