Re: GNU Sharutils buffer overflow vulnerability.

[Dan Yefimov <dan@xxxxxxxxxxxxxxxxxxxxx>]

On Tue, 6 Apr 2004, [iso-8859-1] Shaun Colley wrote:

> I have written a simple patch below to fix the buffer
> overflow bug:
> 
> 
> --- shar-bof.patch ---
> 
> --- shar.1.c    2004-04-06 16:26:55.000000000 +0100
> +++ shar.c      2004-04-06 16:32:32.000000000 +0100
> @@ -1905,7 +1905,7 @@
>         break;
> 
>        case 'o':
> -       strcpy (output_base_name, optarg);
> +       strncpy (output_base_name, optarg,
> sizeof(output_base_name));
>         if (!strchr (output_base_name, '%'))
>           strcat (output_base_name, ".%02d");
>         part_number = 0;
> --- EOF ---
> 
Your patch isn't quite correct since you at least forgot about 
strcat(output_base_name, ".%02d") following patched code. You didn't also
notice subsequent using output_base_name as a format string which may produce 
overflow of output_filename[] because of unnoticed percent symbols passed in. 
Attached a patch accounting for that.
-- 

    Sincerely Your, Dan.

--- src/shar.orig.c     2004-04-07 16:18:23.000000000 +0100
+++ src/shar.c  2004-04-07 16:39:04.000000000 +0100
@@ -212,10 +212,10 @@
 static long first_file_position;
 
 /* Base for output filename.  FIXME: No fix limit in GNU... */
-static char output_base_name[50];
+static char output_base_name[512];
 
 /* Actual output filename.  FIXME: No fix limit in GNU... */
-static char output_filename[50];
+static char output_filename[512];
 
 static char *submitter_address = NULL;
 
@@ -1905,9 +1905,29 @@
        break;
 
       case 'o':
-       strcpy (output_base_name, optarg);
-       if (!strchr (output_base_name, '%'))
-         strcat (output_base_name, ".%02d");
+       /*
+        * Note: the magic '6' below is exactly sizeof(".%02d").
+        * Don't forget to increase size of output_filename[] appropriately
+        * when you increase field width from 2 up to something greater than 4.
+        */
+       {
+               register int i = 0;
+               register char *str = optarg;
+
+               while (i < sizeof(output_base_name) - 6) {
+                       register char c;
+
+                       output_base_name[i++] = (c = *str++);
+                       if (c == '%')
+                               if (i < sizeof(output_base_name) - 6)
+                                       output_base_name[i++] = c;
+                               else {
+                                       i--;
+                                       break;
+                               }
+               }
+               strcpy (output_base_name + i, ".%02d");
+       }
        part_number = 0;
        open_output ();
        break;