winforms icon indicating copy to clipboard operation
winforms copied to clipboard

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

Open zanchey opened this issue 5 years ago • 14 comments

  • .NET Core Version: v5.0.0-preview.7

  • Have you experienced this same bug with .NET Framework?: No

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);
    }
}

zanchey avatar Aug 10 '20 14:08 zanchey

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.

weltkante avatar Aug 11 '20 12:08 weltkante

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 avatar Aug 11 '20 13:08 zanchey

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

zanchey avatar Aug 13 '20 14:08 zanchey

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

tbolon avatar Aug 14 '20 07:08 tbolon

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.

zanchey avatar Aug 14 '20 13:08 zanchey

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

tbolon avatar Aug 14 '20 14:08 tbolon

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.

weltkante avatar Aug 14 '20 14:08 weltkante

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.

zanchey avatar Aug 15 '20 13:08 zanchey

Pinging @cston and @KathleenDollard on this.

KlausLoeffelmann avatar Aug 20 '20 18:08 KlausLoeffelmann

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

ddddpppp avatar Aug 30 '22 09:08 ddddpppp

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).

menees avatar Jan 12 '23 15:01 menees

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

KlausLoeffelmann avatar Jan 12 '23 16:01 KlausLoeffelmann

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

@JeremyKuhne, @merriemcgaw FYI.

KlausLoeffelmann avatar Aug 29 '23 01:08 KlausLoeffelmann

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

KlausLoeffelmann avatar Aug 29 '23 22:08 KlausLoeffelmann