Raydium 3D Game Engine

Official forum for everything about Raydium, ManiaDrive, MeMak, ...
It is currently Thu Mar 28, 2024 8:56 pm

All times are UTC




Post new topic Reply to topic  [ 9 posts ] 
Author Message
 Post subject: Security bugs in Raydium
PostPosted: Wed May 10, 2006 7:50 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
I received this mail from Luigi Auriemma ( http://aluigi.org , thanks to him) and he allows me to post it here.
Code:
Hey,

I want to report some security bugs I have found in the latest SVN
version of Raydium (tested revision 301):

--------------------------------------------------------------
A] buffer-overflow in raydium_log and raydium_console_line_add
--------------------------------------------------------------

The logging function of Raydium is very used in the engine.
For example everytime a client tries to join the server, it logs the
event in the console:

  raydium_log("network: client %i connected as %s"/*,inet_ntoa
(from->sin_addr)*/,n,name);

This useful function is affected by a buffer-overflow bug where the
local buffer str of 255 (RAYDIUM_MAX_NAME_LEN) bytes is filled using
the unsecure sprintf function.
The size of the input packet is 512 (RAYDIUM_NETWORK_PACKET_SIZE)
bytes of which 508 are available for the text to use for exploiting the
vulnerability.

  From raydium/log.c:

// need to be secured
void raydium_log(char *format, ...)
{
char str[RAYDIUM_MAX_NAME_LEN];
va_list argptr;


va_start(argptr,format);
vsprintf(str,format,argptr);
va_end(argptr);

printf("Raydium: %s\n",str);
if(raydium_log_file) fprintf(raydium_log_file,"%s\n",str);
raydium_console_line_add(str);
}


Similar thing for raydium_console_line_add:

  From raydium/console.c:

// need to secure this one too
void raydium_console_line_add(char *format, ...)
{
char str[RAYDIUM_MAX_NAME_LEN];
va_list argptr;
va_start(argptr,format);
vsprintf(str,format,argptr);
va_end(argptr);

raydium_console_line_last++;
if(raydium_console_line_last>=RAYDIUM_CONSOLE_MAX_LINES)
   raydium_console_line_last=0;

strcpy(raydium_console_lines[raydium_console_line_last],str);
}


-------------------------------
B] format string in raydium_log
-------------------------------

The same raydium_log function described above is affected also by a
format string vulnerability caused by the calling of
raydium_console_line_add passing directly the text string without the
required format argument.


--------------------------------------------------------
C] NULL function pointer in raydium_network_netcall_exec
--------------------------------------------------------

The function raydium_network_netcall_exec is called by
raydium_network_read for selecting the specific function to use for
handling the type of packet received.
The problem is that the function pointer is not correctly initialized
(or assigned) if the type of packet received is major than the number
32 (RAYDIUM_NETWORK_MAX_NETCALLS), causing the crash of the program
during the calling of the resulted NULL pointer.

  From raydium/network.c:

void raydium_network_netcall_exec(int type,char *buff)
{
char tmpbuff[RAYDIUM_NETWORK_PACKET_SIZE];
int i;
void (*f)(int, char*);

for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
 if(raydium_network_netcall_type[i]==type)
 {
    memcpy(tmpbuff,buff,RAYDIUM_NETWORK_PACKET_SIZE);
    f=raydium_network_netcall_func[i];
    f(type,tmpbuff);
 }
}


--------------------------------------------------------------------
D] buffer-overflow and invalid memory access in raydium_network_read
--------------------------------------------------------------------

The function raydium_network_read is affectd by some buffer-overflow
bugs which happens during the writing of some global variables
allocated in an array of 32 (RAYDIUM_NETWORK_MAX_SERVERS) elements.
The same function is also affected by an invalid memory access could
happen when the server sends a packet to the client containing an 8
bit id bigger than 8 (RAYDIUM_NETWORK_MAX_CLIENTS).
Both the bugs can be exploited only versus the clients.

>From raydium/network.c:

signed char raydium_network_read(int *id, signed char *type, char *buff)
    ...
    strcpy(raydium_network_server_list[slot].name,name);
    ...
    strcpy(raydium_network_server_list[slot].info,info);
    ...
    i=buff[RAYDIUM_NETWORK_PACKET_OFFSET];
    strcpy(raydium_network_name[i],buff+RAYDIUM_NETWORK_PACKET_OFFSET+1);
    ...


