Steve Langasek wrote:
> This looks like a pretty serious regression in the latest security NMU of
> f2c.  The code now reads:
> 
> char *c_functions       = "c_functions";
> char *coutput           = "c_output";
> char *initfname         = "raw_data";
> char *initbname         = "raw_data.b";
> char *blkdfname         = "block_data";
> char *p1_file           = "p1_file";
> char *p1_bakfile        = "p1_file.BAK";
> char *sortfname         = "init_file";
> char *proto_fname       = "proto_file";
> 
> [...]
> 
>  void
> set_tmp_names(Void)
> {
> #ifdef MSDOS
> [...]
> #else
>         sprintf(c_functions, "%s/f2c_func_XXXXXX", tmpdir);
>         sprintf(initfname,   "%s/f2c_rc_XXXXXX", tmpdir);
>         sprintf(initbname,   "%s/f2c_rc.b_XXXXXX", tmpdir);
>         sprintf(blkdfname,   "%s/f2c_blkd_XXXXXX", tmpdir);
>         sprintf(p1_file,     "%s/f2c_p1f_XXXXXX", tmpdir);
>         sprintf(p1_bakfile,  "%s/f2c_p1fb_XXXXXX", tmpdir);
>         sprintf(sortfname,   "%s/f2c_sort_XXXXXX", tmpdir);
> #endif
> [...]
> }
> 
> which is an obvious overflow condition.
>
> AFAICT, the initialization of these strings is completely inappropriate, and
> should be replaced with a sensibly-sized buffer, followed by the use of
> snprintf() instead of sprintf().  Would you (or Dan McMahill, if that's
> where this patch came from) care to fix this up, or would you like me to
> prepare a patch?

I have to confess I took this patch direct from DSA-661-2 and did not
really look at it in detail so this initialisation problem escaped me.

Here's the tricky bit -- in the stable version, the code is almost
exactly the same, except the block of code at the top of set_tmp_names
is not in this ifdef:

#ifdef MSDOS
        int k;
        if (debugflag == 1)
                return;
        k = strlen(tmpdir) + 24;
        c_functions = (char *)ckalloc(7*k);
        initfname = c_functions + k;
        initbname = initfname + k;
        blkdfname = initbname + k;
        p1_file = blkdfname + k;
        p1_bakfile = p1_file + k;
        sortfname = p1_bakfile + k;
#else

So I think the original patch and the stable version are ok. Steve's
patch also looks ok, but I haven't checked the possible lengths
closely -- Steve's patch does make an assumption about the length of
tmpdir that the above code does not make.

-- 
see shy jo

Attachment: signature.asc
Description: Digital signature

Reply via email to