zoo contains exploitable buffer overflows

[Hans de Goede]

Josh Bressers wrote:
>> As the FE zoo maintainer I've applied the security patch suggested on=20
>> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=3D183109
>>
>> I'm not sure the security analysis here is right, but since the patch
>> seems harmless and zoo is exposed to external input via mail filters
>> such as amavisd-new I preferred to err on the side of caution.
> 
> The issue seems to exist.  You can get cause zoo to segfault upon archive
> creation (this is a new and different issue) by creating a very long
> directory path.
> 
> mkdir `perl -e 'print "A"x254'`
> cd `perl -e 'print "A"x254'`
> mkdir `perl -e 'print "A"x254'`
> cd `perl -e 'print "A"x254'`
> touch feh
> cd ../..
> zoo a arch.zoo `perl -e 'print "A"x254 . "/" . "A"x254 . "/feh"'`
> 
> This causes zoo to crash here:
> 
>     void parse (path_st, fname)
>     register struct path_st *path_st;
>     char *fname;
>     {
>        char tempname[LFNAMESIZE];       /* working copy of supplied fname */
>        char *namep;                   /* points to relevant part of tempname */
> 
>        char *p;
>        strcpy (tempname, fname); <== Buffer overflow
> 
> This points out that zoo is a very poorly written program.  Luckily with
> the new changes to gcc and glibc, these horrible stack buffer overflows are
> non issues.  Run the above commands, you'll see on FC5 it doesn't crash,
> libc catches it.
> 
> 
>> If some people could review the alert and the patch I'd be grateful.
>> To my knowledge other distributions have not acted on the alert yet
>> (it's been published on many security lists in the last days).
> 
> The patch attached to the mail (in bugzilla) looks pretty hokey.  I would
> either malloc however much space is needed, or verify the path won't
> overflow the static space.  Adding more space to a static buffer doesn't
> help, it should still be possible to overflow the buffer.  The only good
> way to fix this I can see is modify the combine() function to either accept
> a maximum string length, or malloc the space itself.
> 
> If you pass a length to combine(), you have the issue of uncleanly exiting.
> The code I see has no nice way to report potential errors, which means
> you'll have to exit() inside combine(), which will leave things in an
> unknown state.
> 
> Fixing zoo is probably never going to happen, this is just one of the
> things that is horribly broken by design.  From my quick look through the
> source, it's pretty bad.  There are going to be countless similar problems
> 

Please add this to the relevant bugzilla entry for tracking.

Thanks,

Hans