Ticket #345 (Invalid)Sat May 25 18:13:05 UTC 2013

Potential DOSFS buffer overrun when handling long filenames?

Reported by:	Rick Murray (539)	Severity:	Normal
Part:	RISC OS: Module	Release:
Milestone:		Status	Invalid

Details by Rick Murray (539):

Referring to the source file <a href=“https://www.riscosopen.org/viewer/view/castle/RiscOS/Sources/FileSys/ImageFS/DOSFS/c/DOSnaming?rev=4.7;content-type=text%2Fplain”>DOSnaming</a>.

There are two functions of note. Here is the first:

<pre><code>/-————————————————————————————————————/
/* Check that the given DOS names are identical */

int namematch(char *wcname,char *fname)
{
char string1²⁵⁷;
char string2²⁵⁷ ;

/* Code assumes characters upto (and including) “file_sep” will always fit

• in RISC OS names.
  /
  before(string1,fname,file_sep,0) ;
  before(string2,wcname,file_sep,0) ;
  if (wild_card_compare(string1,string2,DOSwcmult,DOSwcsing) == TRUE)
  {
  / “string1” is the full (non-wildcarded) filename we have matched with /
  / “string2” is the original (wildcarded) filename we were given on entry */

after(string1,fname,file_sep,0) ; after(string2,wcname,file_sep,0) ; if (wild_card_compare(string1,string2,DOSwcmult,DOSwcsing) == TRUE) return(TRUE) ; } return(FALSE) ;

}</code></pre>

and here is the second:

<pre><code>/-———————————————————————————————————/
/* return a string containing the text before the given character */

char *before(char *newptr,char *text,char marker,int npad)
{
int cpos = chr_pos(text,marker) ;
if (cpos == 0)
cpos = strlen(text) ;

strncpy(newptr,text,cpos) ; if (npad == 0) newptr[cpos] = NULL ; return(newptr) ;

}</code></pre>

What comes to mind is what happens if the filename is <i>longer</i> than 256 characters before the separator? The string1 and string2 chars are only large enough for 256, and the before() function will, if there isn’t a separator, default to the length of the text string (or if there is, to whatever size that is including >256).
In short, passing a stupid-long filename to DOSFS will splatter gooey bits of gunk all over places it shouldn’t.

To fix, not difficult, something like:

Make the filename buffers appropriately long (might be a FAT issue?) and include a check in before() to ensure that this buffer is not overshot.

There is a routine called after() that may need to be checked as well.