On Sun, Apr 24, 2005 at 10:25:56AM -0400, Joey Hess wrote: > 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
Hmm, curious.
> 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.
Yes, the worst case is that the string is truncated and therefore mkstemp()
fails because you don't have a valid template. That didn't seem like too
bad a problem to me.
Cheers,
--
Steve Langasek
postmodern programmer
signature.asc
Description: Digital signature
