dotnet/runtime

API Proposal: Port `MemoryMappedFileSecurity` and add extensions for `MemoryMappedFile`

JeremyKuhne opened this issue · 14 comments

JeremyKuhne commented

Summary

We did not port System.IO.MemoryMappedFiles.MemoryMappedFileSecurity to Core as it wasn't cross-plat. There is still a need to set security on memory mapped files for Windows, just as there is for files/directories/etc. We should port these to System.IO.FileSystem.AccessControl and add extension methods for MemoryMappedFile there so Windows specific codebases can be ported and retain their security model.

Proposal

Port the following types from .NET Framework to Core and add them to System.IO.FileSystem.AccessControl:

namespace System.IO.MemoryMappedFiles
{
    // See enum's full definition in .NET Framework
    [Flags]
    public enum MemoryMappedFileRights;

    public class MemoryMappedFileSecurity : ObjectSecurity<MemoryMappedFileRights> 
    {
        public MemoryMappedFileSecurity();
    }
}

Add the following extension methods for MemoryMappedFile to System.IO.FileSystemAclExtensions:

namespace System.IO
{
    public static class FileSystemAclExtensions
    {
        public static MemoryMappedFile CreateFromFile(
            this MemoryMappedFileSecurity memoryMappedFileSecurity,
            FileStream fileStream,
            string mapName,
            long capacity,
            MemoryMappedFileAccess access,
            HandleInheritability inheritability,
            bool leaveOpen);

        public static MemoryMappedFile CreateNew(
            this MemoryMappedFileSecurity memoryMappedFileSecurity,
            string mapName,
            long capacity,
            MemoryMappedFileAccess access,
            MemoryMappedFileOptions options,
            HandleInheritability inheritability);

        public static MemoryMappedFile CreateOrOpenMappedFile(
            this MemoryMappedFileSecurity memoryMappedFileSecurity,
            string mapName,
            long capacity,
            MemoryMappedFileAccess access,
            MemoryMappedFileOptions options,
            HandleInheritability inheritability);

        public static MemoryMappedFileSecurity GetAccessControl(
            this MemoryMappedFile memoryMappedFile);

        public static void SetAccessControl(
            this MemoryMappedFile memoryMappedFile,
            MemoryMappedFileSecurity memoryMappedFileSecurity);
    }
}

Details

We could create a new extensions class, but this still can logically fit with "file system". Same goes for the assembly- there isn't enough value in creating a new assembly for this.

Creating a non-extension static class is also possible, but I'd rather try to come up with a more generic security model that can abstract platform differences and add a more x-plat friendly model. Hanging platform specific helpers off of platform specific types contains this somewhat.

I didn't make more descriptive names as these match exactly with what is missing on MemoryMappedFile. Theory is it helps discovery.

Note that the implementation of MemoryMappedFileSecurity is trivial, it adds nothing to ObjectSecurity<T> other than the T. Implementing the extension methods is a little more complicated- we may want to try and factor out a more generic set of CreateFile helpers that take NativeObjectSecurity to reduce duplication.

Related Issues

dotnet/corefx#29546 MemoryMappedFile is missing CreateFromFile overload that takes MemoryMappedFileSecurity
dotnet/corefx#41614 API Proposal: Add file and directory creation methods that take an ACL

CC: @danmosemsft, @ericstj, @terrajobst

terrajobst commented

Video

We should introduce a new static type in the same assembly as FileSystemAclExtensions. The pattern we settled on was suffixing the type with Acl.

namespace System.IO.MemoryMappedFiles
{
    public static class MemoryMappedFileAcl
    {
        public static MemoryMappedFile CreateFromFile(
            FileStream fileStream,
            string mapName,
            long capacity,
            MemoryMappedFileAccess access,
            MemoryMappedFileSecurity memoryMappedFileSecurity,
            HandleInheritability inheritability,
            bool leaveOpen);

        public static MemoryMappedFile CreateNew(
            string mapName,
            long capacity,
            MemoryMappedFileAccess access,
            MemoryMappedFileOptions options,
            MemoryMappedFileSecurity memoryMappedFileSecurity,
            HandleInheritability inheritability);

        public static MemoryMappedFile CreateOrOpen(
            string mapName,
            long capacity,
            MemoryMappedFileAccess access,
            MemoryMappedFileOptions options,
            MemoryMappedFileSecurity memoryMappedFileSecurity,
            HandleInheritability inheritability);

        // Those are fine as extensions:

        public static MemoryMappedFileSecurity GetAccessControl(
            this MemoryMappedFile memoryMappedFile);

        public static void SetAccessControl(
            this MemoryMappedFile memoryMappedFile,
            MemoryMappedFileSecurity memoryMappedFileSecurity);
    }
}
carlossanlop commented

@terrajobst your comment with the approved APIs does not mention adding the MemoryMappedFileSecurity class, which does not exist in Core and needs to be ported from Framework.

Edit: From an email conversation, Immo confirmed that existing Framework APIs do not need to be mentioned in the API approval to get ported.

carlossanlop commented

@terrajobst @bartonjs @stephentoub @GrabYourPitchforks @JeremyKuhne @jkotas ,

