View Issue Details

IDProjectCategoryView StatusLast Update
0000577LDMud 3.3Implementationpublic2018-01-29 21:57
Reporterzesstra Assigned Tozesstra  
PriorityhighSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.3 
Fixed in Version3.3.718 
Summary0000577: Potential crashes in send_erq() and send_udp() due to stack overflow
Descriptionf_send_erq() and f_send_udp() allocate temporary buffers for their messages respectively the destination hostname. Using large strings as hostnames or large arrays (if the array size is not suitably limited) may crash the driver.
I will attach a patch for this issue.
Steps To Reproducesend_udp("a"*8000000, 4242, "bla");
TagsNo tags attached.
Attached Files
send_erq_send_udp.diff (5,117 bytes)   
Index: comm.c
===================================================================
--- comm.c	(Revision 2426)
+++ comm.c	(Arbeitskopie)
@@ -3400,6 +3400,10 @@
                  */
 
                 DTN(("telnet machine ready\n"));
+                /* buffer overflows here are impossible even with strcpy(),
+                 * because buff is allocated in backend() as MAX_TEXT+4 and
+                 * ip->text is allocated as MAX_TEXT+2. Ok, as long as nobody
+                 * changes buff in backend() withour changing ip->text ... */
                 strcpy(buff, ip->text);
                 command_giver = ip->ob;
                 trace_level = ip->trace_level;
@@ -3441,6 +3445,10 @@
                     }
                     else
                     {
+                        /* buff is limited to MAX_TEXT+4. Additionally,
+                         * get_message() is usually not called recursvely or
+                         * from a very high stack depth, so alloca() is 
+                         * probably ok. */
                         char *snoop_message = alloca(strlen(buff) + 4);
                         sprintf(snoop_message, "%% %s\n", buff);
                         tell_npc_str(snooper, snoop_message);
@@ -6197,13 +6205,17 @@
         vector_t *v;
         svalue_t *svp;
         char *cp;
-        mp_int j;
+        p_int j;
 
         v = sp[-1].u.vec;
         arglen = VEC_SIZE(v);
-        cp = arg = alloca(arglen);
+        cp = arg = xalloc(arglen);
+        if (!arg) {
+            errorf("Out of memory (%zu bytes) in send_erq() for allocating "
+                   "temporary buffer.\n", arglen);
+        }
         svp = &v->item[0];
-        for (j = (mp_int)arglen; --j >= 0; )
+        for (j = (p_int)arglen; --j >= 0; )
             *cp++ = (char)(*svp++).u.number;
     }
 
@@ -6251,7 +6263,11 @@
         i = 0;
         free_svalue(sp);
     }
-
+    /* cleanup */
+    if (sp[-1].type != T_STRING) {
+        /* free arg only if sp-1 is not a string */
+        xfree(arg);
+    }
     free_svalue(--sp);
     (*--sp).u.number = i;
 
@@ -7051,7 +7067,7 @@
  */
 
 {
-    char *to_host;
+    char *to_host = NULL;
     int to_port;
     char *msg;
     size_t msglen;
@@ -7061,7 +7077,10 @@
     struct sockaddr_in name;
     struct hostent *hp;
     int ret = 0;
-
+    svalue_t *firstarg; /* store the first argument */
+    
+    firstarg = sp-2;  /* hostname */
+    
     switch(0) { default: /* try {...} */
 
         /* Set msg/msglen to the data of the message to send */
@@ -7076,21 +7095,26 @@
             vector_t *v;
             svalue_t *svp;
             char *cp;
-            mp_int j;
+            p_int j;
 
             v = sp->u.vec;
             msglen = VEC_SIZE(v);
-            cp = msg = alloca(msglen);
-            if (!msg)
-                break;
+            /* allocate memory and push error handler onto stack */
+            cp = msg = xalloc_with_error_handler(msglen);
+            if (!msg) {
+                errorf("Out of memory (%zu bytes) in send_udp() for "
+                       "temporary buffer.\n", msglen);
+            }
+            sp = inter_sp;
+            
             svp = &v->item[0];
-            for (j = (mp_int)msglen; --j >= 0; )
+            for (j = (p_int)msglen; --j >= 0; )
                 *cp++ = (char)(*svp++).u.number;
         }
 
         /* Is this call valid? */
 
-        if (!privilege_violation(STR_SEND_UDP, sp-2, sp))
+        if (!privilege_violation(STR_SEND_UDP, firstarg, firstarg + 2))
             break;
 
         if (udp_s < 0)
@@ -7101,16 +7125,17 @@
         {
             size_t adrlen;
 
-            adrlen = mstrsize((sp-2)->u.str);
-            to_host = alloca(adrlen+1);
+            adrlen = mstrsize(firstarg->u.str);
+            /* as there are no runtime error raised below, we just xallocate
+             * and don't bother with an error handler. */
+            to_host = xalloc(adrlen+1);
             if (!to_host)
             {
-                inter_sp = sp;
-                errorf("Out of stack (%zu bytes) for host address\n"
+                errorf("Out of memory (%zu bytes) in send_udp() for host address\n"
                      , (adrlen+1));
                 /* NOTREACHED */
             }
-            memcpy(to_host, get_txt((sp-2)->u.str), adrlen);
+            memcpy(to_host, get_txt(firstarg->u.str), adrlen);
             to_host[adrlen] = '\0';
         }
         to_port = (sp-1)->u.number;
@@ -7156,14 +7181,18 @@
             break;
         ret = 1;
     }
-
-    /* Return the result */
-
-    free_svalue(sp);
-    free_svalue(--sp);
-    free_svalue(--sp);
+    /* Cleanup - an allocated buffer for the message will be on the stack 
+     * above the arguments, therefore clean everything from the first argument
+     * (including) to sp.
+     */
+    sp = pop_n_elems((sp-firstarg)+1, sp);
+    xfree(to_host);
+    
+    /*Return the result */
+    sp++;
     put_number(sp, ret);
     return sp;
+    
 } /* f_send_udp() */
 
 /*-------------------------------------------------------------------------*/
send_erq_send_udp.diff (5,117 bytes)   

Relationships

related to 0000553 resolvedGnomi LDMud 3.2 Backports of 3.3 fixes for 3.2.16 
child of 0000545 new LDMud 3.3 Usages of alloca() have to be checked for possible stack overflow 

Activities

zesstra

2008-10-04 10:55

administrator   ~0000799

Should be fixed in r2436.

Issue History

Date Modified Username Field Change
2008-09-28 13:30 zesstra New Issue
2008-09-28 13:30 zesstra Status new => assigned
2008-09-28 13:30 zesstra Assigned To => zesstra
2008-09-28 13:38 zesstra Relationship added child of 0000545
2008-09-28 13:45 zesstra File Added: send_erq_send_udp.diff
2008-10-04 10:55 zesstra Status assigned => resolved
2008-10-04 10:55 zesstra Fixed in Version => 3.3.718
2008-10-04 10:55 zesstra Resolution open => fixed
2008-10-04 10:55 zesstra Note Added: 0000799
2008-11-17 15:35 Gnomi Relationship added related to 0000553
2010-11-16 09:42 zesstra Source_changeset_attached => ldmud.git master 790d2211
2018-01-29 18:59 zesstra Source_changeset_attached => ldmud.git master 790d2211
2018-01-29 21:57 zesstra Source_changeset_attached => ldmud.git master 790d2211