From: Matthew Mondor <mmondor@pulsar-zone.net>
Date: Fri, 2 Sep 2016 01:21:05 +0000 (+0000)
Subject: Not over yet, but added some more bound checks in string processing.
X-Git-Url: http://git.pulsar-zone.net/?a=commitdiff_plain;p=rubiks-ircd.git

Not over yet, but added some more bound checks in string processing.

The old pre-stdio bounded snprintf(3)/vsnprintf(3) custom code
returned the actual length that could be written.  On the other
hand, stdio oddly returns "what could have been written".  This is
problematic in loops that expect a short write when a buffer overflow
is mitigated, where the next call can become out of bounds.  Hence,
instead of directly using these stdio functions, a wrapper function
is now used which returns the actual string length that could be
written (calling strlen(3)), and also make sure that the end of
the buffer ends with '\0'.

prefix_buffer() did no bounds checking at all and this was noted
in its comment; this function now performs bounds checking.
---

diff --git a/include/ircsprintf.h b/include/ircsprintf.h
index a7bc069..3fbffc5 100644
--- a/include/ircsprintf.h
+++ b/include/ircsprintf.h
@@ -2,14 +2,16 @@
 #define IRCSPRINTF_H
 #include <stdarg.h>
 #include <stdio.h>
+#include <string.h>
 #include "setup.h"
 
 #ifndef TOO_OLD
 
 #define ircsprintf sprintf
-#define ircsnprintf snprintf
 #define ircvsprintf vsprintf
-#define ircvsnprintf vsnprintf
+
+extern int ircsnprintf(char *str, size_t size, const char *fmt, ...);
+extern int ircvsnprintf(char *str, size_t size, const char *fmt, va_list vl);
 
 #else /* TOO_OLD */
 
diff --git a/src/ircsprintf.c b/src/ircsprintf.c
index 12fa611..ee1fe4d 100644
--- a/src/ircsprintf.c
+++ b/src/ircsprintf.c
@@ -1,7 +1,7 @@
-#ifdef TOO_OLD
-
 #include "ircsprintf.h"
 
+#ifdef TOO_OLD
+
 char num[12] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
 char itoa_tab[10] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9'  };
 char xtoa_tab[16] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 
@@ -266,4 +266,30 @@ int ircvsnprintf(char *str, size_t size, const char *format, va_list ap)
 }
 #endif
 
+#else /* TOO_OLD */
+
+/* Unlike snprintf(3), returns actual written length, not what would be */
+int ircsnprintf(char *str, size_t size, const char *fmt, ...)
+{
+    va_list vl;
+
+    va_start(vl, fmt);
+    (void) vsnprintf(str, size, fmt, vl);
+    va_end(vl);
+
+    str[size - 1] = '\0';
+
+    return strlen(str);
+}
+
+/* Unlike vsnprintf(3), returns actual written length, not what would be */
+int ircvsnprintf(char *str, size_t size, const char *fmt, va_list vl)
+{
+    (void) vsnprintf(str, size, fmt, vl);
+
+    str[size - 1] = '\0';
+
+    return strlen(str);
+}
+
 #endif /* TOO_OLD */
diff --git a/src/send.c b/src/send.c
index 852e547..cf8df82 100644
--- a/src/send.c
+++ b/src/send.c
@@ -45,8 +45,9 @@
 extern int currently_processing_netsplit;
 #endif
 
-static char sendbuf[2048];
-static char remotebuf[2048];
+#define SENDBUF_SIZE 2048
+static char sendbuf[SENDBUF_SIZE];
+static char remotebuf[SENDBUF_SIZE];
 static char selfbuf[256];
 
 static int dead_link(aClient *to, char *notice, int sockerr);
@@ -428,7 +429,7 @@ void sendto_one(aClient *to, char *pattern, ...)
     int len;            /* used for the length of the current message */
     
     va_start(vl, pattern);
-    len = ircvsprintf(sendbuf, pattern, vl);
+    len = ircvsnprintf(sendbuf, sizeof(sendbuf), pattern, vl);
    
     if (to->from)
         to = to->from;
@@ -492,7 +493,7 @@ void vsendto_one(aClient *to, char *pattern, va_list vl)
  * remote: 1 if client is remote, 0 if local
  * from: the client sending the message
  * prefix: the prefix as specified (parv[0] usually)
- * buffer: the buffer to dump this into (NO BOUNDS CHECKING!)
+ * buffer: the buffer to dump this into
  * pattern: varargs pattern
  * vl: varargs variable list with one arg taken already
  */
@@ -505,45 +506,43 @@ static inline int prefix_buffer(int remote, aClient *from, char *prefix,
 
     *buffer = ':';
 
-    if(!remote && IsPerson(from))
-    {
+    if(!remote && IsPerson(from)) {
         int flag = 0;
         anUser *user = from->user;
 
-        for(p = from->name; *p; p++)
-                buffer[sidx++] = *p;
+        for (p = from->name; *p != '\0' && sidx < SENDBUF_SIZE; p++)
+	    buffer[sidx++] = *p;
 
-        if (user)
-        {
-            if (*user->username) 
-            {
-                buffer[sidx++] = '!';
-                for(p = user->username; *p; p++)
+        if (user) {
+            if (*user->username != '\0') {
+		if (sidx < SENDBUF_SIZE)
+		    buffer[sidx++] = '!';
+                for (p = user->username; *p != '\0' && sidx < SENDBUF_SIZE;
+		     p++)
                     buffer[sidx++] = *p;
             }
-            if (*user->host && !MyConnect(from)) 
-            {
-                buffer[sidx++] = '@';
-                for(p = user->host; *p; p++)
+            if (*user->host != '\0' && !MyConnect(from)) {
+		if (sidx < SENDBUF_SIZE)
+		    buffer[sidx++] = '@';
+                for (p = user->host; *p != '\0' && sidx < SENDBUF_SIZE; p++)
                     buffer[sidx++] = *p;
                 flag = 1;
             }
         }
-        if (!flag && MyConnect(from) && *user->host) 
-        {
-            buffer[sidx++] = '@';
-            for(p = from->sockhost; *p; p++)
+        if (!flag && MyConnect(from) && *user->host != '\0') {
+	    if (sidx < SENDBUF_SIZE)
+		buffer[sidx++] = '@';
+            for (p = from->sockhost; *p != '\0' && sidx < SENDBUF_SIZE; p++)
                 buffer[sidx++] = *p;
         }
-    }
-    else
-    {
-            for(p = prefix; *p; p++)
+    } else {
+            for (p = prefix; *p != '\0' && sidx < SENDBUF_SIZE; p++)
                 buffer[sidx++] = *p;
     }
 
-    msglen = ircvsprintf(&buffer[sidx], pattern + 3, vl);
+    msglen = ircvsnprintf(&buffer[sidx], SENDBUF_SIZE - sidx, pattern + 3, vl);
     msglen += sidx;
+    buffer[SENDBUF_SIZE - 1] = '\0';
 
     return msglen;
 }