Repositories / rubiks-ircd.git / commitdiff
? search:
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: b52612c)
Not over yet, but added some more bound checks in string processing. master
authorMatthew Mondor <mmondor@pulsar-zone.net>
Fri, 2 Sep 2016 01:21:05 +0000 (01:21 +0000)
committerMatthew Mondor <mmondor@pulsar-zone.net>
Fri, 2 Sep 2016 01:21:05 +0000 (01:21 +0000)
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.

include/ircsprintf.h patch | blob | blame | history
src/ircsprintf.c patch | blob | blame | history
src/send.c patch | blob | blame | history

diff --git a/include/ircsprintf.h b/include/ircsprintf.h
index a7bc069..3fbffc5 100644 (file)
--- a/include/ircsprintf.h
+++ b/include/ircsprintf.h
@@ -2,14 +2,16 @@
 #define IRCSPRINTF_H
 #include <stdarg.h>
 #include <stdio.h>
 #define IRCSPRINTF_H
 #include <stdarg.h>
 #include <stdio.h>
+#include <string.h>
 #include "setup.h"
 
 #ifndef TOO_OLD
 
 #define ircsprintf sprintf
 #include "setup.h"
 
 #ifndef TOO_OLD
 
 #define ircsprintf sprintf
-#define ircsnprintf snprintf
 #define ircvsprintf vsprintf
 #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 */
 
 
 #else /* TOO_OLD */
 
diff --git a/src/ircsprintf.c b/src/ircsprintf.c
index 12fa611..ee1fe4d 100644 (file)
--- a/src/ircsprintf.c
+++ b/src/ircsprintf.c
@@ -1,7 +1,7 @@
-#ifdef TOO_OLD
-
 #include "ircsprintf.h"
 
 #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', 
 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
 
 }
 #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 */
 #endif /* TOO_OLD */
diff --git a/src/send.c b/src/send.c
index 852e547..cf8df82 100644 (file)
--- a/src/send.c
+++ b/src/send.c
@@ -45,8 +45,9 @@
 extern int currently_processing_netsplit;
 #endif
 
 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);
 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);
     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;
    
     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)
  * 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
  */
  * 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 = ':';
 
 
     *buffer = ':';
 
-    if(!remote && IsPerson(from))
-    {
+    if(!remote && IsPerson(from)) {
         int flag = 0;
         anUser *user = from->user;
 
         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;
             }
                     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;
             }
         }
                     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;
         }
                 buffer[sidx++] = *p;
         }
-    }
-    else
-    {
-            for(p = prefix; *p; p++)
+    } else {
+            for (p = prefix; *p != '\0' && sidx < SENDBUF_SIZE; p++)
                 buffer[sidx++] = *p;
     }
 
                 buffer[sidx++] = *p;
     }
 
-    msglen = ircvsprintf(&buffer[sidx], pattern + 3, vl);
+    msglen = ircvsnprintf(&buffer[sidx], SENDBUF_SIZE - sidx, pattern + 3, vl);
     msglen += sidx;
     msglen += sidx;
+    buffer[SENDBUF_SIZE - 1] = '\0';
 
     return msglen;
 }
 
     return msglen;
 }
Rubiks IRCd, based on Bahamut