A proof-of-concept for testing the first 3 bugs is available here:

  http://aluigi.org/poc/raydiumx.zip


And here is a (interesting, I think) part of my response:
Code:
Hi,

First of all, thanks for the time you takes working on this project. Some of theses bugs are already known, but some others, not (good point for raydium_network_netcall_exec() for example !). There's also some potential buffer overflows in ode_net.c, as for example with raydium_ode_network_read(),which reads as many events as said in the first byte of the packet :

memcpy(&n,data+RAYDIUM_NETWORK_PACKET_OFFSET,sizeof(n));

if(type==RAYDIUM_NETWORK_PACKET_ODE_DATA)
 for(i=0;i<n;i++)
    {
    get=(raydium_ode_network_Event *)(data+RAYDIUM_NETWORK_PACKET_OFFSET+sizeof(n)+(i*sizeof(*get)));
    raydium_ode_network_apply(get);
    }

There's probably a lot more errors like that, since the network API was built very naively (no htonl/ntohl, no security considerations, ...), and your work is a good start for us to correct all this.


Let's correct all this, now ! :)


Top
 Profile  
 
PostPosted: Thu May 11, 2006 8:12 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Ok, so there is a little patch that from my point of view is ok.

Code:
Index: raydium/console.c
===================================================================
--- raydium/console.c   (révision 309)
+++ raydium/console.c   (copie de travail)
@@ -228,20 +228,21 @@

 }

-// need to secure this one too
 void raydium_console_line_add(char *format, ...)
 {
-char str[RAYDIUM_MAX_NAME_LEN];
 va_list argptr;
-va_start(argptr,format);
-vsprintf(str,format,argptr);
-va_end(argptr);
+int retlen;

 raydium_console_line_last++;
 if(raydium_console_line_last>=RAYDIUM_CONSOLE_MAX_LINES)
    raydium_console_line_last=0;

-strcpy(raydium_console_lines[raydium_console_line_last],str);
+va_start(argptr,format);
+retlen = vsnprintf(raydium_console_lines[raydium_console_line_last],RAYDIUM_MAX_NAME_LEN - 1,format,argptr);
+va_end(argptr);
+
+if(retlen < 0) retlen = 0;
+raydium_console_lines[raydium_console_line_last][retlen] = '\0';
 }

 int raydium_console_history_read(char **hist)
Index: raydium/log.c
===================================================================
--- raydium/log.c       (révision 309)
+++ raydium/log.c       (copie de travail)
@@ -17,13 +17,17 @@
 {
 char str[RAYDIUM_MAX_NAME_LEN];
 va_list argptr;
+int retlen;


 va_start(argptr,format);
-vsprintf(str,format,argptr);
+retlen = vsnprintf(str,RAYDIUM_MAX_NAME_LEN - 1, format,argptr);
 va_end(argptr);

+if(retlen < 0) retlen = 0;
+str[retlen] = '\0';
+
 printf("Raydium: %s\n",str);
 if(raydium_log_file) fprintf(raydium_log_file,"%s\n",str);
-raydium_console_line_add(str);
+raydium_console_line_add("%s", str);
 }


I wish I managed to use va_list as I wanted but the way things are done here seems to be the easiest.
As far as I can see, it should fix A) and B) problems.

Comments are welcome.

Next, if this is ok, I will take a look at network.c.

--
guitou


Last edited by guitou on Mon May 15, 2006 8:17 pm, edited 1 time in total.

Top
 Profile  
 
 Post subject:
PostPosted: Fri May 12, 2006 12:07 am 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
It's all OK for me. The "need to be secured" comment of raydium_log() can be removed, too.


Top
 Profile  
 
 Post subject:
PostPosted: Fri May 12, 2006 6:26 am 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Ok, commited in revision 310.

--
guitou


Top
 Profile  
 
PostPosted: Mon May 15, 2006 8:15 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
This patch deals with C) problem. raydium_network_queue_is_tcpid has been modified to be consistent but, in fact, the test tmptype >= 0 may be useless. It depends on how we trust raydium_network_netcall_* vars.

