Smalltalk › Squeak › Squeak - Dev

#newFileNamed: has curious and ancient flaw

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

5 messages Options

timrowledge

#newFileNamed: has curious and ancient flaw

… and another thing - take a look at StandardFileStream class>>#newFileNamed:. Note that it carefully checks to see if a file of the requested name already exists? Sounds very sensible, even though the message is not the best name I could imagein for the job.

Unfortunately, if you take a look at how it is implemented you’ll notice FileDirectory>fileExists: which again sounds very reasonable. I’d argue that it is not appropriate for an instance method on a particular directory to be worrying about other directories (this is one place where #asFileName is not ridiculous) but never mind - the important part here is that it carefully makes sure to check we only check files and not directories. Which is of course sensible because we are checking if a *file* exists.

But going back up the stack we notice that this means our test for a pre-existing use of the requested file name will not detect a *directory* of that name. How many filing systems can support a directory containing a file and another directory of the same name? Obviously this can’t in practice be a huge issue since I’ve never seen any mention of it causing a problem but it is a stupid hole in the semantics. Just another one to add to the list...

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Oyster (n.), a person who sprinkles his conversation with Yiddishisms.

monty-3

Re: #newFileNamed: has curious and ancient flaw

> Sent: Saturday, November 25, 2017 at 9:46 PM
> From: "tim Rowledge" <[hidden email]>
> To: "The general-purpose Squeak developers list" <[hidden email]>
> Subject: [squeak-dev] #newFileNamed: has curious and ancient flaw
>
> … and another thing - take a look at StandardFileStream class>>#newFileNamed:. Note that it carefully checks to see if a file of the requested name already exists? Sounds very sensible

Does it? Checking to see if a file exists first and then opening it for writing/creation after is an obvious race condition, which could result in data loss. The open and existence check need to be done together atomically, failing when the file already exists. This is the type of little thing where if we don't get it right, we look amateur.

We have sqFileOpenNew() in newer VMs (it's unused in the image though), but it uses a boolean flag parameter to distinguish between failure caused by the file existing and all other causes, and I'd rather have a set of FilePlugin error codes, which could be mapped to file exceptions in the image, to provide better info for why a file operation failed, and also to fix certain file methods I've seen that just assume a particular reason for a failure.

Eliot was in the middle of a house move, so I'll remind him about this: http://forum.world.st/Custom-module-primitive-error-codes-td4966882.html

> , even though the message is not the best name I could imagein for the job.
> Unfortunately, if you take a look at how it is implemented you’ll notice FileDirectory>fileExists: which again sounds very reasonable. I’d argue that it is not appropriate for an instance method on a particular directory to be worrying about other directories (this is one place where #asFileName is not ridiculous) but never mind - the important part here is that it carefully makes sure to check we only check files and not directories. Which is of course sensible because we are checking if a *file* exists.
>
> But going back up the stack we notice that this means our test for a pre-existing use of the requested file name will not detect a *directory* of that name. How many filing systems can support a directory containing a file and another directory of the same name? Obviously this can’t in practice be a huge issue since I’ve never seen any mention of it causing a problem but it is a stupid hole in the semantics. Just another one to add to the list...
>
>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> Oyster (n.), a person who sprinkles his conversation with Yiddishisms.
>
>
>
>

Brenda Larcom

Re: #newFileNamed: has curious and ancient flaw

On Dec 2, 2017, at 11:26 PM, monty <[hidden email]> wrote:

Sent: Saturday, November 25, 2017 at 9:46 PM
From: "tim Rowledge" <[hidden email]>
To: "The general-purpose Squeak developers list" <[hidden email]>
Subject: [squeak-dev] #newFileNamed: has curious and ancient flaw

… and another thing - take a look at StandardFileStream class>>#newFileNamed:. Note that it carefully checks to see if a file of the requested name already exists?

... Checking to see if a file exists first and then opening it for writing/creation after is an obvious race condition, which could result in data loss. The open and existence check need to be done together atomically, failing when the file already exists. This is the type of little thing where if we don't get it right, we look amateur.

This race condition in particular can result in exploitable security issues as well as accidental data loss issues. One classic reference on the topic (since at least the mid-1990s when I got into secure development professionally) is David Wheeler’s Secure Programming HOWTO (https://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html). Start with section 7.11.1.1 if you like to skim.

If I reviewed StandardFileStream for a client, I would expect this issue to get at least a high severity. Based on what Tim & Monty said above & below (it may be yet more months before I can check an image or VM myself), using this method without creating security holes would be quite challenging.

We have sqFileOpenNew() in newer VMs (it's unused in the image though), but it uses a boolean flag parameter to distinguish between failure caused by the file existing and all other causes,

This sounds like an incomplete past attempt to fix this (or a related) security hole. Starting to use this primitive instead could be a good short-term fix.

and I'd rather have a set of FilePlugin error codes, which could be mapped to file exceptions in the image, to provide better info for why a file operation failed, and also to fix certain file methods I've seen that just assume a particular reason for a failure.

From a security perspective, I like this solution even better. Developer visibility into errors is almost always helpful.

— Brenda

monty-3

Re: #newFileNamed: has curious and ancient flaw

You're the only other person to show concern over this. I wrote sqFileOpenNew() because of this (and fixed some race conditions in sqFileOpen() in the process). Here're the implementations:

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/3f49cd2f9a4833ea8e7d43e958bd216784204183/platforms/Cross/plugins/FilePlugin/sqFilePluginBasicPrims.c#L395

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/66133d2224585cff9a1342b1e36962a9adf12ab8/platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c#L175

But I'd like to get the error codes working first, so we only need to support one version of the primitive in the image, and fallback on the broken Squeak code for older VMs that lack it.

Sent: Sunday, December 03, 2017 at 8:28 AM
From: "Brenda Larcom" <[hidden email]>
To: "The general-purpose Squeak developers list" <[hidden email]>
Subject: Re: [squeak-dev] #newFileNamed: has curious and ancient flaw

On Dec 2, 2017, at 11:26 PM, monty <[hidden email]> wrote:

Sent: Saturday, November 25, 2017 at 9:46 PM

From: "tim Rowledge" <[hidden email]>

To: "The general-purpose Squeak developers list" <[hidden email]>

Subject: [squeak-dev] #newFileNamed: has curious and ancient flaw

… and another thing - take a look at StandardFileStream class>>#newFileNamed:. Note that it carefully checks to see if a file of the requested name already exists?

... Checking to see if a file exists first and then opening it for writing/creation after is an obvious race condition, which could result in data loss. The open and existence check need to be done together atomically, failing when the file already exists. This is the type of little thing where if we don't get it right, we look amateur.

We have sqFileOpenNew() in newer VMs (it's unused in the image though), but it uses a boolean flag parameter to distinguish between failure caused by the file existing and all other causes,

This sounds like an incomplete past attempt to fix this (or a related) security hole. Starting to use this primitive instead could be a good short-term fix.

and I'd rather have a set of FilePlugin error codes, which could be mapped to file exceptions in the image, to provide better info for why a file operation failed, and also to fix certain file methods I've seen that just assume a particular reason for a failure.

From a security perspective, I like this solution even better. Developer visibility into errors is almost always helpful.

— Brenda

Eliot Miranda-2

Re: #newFileNamed: has curious and ancient flaw

On Thu, Dec 7, 2017 at 1:12 AM, monty <[hidden email]> wrote:

You're the only other person to show concern over this. I wrote sqFileOpenNew() because of this (and fixed some race conditions in sqFileOpen() in the process). Here're the implementations:

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/3f49cd2f9a4833ea8e7d43e958bd216784204183/platforms/Cross/plugins/FilePlugin/sqFilePluginBasicPrims.c#L395

https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/66133d2224585cff9a1342b1e36962a9adf12ab8/platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c#L175

But I'd like to get the error codes working first, so we only need to support one version of the primitive in the image, and fallback on the broken Squeak code for older VMs that lack it.

I'm wrestling with git as I type. The support for error codes is in the VMMaker source. It'll be in the git repository soon, and in the Squeak base image soon too. I have meetings planned for this morning but should have it done before Monday.

Sent: Sunday, December 03, 2017 at 8:28 AM
From: "Brenda Larcom" <[hidden email]>
To: "The general-purpose Squeak developers list" <[hidden email]>
Subject: Re: [squeak-dev] #newFileNamed: has curious and ancient flaw

On Dec 2, 2017, at 11:26 PM, monty <[hidden email]> wrote:

Sent: Saturday, November 25, 2017 at 9:46 PM

From: "tim Rowledge" <[hidden email]>

To: "The general-purpose Squeak developers list" <[hidden email]>

Subject: [squeak-dev] #newFileNamed: has curious and ancient flaw

… and another thing - take a look at StandardFileStream class>>#newFileNamed:. Note that it carefully checks to see if a file of the requested name already exists?

... Checking to see if a file exists first and then opening it for writing/creation after is an obvious race condition, which could result in data loss. The open and existence check need to be done together atomically, failing when the file already exists. This is the type of little thing where if we don't get it right, we look amateur.

This race condition in particular can result in exploitable security issues as well as accidental data loss issues. One classic reference on the topic (since at least the mid-1990s when I got into secure development professionally) is David Wheeler’s Secure Programming HOWTO (https://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html). Start with section 7.11.1.1 if you like to skim.

If I reviewed StandardFileStream for a client, I would expect this issue to get at least a high severity. Based on what Tim & Monty said above & below (it may be yet more months before I can check an image or VM myself), using this method without creating security holes would be quite challenging.

We have sqFileOpenNew() in newer VMs (it's unused in the image though), but it uses a boolean flag parameter to distinguish between failure caused by the file existing and all other causes,

This sounds like an incomplete past attempt to fix this (or a related) security hole. Starting to use this primitive instead could be a good short-term fix.

and I'd rather have a set of FilePlugin error codes, which could be mapped to file exceptions in the image, to provide better info for why a file operation failed, and also to fix certain file methods I've seen that just assume a particular reason for a failure.

From a security perspective, I like this solution even better. Developer visibility into errors is almost always helpful.

— Brenda

_,,,^..^,,,_

best, Eliot