We need to add this API to this proposal and make it public:

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

Line 18 in 4f9ae42

private MemoryMappedFile(SafeMemoryMappedFileHandle handle)

As you can see in the current PR discussion, we cannot create the MemoryMappedFile object with the currently available public surface area. The only option we have is to create the object via reflection, by calling the private constructor that receives the handle, which is inefficient.

We cannot call the private constructor because the ACL APIs live in a different assembly (System.IO.FileSystem.AccessControl) than where MemoryMappedFiles lives (System.IO.MemoryMappedFiles).

For some context, the other new ACL APIs did not have this problem, because:

  • For FileInfo and DirectoryInfo, we were working within the same assembly System.IO, so we were able to call the necessary internal methods that lived in FileSystem or in FileStream.
  • For EventWaitHandle, Mutex and Semaphore, those clases exposed their internal safe handle via a property getter, so we were able to create a handle with the proper security, then create the synchronization object, then switch the old handle with the secured handle.
  • For AnonymousPipeServerStream, we added a new internal constructor that received the security object, and we were able to call it because the new APIs were added within the same assembly.
  • 👍2
carlossanlop commented

@terrajobst @bartonjs @stephentoub @GrabYourPitchforks @JeremyKuhne @jkotas ,

We also need to include two new classes to the API proposal, they were not mentioned in the main description. They did not exist in Framework, the concept is new in Core. All the ACL changes we have already fixed had similar classes: an AccessRule and an AuditRule. We need them for MemoryMappedFiles too:

namespace System.IO
{
    public sealed class MemoryMappedFileAccessRule : System.Security.AccessControl.AccessRule
    {
        public MemoryMappedFileAccessRule(System.Security.Principal.IdentityReference identity, System.IO.MemoryMappedFiles.MemoryMappedFileRights eventRights, System.Security.AccessControl.AccessControlType type) : base(default(System.Security.Principal.IdentityReference), default(int), default(bool), default(System.Security.AccessControl.InheritanceFlags), default(System.Security.AccessControl.PropagationFlags), default(System.Security.AccessControl.AccessControlType)) { }
        public MemoryMappedFileAccessRule(string identity, System.IO.MemoryMappedFiles.MemoryMappedFileRights eventRights, System.Security.AccessControl.AccessControlType type) : base(default(System.Security.Principal.IdentityReference), default(int), default(bool), default(System.Security.AccessControl.InheritanceFlags), default(System.Security.AccessControl.PropagationFlags), default(System.Security.AccessControl.AccessControlType)) { }

        public MemoryMappedFileAccessRule(System.Security.Principal.IdentityReference identity, int accessMask, bool isInherited, System.Security.AccessControl.InheritanceFlags inheritanceFlags, System.Security.AccessControl.PropagationFlags propagationFlags, System.Security.AccessControl.AccessControlType type) : base(default(System.Security.Principal.IdentityReference), default(int), default(bool), default(System.Security.AccessControl.InheritanceFlags), default(System.Security.AccessControl.PropagationFlags), default(System.Security.AccessControl.AccessControlType)) { }
        public System.IO.MemoryMappedFiles.MemoryMappedFileRights MemoryMappedFileRights { get; }
    }
    public sealed class MemoryMappedFileAuditRule : System.Security.AccessControl.AuditRule
    {
        public MemoryMappedFileAuditRule(System.Security.Principal.IdentityReference identity, System.IO.MemoryMappedFiles.MemoryMappedFileRights eventRights, System.Security.AccessControl.AuditFlags flags) : this(default(System.Security.Principal.IdentityReference), default(int), default(bool), default(System.Security.AccessControl.InheritanceFlags), default(System.Security.AccessControl.PropagationFlags), default(System.Security.AccessControl.AuditFlags)) { }
        public MemoryMappedFileAuditRule(System.Security.Principal.IdentityReference identity, int accessMask, bool isInherited, System.Security.AccessControl.InheritanceFlags inheritanceFlags, System.Security.AccessControl.PropagationFlags propagationFlags, System.Security.AccessControl.AuditFlags auditFlags) : base(default(System.Security.Principal.IdentityReference), default(int), default(bool), default(System.Security.AccessControl.InheritanceFlags), default(System.Security.AccessControl.PropagationFlags), default(System.Security.AccessControl.AuditFlags)) { }
        public System.IO.MemoryMappedFiles.MemoryMappedFileRights MemoryMappedFileRights { get; }
    }
}
JeremyKuhne commented

We also need to include two new classes to the API proposal

Why do we need these? Can you provide more context/links?

carlossanlop commented

You can see in my PR that the new MemoryMappedFileSecurity class needs to define two properties: AccessRuleType and AuditRuleType.

All the previous ACL changes already had Security/AuditRule/AccessRule classes defined.

The AuditRule and AccessRule objects allow setting the rights and the access control of the security object. For example, in the MutexSecurity PR, I added unit tests that verify the correct values for these objects.

terrajobst commented

@carlossanlop

  • Adding the constructor is fine; it should be documented to transfer ownership to the MemoryMappedFile instance (like FileStream does)