Should they be declared "static" in network.c ? (they seem not to be used anywhere else and I don't see any reference to them in apireference).

Code:
Index: raydium/network.c
===================================================================
--- raydium/network.c   (revision 312)
+++ raydium/network.c   (working copy)
@@ -188,10 +188,13 @@

 signed char raydium_network_queue_is_tcpid(int type)
 {
-int i=0;
+int i=0, tmptype;
 for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
-    if(raydium_network_netcall_type[i]==type && raydium_network_netcall_tcp[i])
+  {
+    tmptype = raydium_network_netcall_type[i];
+    if((tmptype >= 0) && (tmptype == type) && raydium_network_netcall_tcp[i])
        return 1;
+  }
 return 0;
 }

@@ -486,17 +489,20 @@
 void raydium_network_netcall_exec(int type,char *buff)
 {
 char tmpbuff[RAYDIUM_NETWORK_PACKET_SIZE];
-int i;
+int i, tmptype;
 void (*f)(int, char*);

 for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
- if(raydium_network_netcall_type[i]==type)
+{
+ tmptype = raydium_network_netcall_type[i];
+ if((tmptype >= 0) && (tmptype == type) &&
+    (f=raydium_network_netcall_func[i]))
  {
     memcpy(tmpbuff,buff,RAYDIUM_NETWORK_PACKET_SIZE);
-    f=raydium_network_netcall_func[i];
     f(type,tmpbuff);
  }
 }
+}

 signed char raydium_network_timeout_check(void)
 {


Waiting for comments :)


--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 2:23 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
For me the problem is "just" the value -1, which means in facts that a netcall is not used ("filled") yet [1]. So when a packets is received with this type, raydium_network_netcall_exec search this type in raydium_network_netcall_type[] list ... and then finds an empty type (with a false "handler address").

For me, the only fix in raydium_network_netcall_exec() is to change the test for this :
Code:
if(raydium_network_netcall_type[i]==type && type>=0)


Good point with raydium_network_queue_is_tcpid(), but I think that the fix is exactly the same as above. Don't you think ?

[1] raydium_network_init() sample:
Code:
for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
    {
    raydium_network_netcall_type[i]=-1;
    raydium_network_netcall_func[i]=0;
    raydium_network_netcall_tcp[i]=0;
    }


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 2:50 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Ok but if we only have to check if type is >= 0 then we should'nt loop. I mean, if type is < 0, then nothing to do.
The fix will then be reduced to:

Code:
Index: raydium/network.c
===================================================================
--- raydium/network.c   (révision 312)
+++ raydium/network.c   (copie de travail)
@@ -189,7 +189,9 @@
 signed char raydium_network_queue_is_tcpid(int type)
 {
 int i=0;
-for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
+
+if(type >= 0)
+  for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
     if(raydium_network_netcall_type[i]==type && raydium_network_netcall_tcp[i])
        return 1;
 return 0;
@@ -489,13 +491,14 @@
 int i;
 void (*f)(int, char*);

-for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
- if(raydium_network_netcall_type[i]==type)
- {
-    memcpy(tmpbuff,buff,RAYDIUM_NETWORK_PACKET_SIZE);
-    f=raydium_network_netcall_func[i];
-    f(type,tmpbuff);
- }
+if(type >= 0)
+  for(i=0;i<RAYDIUM_NETWORK_MAX_NETCALLS;i++)
+   if(raydium_network_netcall_type[i]==type)
+   {
+     memcpy(tmpbuff,buff,RAYDIUM_NETWORK_PACKET_SIZE);
+     f=raydium_network_netcall_func[i];
+     f(type,tmpbuff);
+   }
 }

 signed char raydium_network_timeout_check(void)


--
guitou


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 3:30 pm 
Offline
User avatar

Joined: Sun Mar 16, 2003 2:53 am
Posts: 2591
Location: gnniiiii (Scrat)
Right, all OK for me with the point C.


Top
 Profile  
 
 Post subject:
PostPosted: Sun May 21, 2006 4:32 pm 
Offline

Joined: Sun Apr 30, 2006 8:55 pm
Posts: 22
Location: Plaisir (et oui, ça fait plaisir)
Alright, commited in revision 315.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 9 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 28 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group