[PATCH] sanity check in add_packed_git()

[PATCH] sanity check in add_packed_git()

[Pavel Roskin]

add_packed_git() tries to get the pack SHA1 by parsing its name.  It may
access uninitialized memory for packs with short names.

Signed-off-by: Pavel Roskin <proski@gnu.org>

diff --git a/sha1_file.c b/sha1_file.c
index fa22e9c..d83d824 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -464,7 +464,7 @@ struct packed_git *add_packed_git(char *
 	p->pack_last_used = 0;
 	p->pack_use_cnt = 0;
 	p->pack_local = local;
-	if (!get_sha1_hex(path + path_len - 40 - 4, sha1))
+	if ((path_len > 44) && !get_sha1_hex(path + path_len - 44, sha1))
 		memcpy(p->sha1, sha1, 20);
 	return p;
 }


-- 
Regards,
Pavel Roskin

Re: [PATCH] sanity check in add_packed_git()

[Junio C Hamano]

Pavel Roskin <proski@gnu.org> writes:

> add_packed_git() tries to get the pack SHA1 by parsing its name.  It may
> access uninitialized memory for packs with short names.

Thanks.

This fixes when there is ".git/objects/packs/junk-X.pack", so in
that sense it is a real fix and I'll take it.

However, I feel a bit uneasy about this whole thing.  The core
does not care how you name your packs, as long as .pack and .idx
files have the same prefix, but we started relying on the prefix
being "pack-" followed by 40 hexadecimal digits since we started
packed repository support in http-fetch, and we also allowed
sha1_pack_name() function that shares the assumption to sneak
into the real core part of the system around the same time (end
of July 2005).  git-repack and git-verify-pack stay ignorant
about this prefix convention and I think that is a good
property.  However, we might be better off if we declare that
the pack files *must* be named following that convention
(currently nobody enforces it, but as long as the user uses "git
repack" to create packs, the packs automatically follow that
convention), and make check_packed_git_idx() reject a packfile
whose name does not begin with "pack-" or the 40 hexdigits that
follow does not match the hash of the object names the index
records.  If we go that route, then this patch becomes unneeded;
more extensive check needs to be done in check_packed_git_idx().

Thoughts?

Re: [PATCH] sanity check in add_packed_git()

[Daniel Barkalow]

On Wed, 21 Dec 2005, Junio C Hamano wrote:

> Pavel Roskin <proski@gnu.org> writes:
> 
> > add_packed_git() tries to get the pack SHA1 by parsing its name.  It may
> > access uninitialized memory for packs with short names.
> 
> Thanks.
> 
> This fixes when there is ".git/objects/packs/junk-X.pack", so in
> that sense it is a real fix and I'll take it.
> 
> However, I feel a bit uneasy about this whole thing.  The core
> does not care how you name your packs, as long as .pack and .idx
> files have the same prefix, but we started relying on the prefix
> being "pack-" followed by 40 hexadecimal digits since we started
> packed repository support in http-fetch, and we also allowed
> sha1_pack_name() function that shares the assumption to sneak
> into the real core part of the system around the same time (end
> of July 2005).  git-repack and git-verify-pack stay ignorant
> about this prefix convention and I think that is a good
> property.  However, we might be better off if we declare that
> the pack files *must* be named following that convention
> (currently nobody enforces it, but as long as the user uses "git
> repack" to create packs, the packs automatically follow that
> convention), and make check_packed_git_idx() reject a packfile
> whose name does not begin with "pack-" or the 40 hexdigits that
> follow does not match the hash of the object names the index
> records.  If we go that route, then this patch becomes unneeded;
> more extensive check needs to be done in check_packed_git_idx().
> 
> Thoughts?

I'd like to require it to be a hash, just because that makes it 
prohibitively difficult to make something people will accept as
pack-05f611b3b8198b262acdf678584d365f8e879aec.pack other than the one from 
the git repository. Sure, it would only be a minor DoS and a bit of 
confusion, because it still couldn't do any worse than contain some 
different objects, but it would block people from getting the pack with 
the objects they want. Since nobody seems interested in naming their 
packs, so far as I know, it seems best to force the names to be 
universally unique, modulo an incredible coincidence.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] sanity check in add_packed_git()

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> I'd like to require it to be a hash, just because that makes it 
> prohibitively difficult to make something people will accept as
> pack-05f611b3b8198b262acdf678584d365f8e879aec.pack other than the one from 
> the git repository.

How about doing something like this, then?

I haven't tried this with http-fetch but with this I think we
can unconditionally assign p->sha1 in add_packed_git().

-- >8 --
Subject: [PATCH] Require packfiles to follow the naming convention.

With this, pack files must be named "pack-[0-9a-f]{40}.pack",
with corresponding .idx file, and the hexadecimal part must
match the SHA1 checksum git-pack-objects computed when the pack
was created originally (i.e. sum over sorted object names of all
the contained objects).  The core would ignore a valid packfile
in $GIT_OBJECT_DIRECTORY/packs that "git-unpack-objects" would
expand properly if it does not follow the naming convention.

[jc: this breaks existing test t5300.]

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 sha1_file.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

4ff64a29401b28cfbd925cf86eed8b8e734ed24b
diff --git a/sha1_file.c b/sha1_file.c
index 6011473..01ac925 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -319,18 +319,33 @@ struct packed_git *packed_git;
 static int check_packed_git_idx(const char *path, unsigned long *idx_size_,
 				void **idx_map_)
 {
+	SHA_CTX ctx;
+	unsigned char sha1[20];
+	unsigned char packname[20];
 	void *idx_map;
 	unsigned int *index;
 	unsigned long idx_size;
 	int nr, i;
-	int fd = open(path, O_RDONLY);
+	int fd;
 	struct stat st;
+
+	fd = open(path, O_RDONLY);
 	if (fd < 0)
 		return -1;
 	if (fstat(fd, &st)) {
 		close(fd);
 		return -1;
 	}
+
+	/* We require "...../pack-XXXX{40}XXXX.idx" now.
+	 * 5-byte "pack-", 40-byte hex and 4-byte .idx.
+	 */
+	i = strlen(path);
+	if (i < 49 || strcmp(path + i - 4, ".idx") ||
+	    strncmp(path + i - 49, "pack-", 5) ||
+	    get_sha1_hex(path + i - 44, packname))
+		return error("non index file %s ignored", path);
+
 	idx_size = st.st_size;
 	idx_map = mmap(NULL, idx_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
@@ -362,6 +377,27 @@ static int check_packed_git_idx(const ch
 	if (idx_size != 4*256 + nr * 24 + 20 + 20)
 		return error("wrong index file size");
 
+	/*
+	 * File checksum and index name.
+	 */
+	SHA1_Init(&ctx);
+	SHA1_Update(&ctx, idx_map, idx_size-20);
+	SHA1_Final(sha1, &ctx);
+
+	if (memcmp(sha1, idx_map + idx_size - 20, 20))
+		return error("index checksum mismatch");
+
+	SHA1_Init(&ctx);
+	for (i = 0; i < nr; i++) {
+		unsigned char *ent = (idx_map + 4*256) + i * 24 + 4;
+		SHA1_Update(&ctx, ent, 20);
+	}
+	SHA1_Final(sha1, &ctx);
+	
+	if (memcmp(sha1, packname, 20))
+		return error("pack index name %s does not match contents %s",
+			     path, sha1_to_hex(sha1));
+
 	return 0;
 }
 
-- 
1.0.GIT