git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 3/6] daemon: fix off-by-one in logging extended attributes

From
Jeff King <peff@peff.net>
Date
Jan 25, 2018, 00:56 UTC
Message-ID
<20180125005606.GC26850@sigill.intra.peff.net>
In-Reply-To
<20180125005447.GA26661@sigill.intra.peff.net>
If receive a request like:
  git-upload-pack /foo.git\0host=localhost

we mark the offset of the NUL byte as "len", and then log the bytes after the NUL with a "%.*s" placeholder, using "pktlen - len" as the length, and "line + len + 1" as the start of the string.

This is off-by-one, since the start of the string skips past the separating NUL byte, but the adjusted length includes it. Fortunately this doesn't actually read past the end of the buffer, since "%.*s" will stop when it hits a NUL. And regardless of what is in the buffer, packet_read() will always add an extra NUL terminator for safety.

As an aside, the git.git client sends an extra NUL after a "host" field, too, so we'd generally hit that one first, not the one added by packet_read(). You can see this in the test output which reports 15 bytes, even though the string has only 14 bytes of visible data. But the point is that even a client sending unusual data could not get us to read past the end of the buffer, so this is purely a cosmetic fix.

Reported-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
This code actually goes away in the next patch, but I thought it was
worth dealing with individually, since it is a memory error (albeit a
harmless one).
 daemon.c              |  4 ++--
 t/t5570-git-daemon.sh | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/daemon.c b/daemon.c
index e37e343d0a..d78afc8e96 100644
--- a/daemon.c
+++ b/daemon.c
@@ -759,8 +759,8 @@ static int execute(void)
 	len = strlen(line);
 	if (pktlen != len)
 		loginfo("Extended attributes (%d bytes) exist <%.*s>",
-			(int) pktlen - len,
-			(int) pktlen - len, line + len + 1);
+			(int) pktlen - len - 1,
+			(int) pktlen - len - 1, line + len + 1);
 	if (len && line[len-1] == '\n') {
 		line[--len] = 0;
 		pktlen--;
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index f92ebc5cd5..359af3994a 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -183,5 +183,16 @@ test_expect_success 'hostname cannot break out of directory' '
 		git ls-remote "$GIT_DAEMON_URL/escape.git"
 '
 
+test_expect_success 'daemon log records hostnames' '
+	cat >expect <<-\EOF &&
+	Extended attributes (15 bytes) exist <host=localhost>
+	EOF
+	>daemon.log &&
+	GIT_OVERRIDE_VIRTUAL_HOST=localhost \
+		git ls-remote "$GIT_DAEMON_URL/interp.git" &&
+	grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
+	test_cmp expect actual
+'
+
 stop_git_daemon
 test_done
-- 
2.16.1.273.gfdaa03aa74
Previous: Jeff KingNext: Jeff King
Message 6 of 13 in “off-by-one errors in git-daemon”
  1. 0/6 off-by-one errors in git-daemonJeff King, Jan 25, 2018
  2. 1/6 t5570: use ls-remote instead of clone for interp testsJeff King, Jan 25, 2018
  3. 2/6 t/lib-git-daemon: record daemon logJeff King, Jan 25, 2018
  4. Lucas WerkmeisterJan 25, 2018
  5. Jeff KingJan 25, 2018
  6. 3/6 daemon: fix off-by-one in logging extended attributesJeff King, Jan 25, 2018
  7. 4/6 daemon: handle NULs in extended attribute stringJeff King, Jan 25, 2018
  8. 5/6 t/lib-git-daemon: add network-protocol helpersJeff King, Jan 25, 2018
  9. 6/6 daemon: fix length computation in newline strippingJeff King, Jan 25, 2018
  10. Eric SunshineJan 25, 2018
  11. Jeff KingJan 26, 2018
  12. Junio C HamanoJan 25, 2018
  13. Jeff KingJan 25, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.