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