Buffer overflows

[Timo Sirainen <tss@iki.fi>]

[-- Attachment #1.1: Type: text/plain, Size: 1497 bytes --]

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);


[-- Attachment #1.2: git-strings.diff --]
[-- Type: text/x-patch, Size: 6100 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]