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