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

Buffer overflows

From
TSTimo Sirainen <tss@iki.fi>
Date
Aug 30, 2007, 19:26 UTC
Message-ID
<1188502009.29782.874.camel@hurina>

Looks like nothing has happened since my last mail about this (http://marc.info/?l=git&m=117962988804430&w=2).

I sure hope no-one's using git-mailinfo to do any kind of automated mail processing from untrusted users. Here's one way to cause it to overflow a buffer in stack:

Subject: =?iso-8859-15?b?pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSk
 pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSk
 pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSk
 pKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKSkpKQ=?=

It's not the only way. I just accidentally hit that when trying to verify another buffer overflow. Just look for strcpy()s in the file.

Libc string handling functions are broken and should not be used for anything. It's annoying when such a large project as Git is still repeating the same old mistakes. I guess security doesn't matter to anyone.

Attached once again beginnings of safer string handling functions, which should be easy to use to replace the existing string handling code. I even thought about creating some kind of an automated tool to do this, but that's a bit too much trouble with no gain for myself.

Usage goes like:

STATIC_STRING(str, 1024); sstr_append(str, "hello "); sstr_printfa(str, "%d", 5);

struct string *str; str = str_alloc(1024); // initial malloc size, grows when needed str_append(str, "hello "); str_printfa(str, "%d", 5); str_free(&str);

diff --git a/Makefile b/Makefile
index 4eb4637..c79eced 100644
--- a/Makefile
+++ b/Makefile
@@ -135,7 +135,7 @@ uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
 
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
@@ -283,7 +283,7 @@ XDIFF_LIB=xdiff/lib.a
 LIB_H = \
 	archive.h blob.h cache.h commit.h csum-file.h delta.h grep.h \
 	diff.h object.h pack.h pkt-line.h quote.h refs.h list-objects.h sideband.h \
-	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
+	run-command.h strbuf.h str.h str-static.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \
 	utf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \
 	mailmap.h remote.h
@@ -302,7 +302,7 @@ LIB_OBJS = \
 	object.o pack-check.o pack-write.o patch-delta.o path.o pkt-line.o \
 	sideband.o reachable.o reflog-walk.o \
 	quote.o read-cache.o refs.o run-command.o dir.o object-refs.o \
-	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
+	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o str.o str-static.o \
 	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \
 	revision.o pager.o tree-walk.o xdiff-interface.o \
 	write_or_die.o trace.o list-objects.o grep.o match-trees.o \
diff --git a/str-static.c b/str-static.c
new file mode 100644
index 0000000..a3f48f8
--- /dev/null
+++ b/str-static.c
@@ -0,0 +1,40 @@
+#include "str-static.h"
+
+void str_static_append(struct string_static *str, const char *cstr)
+{
+	unsigned int avail = str->size - str->len;
+	unsigned int len = strlen(cstr);
+
+	if (len >= avail) {
+		len = avail - 1;
+		str->overflowed = 1;
+	}
+	memcpy(str->buf + str->len, cstr, len);
+	str->len += len;
+	str->buf[str->len] = '\0';
+}
+
+void str_static_printfa(struct string_static *str, const char *fmt, ...)
+{
+	unsigned int avail = str->size - str->len;
+	va_list va;
+	int ret;
+
+	va_start(va, fmt);
+	ret = vsnprintf(str->buf + str->len, avail, fmt, va);
+	if (ret < (int)avail)
+		str->len += ret;
+	else {
+		str->len += avail - 1;
+		str->overflowed = 1;
+	}
+	va_end(va);
+}
+
+void str_static_truncate(struct string_static *str, unsigned int len)
+{
+	if (len >= str->size)
+		len = str->size - 1;
+	str->len = len;
+	str->buf[len] = '\0';
+}
diff --git a/str-static.h b/str-static.h
new file mode 100644
index 0000000..19cb28f
--- /dev/null
+++ b/str-static.h
@@ -0,0 +1,33 @@
+#ifndef STR_STATIC_H
+#define STR_STATIC_H
+
+#include "git-compat-util.h"
+
+struct string_static {
+	unsigned int size;
+	unsigned int len:31;
+	unsigned int overflowed:1;
+	char buf[];
+};
+
+#define STR_STATIC(name, size) \
+	union { \
+	  struct string_static string; \
+	  char string_buf[sizeof(struct string_static) + (size) + 1]; \
+	} name = { { (size)+1, 0, 0 } }
+
+extern void str_static_append(struct string_static *str, const char *cstr);
+extern void str_static_printfa(struct string_static *str, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
+extern void str_static_truncate(struct string_static *str, unsigned int len);
+
+#define sstr_append(str, cstr) str_static_append(&(str).string, cstr)
+#define sstr_printfa(str, fmt, ...) \
+	str_static_printfa(&(str).string, fmt, __VA_ARGS__)
+#define sstr_truncate(str, len) str_static_truncate(&(str).string, len)
+
+#define sstr_c(str) ((str).string.buf)
+#define sstr_len(str) ((str).string.len)
+#define sstr_overflowed(str) ((str).string.overflowed)
+
+#endif
diff --git a/str.c b/str.c
new file mode 100644
index 0000000..7530073
--- /dev/null
+++ b/str.c
@@ -0,0 +1,75 @@
+#include "str.h"
+#include "git-compat-util.h"
+
+extern struct string *str_alloc(unsigned int initial_size)
+{
+	struct string *str;
+
+	str = xmalloc(sizeof(*str));
+	str->len = 0;
+	str->size = initial_size + 1;
+	str->buf = xmalloc(str->size);
+	str->buf[0] = '\0';
+	return str;
+}
+
+void str_free(struct string **_str)
+{
+	struct string *str = *_str;
+
+	free(str->buf);
+	str->buf = NULL;
+	free(str);
+
+	*_str = NULL;
+}
+
+static void str_grow_if_needed(struct string *str, unsigned int len)
+{
+	unsigned int avail = str->size - str->len;
+
+	if (len >= avail) {
+		str->size = (str->len + len) * 2;
+		str->buf = xrealloc(str->buf, str->size);
+	}
+}
+
+void str_append(struct string *str, const char *cstr)
+{
+	unsigned int len = strlen(cstr);
+
+	str_grow_if_needed(str, len);
+	memcpy(str->buf + str->len, cstr, len);
+	str->len += len;
+	str->buf[str->len] = '\0';
+}
+
+void str_printfa(struct string *str, const char *fmt, ...)
+{
+	unsigned int avail = str->size - str->len;
+	va_list va, va2;
+	int ret;
+
+	va_start(va, fmt);
+	va_copy(va2, va);
+	ret = vsnprintf(str->buf + str->len, avail, fmt, va);
+	assert(ret >= 0);
+
+	if ((unsigned int)ret >= avail) {
+		str_grow_if_needed(str, ret);
+		avail = str->size - str->len;
+
+		ret = vsnprintf(str->buf + str->len, avail, fmt, va2);
+		assert(ret >= 0 && (unsigned int)ret < avail);
+	}
+	str->len += ret;
+	va_end(va);
+}
+
+void str_truncate(struct string *str, unsigned int len)
+{
+	if (len >= str->size)
+		len = str->size - 1;
+	str->len = len;
+	str->buf[len] = '\0';
+}
diff --git a/str.h b/str.h
new file mode 100644
index 0000000..329aad4
--- /dev/null
+++ b/str.h
@@ -0,0 +1,20 @@
+#ifndef STR_H
+#define STR_H
+
+struct string {
+	unsigned int len, size;
+	char *buf;
+};
+
+extern struct string *str_alloc(unsigned int initial_size);
+extern void str_free(struct string **str);
+
+extern void str_append(struct string *str, const char *cstr);
+extern void str_printfa(struct string *str, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
+extern void str_truncate(struct string *str, unsigned int len);
+
+#define str_c(str) ((str)->buf)
+#define str_len(str) ((str)->len)
+
+#endif
Next: Lukas Sandström
Message 1 of 26 in “Buffer overflows”
  1. Timo SirainenAug 30, 2007
  2. Lukas SandströmAug 30, 2007
  3. Linus TorvaldsAug 30, 2007
  4. Timo SirainenAug 30, 2007
  5. Reece DunnAug 30, 2007
  6. Timo SirainenAug 30, 2007
  7. Reece DunnAug 30, 2007
  8. Wincent ColaiutaAug 31, 2007
  9. Simon 'corecode' SchubertAug 31, 2007
  10. Junio C HamanoAug 30, 2007
  11. Pierre HabouzitAug 30, 2007
  12. Timo SirainenAug 30, 2007
  13. Johan HerlandSep 2, 2007
  14. Reece DunnSep 2, 2007
  15. David KastrupSep 2, 2007
  16. Reece DunnSep 2, 2007
  17. Jakub NarebskiSep 3, 2007
  18. Junio C HamanoSep 3, 2007
  19. René ScharfeSep 2, 2007
  20. Lukas SandströmSep 2, 2007
  21. Linus TorvaldsAug 31, 2007
  22. Timo SirainenAug 31, 2007
  23. Andreas EricssonAug 31, 2007
  24. Johannes SchindelinAug 31, 2007
  25. Temporary fix for stack smashing in mailinfoAlex Riesen, Aug 30, 2007
  26. Junio C HamanoAug 30, 2007

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.