Serve only the files a dumb clone asks for

The object path check let any run of safe characters through, so a
spelling such as info//alternates or info/./alternates reached the
alternates file the exact name was refused for. Only a loose object, a
pack with its index and info/http-alternates are served now, and any
other path is not found.

3 files changed, 30 insertions, 21 deletions

diff --git a/source/ui-clone.c b/source/ui-clone.cindex 610aaa6..dad21d5 100644--- a/source/ui-clone.c+++ b/source/ui-clone.c@@ -79,20 +79,31 @@ static void print_pack_info(void) 	} } +static size_t hex_span(const char *s)+{+	return strspn(s, "0123456789abcdef");+}+ /*- * Only the characters an object path can hold pass, and no dotdot component.+ * A dumb client asks for three shapes of file below objects: a loose object+ * at xx/rest-of-hex, a pack or its index at pack/pack-hex.pack or .idx, and+ * info/http-alternates. Nothing else is served, so no spelling of a path can+ * reach a file outside those, the alternates file among them.  */-static int path_is_safe(const char *path)+static int object_path_is_served(const char *path) {-	const char *p;+	const size_t hexsz = the_hash_algo->hexsz;+	const char *rest; -	for (p = path; *p; ++p) {-		if (*p == '.' && *(p + 1) == '.')-			return 0;-		if (!isalnum((unsigned char)*p) && *p != '/' && *p != '.' && *p != '-')-			return 0;+	if (!strcmp(path, "info/http-alternates"))+		return 1;+	if (skip_prefix(path, "pack/pack-", &rest)) {+		size_t n = hex_span(rest);++		return n == hexsz && (!strcmp(rest + n, ".pack") || !strcmp(rest + n, ".idx")); 	}-	return 1;+	return hex_span(path) == 2 && path[2] == '/' && hex_span(path + 3) == hexsz - 2 &&+		path[hexsz + 1] == '\0'; }  /*@@ -166,17 +177,11 @@ void cgit_clone_objects(void) 		return; 	} -	// The alternates file names directories on the server's disk, which a-	// client fetching over http cannot reach, so only the http form that-	// is written for such clients goes out.-	if (!strcmp(ctx.qry.path, "info/alternates")) {+	if (!object_path_is_served(ctx.qry.path)) { 		cgit_print_error_page(404, "Not Found", "Not found"); 		return; 	} -	if (!path_is_safe(ctx.qry.path))-		goto err;- 	path = repo_git_path(the_repository, "objects/%s", ctx.qry.path); 	send_file(path); 	free(path);
diff --git a/source/ui-clone.c b/source/ui-clone.c
index 610aaa6..dad21d5 100644
--- a/source/ui-clone.c
+++ b/source/ui-clone.c
@@ -79,20 +79,31 @@ static void print_pack_info(void)
 	}
 }
 
+static size_t hex_span(const char *s)
+{
+	return strspn(s, "0123456789abcdef");
+}
+
 /*
- * Only the characters an object path can hold pass, and no dotdot component.
+ * A dumb client asks for three shapes of file below objects: a loose object
+ * at xx/rest-of-hex, a pack or its index at pack/pack-hex.pack or .idx, and
+ * info/http-alternates. Nothing else is served, so no spelling of a path can
+ * reach a file outside those, the alternates file among them.
  */
-static int path_is_safe(const char *path)
+static int object_path_is_served(const char *path)
 {
-	const char *p;
+	const size_t hexsz = the_hash_algo->hexsz;
+	const char *rest;
 
-	for (p = path; *p; ++p) {
-		if (*p == '.' && *(p + 1) == '.')
-			return 0;
-		if (!isalnum((unsigned char)*p) && *p != '/' && *p != '.' && *p != '-')
-			return 0;
+	if (!strcmp(path, "info/http-alternates"))
+		return 1;
+	if (skip_prefix(path, "pack/pack-", &rest)) {
+		size_t n = hex_span(rest);
+
+		return n == hexsz && (!strcmp(rest + n, ".pack") || !strcmp(rest + n, ".idx"));
 	}
-	return 1;
+	return hex_span(path) == 2 && path[2] == '/' && hex_span(path + 3) == hexsz - 2 &&
+		path[hexsz + 1] == '\0';
 }
 
 /*
@@ -166,17 +177,11 @@ void cgit_clone_objects(void)
 		return;
 	}
 
-	// The alternates file names directories on the server's disk, which a
-	// client fetching over http cannot reach, so only the http form that
-	// is written for such clients goes out.
-	if (!strcmp(ctx.qry.path, "info/alternates")) {
+	if (!object_path_is_served(ctx.qry.path)) {
 		cgit_print_error_page(404, "Not Found", "Not found");
 		return;
 	}
 
-	if (!path_is_safe(ctx.qry.path))
-		goto err;
-
 	path = repo_git_path(the_repository, "objects/%s", ctx.qry.path);
 	send_file(path);
 	free(path);
diff --git a/tests/t0301-security.sh b/tests/t0301-security.shindex 3df2cee..d9ec265 100755--- a/tests/t0301-security.sh+++ b/tests/t0301-security.sh@@ -452,10 +452,10 @@ test_expect_success 'a posted length past any buffer is clamped' '  test_expect_success 'the dumb transport refuses a path that climbs out' ' 	cveq "url=cve/objects/../config" >tmp &&-	grep "^Status: 400" tmp &&+	grep "^Status: 404" tmp && 	! grep "repositoryformatversion" tmp && 	cveq "url=cve/objects/info/../../config" >tmp &&-	grep "^Status: 400" tmp &&+	grep "^Status: 404" tmp && 	! grep "repositoryformatversion" tmp ' 
diff --git a/tests/t0301-security.sh b/tests/t0301-security.sh
index 3df2cee..d9ec265 100755
--- a/tests/t0301-security.sh
+++ b/tests/t0301-security.sh
@@ -452,10 +452,10 @@ test_expect_success 'a posted length past any buffer is clamped' '
 
 test_expect_success 'the dumb transport refuses a path that climbs out' '
 	cveq "url=cve/objects/../config" >tmp &&
-	grep "^Status: 400" tmp &&
+	grep "^Status: 404" tmp &&
 	! grep "repositoryformatversion" tmp &&
 	cveq "url=cve/objects/info/../../config" >tmp &&
-	grep "^Status: 400" tmp &&
+	grep "^Status: 404" tmp &&
 	! grep "repositoryformatversion" tmp
 '
 
diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.shindex cbb4cdc..005c6cd 100755--- a/tests/t0303-robustness.sh+++ b/tests/t0303-robustness.sh@@ -592,9 +592,13 @@ test_expect_success 'the dumb transport sends a file with its size' ' test_expect_success 'the dumb transport withholds the alternates file' ' 	mkdir -p repos/rob/.git/objects/info && 	echo "$PWD/repos/foo/.git/objects" >repos/rob/.git/objects/info/alternates &&-	robq "url=rob/objects/info/alternates" >tmp &&-	grep "^Status: 404" tmp &&-	! grep "repos/foo" tmp &&+	for path in info/alternates info//alternates info/./alternates ./info/alternates \+		info/Alternates /info/alternates+	do+		robq "url=rob/objects/$path" >tmp &&+		grep "^Status: 404" tmp &&+		! grep "repos/foo" tmp || return 1+	done && 	rm repos/rob/.git/objects/info/alternates && 	robq "url=rob/objects/info/packs" >tmp && 	grep "^Status: 200" tmp
diff --git a/tests/t0303-robustness.sh b/tests/t0303-robustness.sh
index cbb4cdc..005c6cd 100755
--- a/tests/t0303-robustness.sh
+++ b/tests/t0303-robustness.sh
@@ -592,9 +592,13 @@ test_expect_success 'the dumb transport sends a file with its size' '
 test_expect_success 'the dumb transport withholds the alternates file' '
 	mkdir -p repos/rob/.git/objects/info &&
 	echo "$PWD/repos/foo/.git/objects" >repos/rob/.git/objects/info/alternates &&
-	robq "url=rob/objects/info/alternates" >tmp &&
-	grep "^Status: 404" tmp &&
-	! grep "repos/foo" tmp &&
+	for path in info/alternates info//alternates info/./alternates ./info/alternates \
+		info/Alternates /info/alternates
+	do
+		robq "url=rob/objects/$path" >tmp &&
+		grep "^Status: 404" tmp &&
+		! grep "repos/foo" tmp || return 1
+	done &&
 	rm repos/rob/.git/objects/info/alternates &&
 	robq "url=rob/objects/info/packs" >tmp &&
 	grep "^Status: 200" tmp