Bug#497835: gmanedit audit

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#497835: gmanedit audit

Nico Golde
tags 497835 + security
severity 497835 grave
thanks

Hi Steffen,
Ccing the bug report.
* Steffen Joeris <[hidden email]> [2008-09-05 11:51]:
> Looking at #497835 I guess it might not be a bad idea to request a security
> audit for gmanedit. I have added a placeholder temp item to the security
> tracker for now. Can someone request the audit or shortly outline how to do
> it or who to mail?

In theory it is possible to overflow a buffer with a large COMMAND
string in the configuration file (I find it not very likely
that someone sends you a gmanedit configuration file
though):

callbacks.c:
    638         gchar command[50],*datos;
    639         gint exitstatus;
    640
    641 /* I read conf file ~/.gmaneditrc */
    642        
    643         strcpy(temp, "/tmp/gmanedit.XXXXXX");
    644         mkstemp (temp);
    645         datos=ReadConfFromFile("COMMAND");
    646
    647         if (datos==NULL)
    648         {
    ...
    651         }
    652         else
    653         {
    654                 strcpy(command,datos);
    655                 strcat(command," -l ");
    656                 strcat(command,temp);
    657         }      

The buffer that comes from ReadConfFromFile can be bigger than command can take:
    862 static gchar *ReadConfFromFile(gchar *variable)
    863 {
    864   FILE *f;
    865   gchar readed[100];
    866   gchar *home;
    867   gchar *tok;
    868  
    869 // Intento de abrir el fichero con la configuraci√≥n personalizada  
    870   home = getenv("HOME");
    871   strcpy(readed,home);
    872   strcat(readed,"/.gmaneditrc");
    ...
    880   while (fgets(readed,80,f) != NULL)
    881   {
    882 // Lo siguiente quita los retornos de carro de las l√≠neas leidas
    883      if (readed[strlen(readed)-1] == '\n')
    884         readed[strlen(readed)-1] = '\0';
    885        
    886      if ((readed[0] != '#') && (!strncmp(variable,readed,strlen(variable))))
    887      {
    888         tok = strtok(readed,"=");
    889         tok = strtok(NULL,"=");
    890         fclose(f);
    891         return(tok);

In practice this is a bug because the pointer on readed is not valid anymore after
the function was left. It can be still available though
in some cases. It should be fixed definitely.

Similar problems are all over the code, heavy strcpy/strcat usage without
any bounds checking but using self supplied data.

When reading a manpage file there can be a problem if the text is converted to utf8:
callbacks.c:
   1148 static void open_man_file(gchar *manfile)
   ...
   1156         gchar *utf8;
   1157         gchar * buffer = (gchar*)malloc(BUFFER_SIZE);
   1158
   1159
   ....
   1179         if ((f=gzopen((gchar *)manfile,"rb"))!=NULL)
   1180         {
   1181           while(!gzeof(f))
   1182           {
   1183                 bytes_read=gzread(f,buffer,BUFFER_SIZE);
   1184                 if (bytes_read>0)
   1185                 {
   1186                         utf8 = NULL;
   1187                         if (g_utf8_validate(buffer, -1, NULL) == FALSE)
   1188                         {
   1189                                 utf8 = g_locale_to_utf8(buffer, -1, NULL, NULL, NULL);
   1190                         }
   1191                         if (utf8 != NULL)
   1192                                 strncpy(buffer,utf8,strlen(utf8));
   1193                         gtk_text_buffer_insert_at_cursor(tb, buffer ,bytes_read);

This should be an exploitable heap overflow. When converting the manpage buffer from the
current locale to utf8 (line 1189) the resulting buffer can be bigger than
the previous locale buffer. As the strncpy call in line 1192 is useless as it uses strlen(utf8)
as the upper bound it is possible to overflow buffer in some situations.
Because of this I raise the severity and readd the security tag.


The overall code quality is pretty bad from my point of view and this
program should be heavily reworked.

Kind regards
Nico

--
Nico Golde - http://www.ngolde.de - [hidden email] - GPG: 0x73647CFF
For security reasons, all text in this mail is double-rot13 encrypted.

attachment0 (204 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#497835: gmanedit audit

Joop Stakenborg-3
2008/9/5 Nico Golde <[hidden email]>:

>
> The overall code quality is pretty bad from my point of view and this
> program should be heavily reworked.
>

Thanks,

I have never had a serious look at the actual code. My job has been to
port gmanedit to GTK+ version 2, to make sure this software would not
get lost.

I will start on a rework in a few weeks time. If you feel the package
isn't good enough for Lenny then please remove it from testing.

Regards,
Joop pa3aba at debian dot org



--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]