namespace System.IO.MemoryMappedFiles
{
    public partial class MemoryMappedFile
    {
        public MemoryMappedFile(SafeMemoryMappedFileHandle handle);
    }
}
  • It's unclear why we need these new types. If we're porting a .NET Framework feature, we should port the existing .NET Framework types.
namespace System.IO
{
    public sealed class MemoryMappedFileAccessRule : AccessRule
    {
        public MemoryMappedFileAccessRule(IdentityReference identity, MemoryMappedFileRights eventRights, AccessControlType type)
            : base(default(IdentityReference), default(int), default(bool), default(InheritanceFlags), default(PropagationFlags), default(AccessControlType));
        public MemoryMappedFileAccessRule(string identity, MemoryMappedFileRights eventRights, AccessControlType type)
            : base(default(IdentityReference), default(int), default(bool), default(InheritanceFlags), default(PropagationFlags), default(AccessControlType));
        public MemoryMappedFileAccessRule(IdentityReference identity, int accessMask, bool isInherited, InheritanceFlags inheritanceFlags, PropagationFlags propagationFlags, AccessControlType type)
            : base(default(IdentityReference), default(int), default(bool), default(InheritanceFlags), default(PropagationFlags), default(AccessControlType));
        public MemoryMappedFileRights MemoryMappedFileRights { get; }
    }
    public sealed class MemoryMappedFileAuditRule : AuditRule
    {
        public MemoryMappedFileAuditRule(IdentityReference identity, MemoryMappedFileRights eventRights, AuditFlags flags)
            : this(default(IdentityReference), default(int), default(bool), default(InheritanceFlags), default(PropagationFlags), default(AuditFlags));
        public MemoryMappedFileAuditRule(IdentityReference identity, int accessMask, bool isInherited, InheritanceFlags inheritanceFlags, PropagationFlags propagationFlags, AuditFlags auditFlags)
            : base(default(IdentityReference), default(int), default(bool), default(InheritanceFlags), default(PropagationFlags), default(AuditFlags));
        public MemoryMappedFileRights MemoryMappedFileRights { get; }
    }
}
  • 👍1
carlossanlop commented

Update: After a deeper investigation, I concluded I do not need to add the MemoryMappedFileAccessRule and MemoryMappedFileAuditRule. Those don't exist in Framework thanks to the fact that MemoryMappedFileSecurity inherits from System.Security.AccessControl.ObjectSecurity<MemoryMappedFileRights>, so there is no need to implement methods and properties that depend on specialized audit/access rule types.

As I mentioned in this comment, in order to avoid using reflection to call private factory methods, we would have to make MemoryMappedFile constructors public and duplicate some existing functionality from the MemoryMappedFile factory methods.

I will need to request making this constructor public, which receives three arguments:

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

Line 28 in 4f9ae42

private MemoryMappedFile(SafeMemoryMappedFileHandle handle, FileStream fileStream, bool leaveOpen)

It's needed so that the new MemoryMappedFileAcl.CreateFromFile method that receives an ACL can create a MemoryMappedFile instance by passing the handle, the file stream and the option to leave the underlying handle open. It's currently used in the existing method MemoryMappedFile.CreateFromFile which we need to duplicate in the new Acl class:

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

Line 189 in 4f9ae42

return new MemoryMappedFile(handle, fileStream, false);

Just for completeness, the other constructor which we already approved (which receives only a handle), is used in MemoryMappedFile.CreateNew and MemoryMappedFile.CreateOrOpen (and their functionality had to be duplicated too):

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

Line 312 in 4f9ae42

return new MemoryMappedFile(handle);

runtime/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs

Line 381 in 4f9ae42

return new MemoryMappedFile(handle);

GrabYourPitchforks commented

Review notes - we need to know a little more about the scenario. In particular, is the only reason the FileStream is accepted in the ctor is so that there's something to dispose at the end of the operation? Can we instead dispose the FileStream and the memmapped handle at the same time through some clever ref counting mechanism?

See if there's perhaps a way to enable this scenario without exposing this particular ctor.

carlossanlop commented

We are closing the PR. Will reopen once there is a customer request for this.

JonLuke007 commented

I feel being able to set access permissions for MemoryMappedFiles (shared memory) is needed in ".NET5". I'm currently using MemoryMappedFiles to communicate with my Blazor Server (using CreateOrOpen). In ".NET Framework 4.8" I simply pass the MemoryMappedFileSecurity parameter giving my Blazor Server access to my shared memory. Can we add this capability to ".NET5-windows" please? Any advice? (btw I need the high performance of shared memory)

  • 👍1
adamsitnik commented

The need for this feature was asked internally within MS. To unblock the customer, I've copy-pasted the old code from .NET Framework, polished it a bit and tested with custom security descriptors. You can find it here.

carlossanlop commented

Reopening. Besides the external request in the comment above, there's also an additional internal request, so it's worth pursuing.

carlossanlop commented

The original attempt to porting the APIS was in this PR #465 . Instead of attempting to bring the missing additional constructor #33206 , we can follow the suggestions provided by @jkotas in this comment: #465 (comment) .