This is the mail archive of the insight@sourceware.org mailing list for the Insight project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: Possible security flaw in Insight


Ben Hutchings wrote:
> This security advisory explains a bug in some versions of Tcl, which
> may affect Insight.
> 
> Ben.
> 
> readdir_r considered harmful
> ============================


  Well, readdir_r is used in tcl/unix/tclUnixThrd.c as follows:

--------------------------------snip--------------------------------
typedef struct ThreadSpecificData {
    char	    	nabuf[16];
    struct tm   	gtbuf;
    struct tm   	ltbuf;
    struct {
	Tcl_DirEntry ent;
	char name[PATH_MAX+1];
    } rdbuf;
} ThreadSpecificData;

Tcl_DirEntry *
TclpReaddir(DIR * dir)
{
    Tcl_DirEntry *ent;
#ifdef TCL_THREADS
    ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey);

#ifdef HAVE_READDIR_R
    ent = &tsdPtr->rdbuf.ent; 
    if (TclOSreaddir_r(dir, ent, &ent) != 0) {
	ent = NULL;
    }
--------------------------------snip--------------------------------
[  In tclUnixPort.h we have

#ifdef HAVE_STRUCT_DIRENT64
typedef struct dirent64	Tcl_DirEntry;
#   define TclOSreaddir		readdir64
#   define TclOSreaddir_r	readdir64_r
#else
typedef struct dirent	Tcl_DirEntry;
#   define TclOSreaddir		readdir
#   define TclOSreaddir_r	readdir_r
#endif

]
--------------------------------snip--------------------------------

  Since PATH_MAX+1 is more than long enough for the longest path possible, and
we only need enough space for a single pathname component, which is at most
NAME_MAX+1[*], I don't see any way this could overflow.  In fact, we could
safely reduce the size of that buffer and save some memory, assuming that it's
not used for other purposes as well (which I have _not_ verified).

> Recommendations
> ---------------

> Suggested code for calculating the required buffer size for readdir_r
> follows.


  I'm with Zaraza (sp?) on this one.  What's wrong with statically sizing it
to NAME_MAX+1, in accordance with the demands of the posix spec?


http://opengroup.org/onlinepubs/009695399/functions/readdir_r.html

" The thread-safe version of the directory reading function returns values in
a user-supplied buffer instead of possibly using a static data area that may
be overwritten by each call. Either the {NAME_MAX} compile-time constant or
the corresponding pathconf() option can be used to determine the maximum sizes
of returned pathnames. "

    cheers,
      DaveK

[*]  http://opengroup.org/onlinepubs/009695399/basedefs/limits.h.html:
PATH_MAX must include +1 for the terminating NUL already.  NAME_MAX does not.
-- 
Can't think of a witty .sigline today....


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]