[m-rev.] for review: Implement secure temporary file creation for .NET.
paul at bone.id.au
Mon May 9 12:55:32 AEST 2016
On Mon, May 09, 2016 at 10:10:32AM +0800, Sebastian Godelet wrote:
> Hi Paul,
> Thanks for looking into this.
> I've actually identified a small issue in the patch I send to the mailing list,
> So I've updated my diff, please see https://github.com/Mercury-Language/mercury/pull/40 (with optional .diff|.patch suffix).
> Issue was that the buffer size has to be positive on Windows: File.Create(FileName, 0 -> 512,
> I haven't changed anything else though.
> > -----Original Message-----
> > From: Paul Bone [mailto:paul at bone.id.au]
> > Sent: Monday, May 09, 2016 08:51
> > To: Sebastian Godelet <sebastian.godelet at outlook.com>
> > Cc: 'Mercury Reviews' <reviews at lists.mercurylang.org>
> > Subject: Re: [m-rev.] for review: Implement secure temporary file creation
> > for .NET.
> > On Fri, May 06, 2016 at 05:29:27PM +0800, Sebastian Godelet wrote:
> > > Hi,
> > >
> > > I've applied my previous changes to the do_make_temp predicate for C#.
> > > While going through the C implementation I noticed that the call to
> > > mkstemp is violating the functional contract, namely that XXXXXX needs to
> > be at the end of the string. This seems to work since Suffix is "" in the usual
> > case.
> > > IMHO Suffix just should be removed (as using Suffix != "" will not work at
> > the moment anyway).
> > Someone else suggested this, however once fixed for C (see my comment
> > about mkstemps below) the only unsupported backend is Erlang. Which can
> > probably be fixed easily also.
> Well when I went through Mono source code I noticed that they only seem to support the mkstemp function for all platforms; assuming that I actually understand the Mono library resolving mechanism correctly (they certainly have an implementation if the standard C library doesn't support this). Note that for our purposes, we could still try to use mkstemps, since on Windows that code is not executed anyway (even on Mono). So at the moment I also have left out the use of Suffix for the .NET/Windows implementation (but it would of course be easy to add), alternatively we could try to call mkstemps inside a try catch block and revert to the use of mkstemp in case it doesn't exist. I can test this easily.
Ah, I was thinking of the C on windows case. I'm not sure about C# on
windows, we might have to stick to mkstemp there - but that means it will
not support suffixes.
Maybe we should make open(..., O_CREAT | O_EXCL) / mkdir and their
equivilents in Java C# and Erlang the primative operation, and write the
logic that generates random names and tries them in Mercury, then we don't
need to rely on any support for mkstemp etc. What do you think?
All we need from each target language is open and mkdir with O_CREAT |
O_EXCL behavior that also lets us set the file's permissions atomically. We
could still do it with less but it's be insecure.
> > Although mkstemps is non-standard, it's still a realistic alternative since we
> > already support systems without mkstemp. So we'd continue to support two
> > implementations: generate names ourselves and use open / mkstemps. - I'll
> > make this a separate proposal.
> For the make_temp_file, should there be always two versions? : One that creates a temporary file, and one that creates and opens a tempory file? The implementations of those two could and should be different, currently we create a file and then close the stream, but for example on Windows it is much more efficient to open a stream temporary and let the system delete it when the file handle is closed. Like the O_TMPFILE - Linux only? - flag I mentioned earlier or the FILE_ATTRIBUTE_TEMPORARY the FILE_FLAG_DELETE_ON_CLOSE Windows equivalent (although we use _wopen at the moment instead of CreateFile).
I would like to see that yes. Returning the newly created and opened file
is ideal. What I was referring to however was how on the C backend
conditional compilation means that mkstemp is used when it's available and a
loop with a call to open is used otherwise.
> I would also volunteer to implement the mkdtemp(s) implementation for Windows if you don't mind. It would be very similar to the .NET/Windows make_temp_directory implementation (i.e. use the CreateDirectoryW Win32 call, etc.). After all that is what the .NET API is wrapping in the first place.
Sounds good but let's wait until we decide what the best general approach
More information about the reviews