Bugzilla Bug 65773

hi,
attached is a patch, that fixes 2 buffer overflows in app-arch/sharutils:

the first one was discovered by ulf harnhammar;
see:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=265904
for details.
while looking through the code, i discovered one more overflow:
try this:
bash-2.05# unshar `perl -e 'print "A"x1500'`/tmp/testing
unshar: /home/flo/AA[A*x]AAA/tmp/testingSegmentation fault (core dumped)
attached patch fixes all two problems.

best regards
florian [rootshell]

Created an attachment (id=40686) [details]
... the patch

Mike you did one of the previous security fixes please do it again.

Also this package lacks metadata.xml.

(From update of attachment 40686 [details])
>diff -Naur ./sharutils-4.2.1/src/shar.c ./sharutils-4.2.1_new/src/shar.c
>--- ./sharutils-4.2.1/src/shar.c	1999-09-10 21:20:41.000000000 +0200
>+++ ./sharutils-4.2.1_new/src/shar.c	2004-09-29 09:06:06.782294248 +0200
>@@ -1571,7 +1571,7 @@
> 	  sprintf (command, "%s '%s'", CHARACTER_COUNT_COMMAND, local_name);
> 	  if (pfp = popen (command, "r"), pfp)
> 	    {
>-	      char wc[BUFSIZ];
>+		char wc[BUFSIZ], tempform[50];      
> 	      const char *prefix = "";
> 
> 	      if (did_md5)
>@@ -1579,8 +1579,8 @@
> 		  fputs ("  else\n", output);
> 		  prefix = "  ";
> 		}
>-
>-	      fscanf (pfp, "%s", wc);
>+	sprintf (tempform, "%%%ds", BUFSIZ - 1);
>+	fscanf (pfp, tempform, wc);
> 	      fprintf (output, "\
> %s  shar_count=\"`%s '%s'`\"\n\
> %s  test %s -eq \"$shar_count\" ||\n\
>diff -Naur ./sharutils-4.2.1/src/unshar.c ./sharutils-4.2.1_new/src/unshar.c
>--- ./sharutils-4.2.1/src/unshar.c	1995-11-21 17:22:14.000000000 +0100
>+++ ./sharutils-4.2.1_new/src/unshar.c	2004-09-29 09:06:12.216468128 +0200
>@@ -346,8 +346,8 @@
> {
>   size_t size_read;
>   FILE *file;
>-  char name_buffer[NAME_BUFFER_SIZE];
>-  char copy_buffer[NAME_BUFFER_SIZE];
>+  char name_buffer[NAME_BUFFER_SIZE] = {'\0'};
>+  char copy_buffer[NAME_BUFFER_SIZE] = {'\0'};
>   int optchar;
> 
>   program_name = argv[0];
>@@ -409,14 +409,15 @@
>   if (optind < argc)
>     for (; optind < argc; optind++)
>       {
>-	if (argv[optind][0] == '/')
>-	  stpcpy (name_buffer, argv[optind]);
>-	else
>-	  {
>-	    char *cp = stpcpy (name_buffer, current_directory);
>-	    *cp++ = '/';
>-	    stpcpy (cp, argv[optind]);
>-	  }
>+	if (argv[optind][0] == '/') {
>+		strncpy (name_buffer, argv[optind], sizeof(name_buffer));
>+		name_buffer[sizeof(name_buffer)-1] = '\0';
>+	}
>+	else {

>>> snprintf(name_buffer, sizeof(name_buffer),"%s/%s", current_directory, argv[optind]);
>>> name_buffer[sizeof(name_buffer)-1] = '\0';

>+	}
> 	if (file = fopen (name_buffer, "r"), !file)
> 	  error (EXIT_FAILURE, errno, name_buffer);
> 	unarchive_shar_file (name_buffer, file);

please post an additional attachment instead of trying to edit it ... bugzilla
mangles the patch horribly otherwise :/

Created an attachment (id=40702) [details]
the revised (working !!!) patch - sorry, wrote the first one while sleeping, i
guess... ;-)

updated patch looks sane to me; added to 4.2.1-r10

Arches, please mark 4.2.1-r10 stable

-r10 wont build here.. it asks for
app-arch/sharutils/files/sharutils-4.2.1-r6-gentoo.diff which doesnt exist..
should it point to sharutils-4.2.1-gentoo.patch or is it something else you
forgot to commit ?

Note: 
There are far more than just 2 problem areas in sharutils. When I last reviewed it I saw some many problem areas that I just gave up and said this thing needs a complete rewrite. So if you guys are going to do a GLSA I _really_ suggest you look quite a bit closer at the code. Also due to the nature of sharutils it's probably best to work with upstream on this package.

app-arch/sharutils/files/sharutils-4.2.1-r6-gentoo.diff is deleted by portage
emerge sync
...
deleting app-arch/sharutils/files/sharutils-4.2.1-r6-gentoo.diff

vapier renamed the patch without fixing -r10.. I fixed it.. Holding off a
little before marking it x86... Can anyone look at the other problems solar
mentionned ?

Hmm dunno what to do about this package. It has no Gentoo maintainer, it has no
clear upstream maintainer, and it's full of various flaws.

Like solar said, it's not the first time we patch something in there (see bug
46998) and it needs a thorough audit. But unless someone volunteers to audit
this package and coordinate fixes with upstream (see bug 46998 for pointers),
it's probably better to patch vulnerabilities as they are found.

last upstream release was in 1999

to me that says this package is dead upstream (just like so many GNU utils)

maybe someday, out of the blue, a maintainer will step up ... but for now, none exist

so what do we do now? test it and mark it stable or take it out of cvs?

why would we take it out of cvs ?

marked amd64/arm/hppa/ia64/s390 stable

ppc stable now too

sparc stable then...

Stable on x86 too then...

Stable on alpha.

*** Bug 65846 has been marked as a duplicate of this bug. ***

What depends on sharutils?

jaervosz asked me to review the GLSA draft, but frankly given solar's comment I would prefer pulling it out entirely.  Is this feasible?

If that's not a good option, for whatever reason, I'll go ahead and review, but someone (possi/probably me, if I have the time) really should conduct a more thorough security audit.

this package isnt going anywhere, stop thinking about removing it

it's one of those old school standard utilities ... shar archives arent as popular anymore, but go back in time and they were pretty heavily used

GLSA 200410-01

Stable on mips.

stable on ppc64, thanks!

There is a 4.3.77 version out now, that fixes lots of buffer overflows. Perhaps
you could package that?

I don't think that version is 100% perfect either. I'll do a thorough audit of
it, when I get the time.

// Ulf Harnhammar
   Debian Security Audit Project