dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.4k stars 977 forks source link

WindowsFormsApplicationBase.IsSingleInstance applications are global on a machine, not per user #3715

Open zanchey opened 4 years ago

zanchey commented 4 years ago

Problem description: The implementation of WindowsFormsApplicationBase.IsSingleInstance from #3200 uses a named pipe as its synchronization and communication tool, but named pipes are in a global namespace (at least on Windows) and thus any assembly can only be running once on any given machine.

Expected behavior: Able to start this executable in multiple user accounts (under terminal services or fast user switching) at once.

Actual behaviour: Starting the executable in a second user account produces an uncaught CantStartSingleInstanceException exception.

Strictly speaking this is a potential denial of service issue; by using a named pipe with a predictable name, users can prevent others on the same machine from ever starting the application. Adding the session identifier to the pipe name is enough to make the named pipe session-local, but not enough to prevent others from creating a pipe with the same name.

I don't know what other options there are; mutexes and event handles have a session namespace, but don't allow for data transfer. Local TCP runs into trouble with firewalls (as the old Remoting implementation did). Cursory reading shows that named memory mappings are Windows only, and require marshalling/synchronisation to a much greater degree, though perhaps a memory mapping containing only the randomly-generated named pipe name is the way to go (like WCF).

Minimal repro: Borrowed from https://stackoverflow.com/a/19326/125549:

public class SingleInstanceApplication : System.Windows.Application
{
    protected override void OnStartup(System.Windows.StartupEventArgs e)
    {
        // Call the OnStartup event on our base class
        base.OnStartup(e);

        // Create our MainWindow and show it
        MainWindow window = new MainWindow();
        window.Show();
    }

    public void Activate()
    {
        // Reactivate the main window
        MainWindow.Activate();
    }
}

public class SingleInstanceManager : Microsoft.VisualBasic.ApplicationServices.WindowsFormsApplicationBase
{
    private SingleInstanceApplication _application;
    private System.Collections.ObjectModel.ReadOnlyCollection<string> _commandLine;

    public SingleInstanceManager()
    {
        IsSingleInstance = true;
    }

    protected override bool OnStartup(Microsoft.VisualBasic.ApplicationServices.StartupEventArgs eventArgs)
    {
        // First time _application is launched
        _commandLine = eventArgs.CommandLine;
        _application = new SingleInstanceApplication();
        _application.Run();
        return false;
    }

    protected override void OnStartupNextInstance(StartupNextInstanceEventArgs eventArgs)
    {
        // Subsequent launches
        base.OnStartupNextInstance(eventArgs);
        _commandLine = eventArgs.CommandLine;
        _application.Activate();
    }
}

public class EntryPoint
{
    [STAThread]
    public static void Main(string[] args)
    {
        SingleInstanceManager manager = new SingleInstanceManager();
        manager.Run(args);
    }
}
weltkante commented 4 years ago

Can the named pipe be put into logon session namespace?

This doc says

client processes can use the "Local\" prefix to explicitly create an object in their session namespace

And this doc seems to suggest that \\.\pipe\LOCAL prefix is valid for named pipes, at least since Win10/1709.

Might be worth investigating if named pipes can be put into the users logon session namespace on all supported Windows versions by naming them accordingly, instead of reimplementing everything.

zanchey commented 4 years ago

No, the local namespace doesn't apply to named pipes - you just get a pipe called "Local\whatever" in the global namespace. I think the \\.\pipe\LOCAL name is restricted to UWP only, and is just used to enforce a UWP boundary.

zanchey commented 4 years ago

Some additional considerations in https://devblogs.microsoft.com/oldnewthing/20070629-00/?p=26213

tbolon commented 4 years ago

Why not adding the session id in the pipe name to make it unique per session ?

zanchey commented 4 years ago

A named pipe with a predictable name creates a scenario where another user can register the same pipe name and therefore prevent your application from starting. This is a potential security consideration on multi-user servers.

tbolon commented 4 years ago

But it must be a predictable name (based on the entry module id it seems : Entry.ManifestModule.ModuleVersionId.ToString()) to be guessed by another instance. So adding a session id does not reduce the security. Or I miss something ? See also #3296

weltkante commented 4 years ago

Could you retrieve the session ID and reject pipe connections from different sessions? Outright rejecting undesired connections might be better than a random pipe name. Not sure if the pipe handle is available for this though, and whether this session ID is actually the same as the logon session.

zanchey commented 4 years ago

But it must be a predictable name (based on the entry module id it seems : Entry.ManifestModule.ModuleVersionId.ToString()) to be guessed by another instance. So adding a session id does not reduce the security. Or I miss something ? See also #3296

It does need to be a consistent name, but it needs to be consistent in a local namespace - the global namespace is the problem. If your program runs with a pipe named \\.\pipe\somestring-sessionid, I can create some pipes at \\.\pipe\somestring-{0,1,2,3}... in my session, and then you won't be able to start it in your session because you can't have two pipes with the same name on the same system.

I think you can do it with a mutex, a shared memory segment and a pipe, but that's a lot of moving parts. FindWindow and WM_COPYDATA is a possibility too, though it's easy to deadlock.

KlausLoeffelmann commented 4 years ago

Pinging @cston and @KathleenDollard on this.

ddddpppp commented 2 years ago

My .Net 6 build is hitting this issue. Has the issue been progressed or is there a effective workaround?

menees commented 1 year ago

My .NET 7 app just hit this issue. 🙁 Since this won't be fixed until at least .NET 8, I'm going to remove use of WindowsFormsApplicationBase and go with a named mutex approach instead (like this or this).

KlausLoeffelmann commented 1 year ago

@KathleenDollard: I am strongly in favor to get this fixed. Let's talk!

KlausLoeffelmann commented 1 year ago

This is nothing the WinForms team can decide on their own. Bumping this to .NET 9.

@JeremyKuhne, @merriemcgaw FYI.

KlausLoeffelmann commented 1 year ago

If we decided to get this addressed, we should address this, too: https://github.com/dotnet/winforms/issues/3936

KlausLoeffelmann commented 2 months ago

Moving this to .NET 10. (@Olina-Zhang: I think we have at least a couple of these. Let's try to consolidate them into one?)

Olina-Zhang commented 2 months ago

@KlausLoeffelmann the PR: https://github.com/dotnet/winforms/pull/11258 has mentioned all issues together about WindowsFormsApplicationBase.IsSingleInstance. Do you mean we create a new issue with writing these issues together?