Re: GNU Sharutils buffer overflow vulnerability.
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;