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
Attachment:
signature.asc
Description: This is a digitally signed message part
