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

Yoshiki, I had the same code and was scratching my head over it. I had forgotten that I had originally cloned the Windows logic. However when I was comparing to the unix code I decided to dump that code base and replace it with Ian's rewrite & Bert's modifications.

You might consider reviewing the unix code for isAccessiblePathName & etc so all VM have the same behaviour.

On 25-Aug-09, at 3:30 AM, Yoshiki Ohshima wrote:

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...)

-- = = = ======================================================================== John M. McIntosh johnmci@smalltalkconsulting.com Twitter: squeaker68882 Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com = = = ========================================================================

On 25.08.2009, at 19:36, Yoshiki Ohshima wrote:

At Tue, 25 Aug 2009 10:09:13 -0700, John M McIntosh wrote:

...

Yoshiki, I had the same code and was scratching my head over it. I had forgotten that I had originally cloned the Windows logic. However when I was comparing to the unix code I decided to dump that code base and replace it with Ian's rewrite & Bert's modifications.

You might consider reviewing the unix code for isAccessiblePathName & etc so all VM have the same behaviour.

Thank you, John. Attached is a revised patch, which could be good to get in.

As I wrote, the logic is almost the same as the Unix version; but there is no realpath(), and there is no strong urge to support symlinks. So it only tests if the remaining part contains ".." (not just "..", which could be a part of legitimate file name).

-- Yoshiki <sqWin32Security.c.diff>

In your patch, resourceDirectory defaults to "C:\My Squeak". I think the image directory is a better default? That's what I did for unix anyway.

- Bert -

Hi Yoshiki -

Can you send me the full file instead of a diff? It's easy to diff in SVN but it's not easy to apply a patch on Windows ;-)

Cheers, - Andreas

Yoshiki Ohshima wrote:

---

At Tue, 25 Aug 2009 10:09:13 -0700, John M McIntosh wrote:

...

Yoshiki, I had the same code and was scratching my head over it. I had forgotten that I had originally cloned the Windows logic. However when I was comparing to the unix code I decided to dump that code base and replace it with Ian's rewrite & Bert's modifications.

You might consider reviewing the unix code for isAccessiblePathName & etc so all VM have the same behaviour.

Thank you, John. Attached is a revised patch, which could be good to get in.

As I wrote, the logic is almost the same as the Unix version; but there is no realpath(), and there is no strong urge to support symlinks. So it only tests if the remaining part contains ".." (not just "..", which could be a part of legitimate file name).

-- Yoshiki

On 25.08.2009, at 23:00, Yoshiki Ohshima wrote:

At Tue, 25 Aug 2009 11:46:11 -0700, Andreas Raab wrote:

...

Hi Yoshiki -

Can you send me the full file instead of a diff? It's easy to diff in SVN but it's not easy to apply a patch on Windows ;-)

Ah, I just copied the diff file from my VM compiling machine this morning and now I have trouble to apply it on the Windows machine here. I'll make the default value change Bert suggested anyway. But if somebody can just apply the patch in the meantime so that people can take a look at it before evening, that would be great.

-- Yoshiki

Har har. Here you go.

- Bert -

At Tue, 25 Aug 2009 21:14:46 -0700, Andreas Raab wrote:

Okay it's in. Do you guys need a new VM build for Etoys or do you have your own VM building setup? If you need a new one, do you just want a clone of 3.11.3 with this fix or do you want it updated to the latest VMMaker? Since there is currently no external pressure on these issues it's your pick.

Ah, thank you, Andreas. I just made the default value change like below or attached.

It turned out that our schedule is pushed back a bit and we have some weeks. Getting the other latest stuff like SerialPlugin update and in sync with the new VM version and have it tested with community would be good. So, can you update it to the VMMaker and actually roll a new VM? Thank you!

--------------------------------- *** sqWin32Security.c Tue Aug 25 21:46:02 2009 --- sqWin32Security.c.yoshiki Tue Aug 25 21:45:59 2009 *************** *** 246,252 **** lstrcpy(untrustedUserDirectory, TEXT("C:\My Squeak\%USERNAME%"));

/* establish untrusted user directory */ ! lstrcpy(resourceDirectory, TEXT("C:\My Squeak"));

/* Look up shGetFolderPathW */ shGetFolderPath = (void*)GetProcAddress(LoadLibrary("SHFolder.dll"), --- 246,255 ---- lstrcpy(untrustedUserDirectory, TEXT("C:\My Squeak\%USERNAME%"));

/* establish untrusted user directory */ ! lstrcpy(resourceDirectory, imagePath); ! if (resourceDirectory[lstrlen(resourceDirectory)-1] == '\') { ! resourceDirectory[lstrlen(resourceDirectory)-1] = 0; ! }

/* Look up shGetFolderPathW */ shGetFolderPath = (void*)GetProcAddress(LoadLibrary("SHFolder.dll"), ---------------------------------

-- Yoshiki

On 26.08.2009, at 07:45, Andreas Raab wrote:

Yoshiki Ohshima wrote:

...

It turned out that our schedule is pushed back a bit and we have some weeks. Getting the other latest stuff like SerialPlugin update and in sync with the new VM version and have it tested with community would be good. So, can you update it to the VMMaker and actually roll a new VM? Thank you!

Okay, 3.11.4 is in place now. I had to fix several issues in the serial port code which didn't even compile so I'm certain nobody has tested that code yet. If there is anyone out there who wants to try it, please give it a shot to see if it actually works as advertised. Other than that the VM should work just as well as 3.11.3.

Great! Thanks!

Oh, and I finally remembered to add the version info to the VM.

You mean "Smalltalk vmVersion" does return something sensible? Can I see?

- Bert -