<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div></div><div>On Dec 2, 2017, at 11:26 PM, monty <<a href="mailto:monty2@programmer.net">monty2@programmer.net</a>> wrote:</div><div><br></div><blockquote type="cite"><div><blockquote type="cite"><span>Sent: Saturday, November 25, 2017 at 9:46 PM</span><br></blockquote><blockquote type="cite"><span>From: "tim Rowledge" <<a href="mailto:tim@rowledge.org">tim@rowledge.org</a>></span><br></blockquote><blockquote type="cite"><span>To: "The general-purpose Squeak developers list" <<a href="mailto:squeak-dev@lists.squeakfoundation.org">squeak-dev@lists.squeakfoundation.org</a>></span><br></blockquote><blockquote type="cite"><span>Subject: [squeak-dev] #newFileNamed: has curious and ancient flaw</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>… 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? </span><br></blockquote><span></span><br><span>... 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.</span><br></div></blockquote><div><br></div>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 (<a href="https://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html">https://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html</a>).  Start with section 7.11.1.1 if you like to skim.<div><br></div><div>If I reviewed <span style="background-color: rgba(255, 255, 255, 0);">StandardFileStream</span> 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.<div><br><div><blockquote type="cite"><div><span></span><span>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, </span></div></blockquote><div><br></div>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.<br><br><blockquote type="cite"><div><span>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.</span><br></div></blockquote><div><br></div>From a security perspective, I like this solution even better.  Developer visibility into errors is almost always helpful.</div><div><br>— Brenda</div></div></div></body></html>