[Vm-dev] Resource Directory support for Windows (VM 4.2.1Beta Window version ?)

Yoshiki Ohshima yoshiki at vpri.org
Tue Aug 25 10:30:54 UTC 2009


At Mon, 24 Aug 2009 12:41:33 +0200,
Bert Freudenberg wrote:
> 
> <nag>except for resource directory support in the security plugin</ 
> nag> ;)

  Well, so I ended up volunteering.  I don't claim that attached patch
is complete (it is working for me but security patch rewrite shouldn't
be done over night), but more reviewing and testing is really
appreciated.

  Actually, the sqWin32Security.c looks somewhat strange.  The
beginning of isAccessiblePathName() looks:

------------------------
static int isAccessiblePathName(TCHAR *pathName) {
  int i;
  /* Check if the path/file name is subdirectory of the image path */
  for(i=0; i<lstrlen(untrustedUserDirectory)-1; i++)
    if(untrustedUserDirectory[i] != pathName[i]) return 0;
------------------------

but this doesn't check the length of pathName so it can do
out-of-bounds read-access for pathName.  (it returns as soon as it
sees a char that is not part of the legitimate path , but still...)

and then it reads:
-----------
 /* special check for the trusted directory */
  if(pathName[i] == 0) return 1; /* allow access to trusted directory */
-----------

The comment is wrong, I suppose.  Not sure what is the special check
here.

And then:
-----------
 /* check last character in image path (e.g., backslash) */
  if(untrustedUserDirectory[i] != pathName[i]) return 0;
-----------
 
This, along with other lines, is a way to test the two strings are
equal.  But is the comment right?  The last character is not typically
backslash in our usual settings.

  For isAccessibleFileName(), if the function only tests the path part
anyway, it can be the same implementation to isAccessiblePathName(),
right?

expandMyDocuments() is declared as:
-----------
char *expandMyDocuments(char *pathname, char *replacement, char *result)
-----------

but the last line of the function reads:
-----------
  return strlen(result);
-----------
the caller expects the length so it should be int or size_t.

Anyway, here is my patch.  The logic is largely borrowed from the Unix
version of it by Bert.  Please review it and let us know what you
think.

-- Yoshiki
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sqWin32Security.c.diff
Type: application/octet-stream
Size: 7542 bytes
Desc: not available
Url : http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20090825/1568beff/sqWin32Security.c.obj


More information about the Vm-dev mailing list