Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
zanchey opened this issue Aug 10, 2020 · 16 comments · May be fixed by #11258
Open
Assignees
Labels
area-VisualBasic 🪲 bug Product bug (most likely) blocking-migration An issue that is preventing the developer from migrating from .NET Framework or earlier .NET 🚧 work in progress Work that is current in progress tenet-compatibility Incompatibility with previous versions or with WinForms for .NET Framework
Milestone

Comments

@zanchey
Copy link

zanchey commented Aug 10, 2020

  • .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);
    }
}
@weltkante
Copy link
Contributor

weltkante commented Aug 11, 2020

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
Copy link
Author

zanchey commented Aug 11, 2020

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
Copy link
Author

zanchey commented Aug 13, 2020

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

@tbolon
Copy link
Contributor

tbolon commented Aug 14, 2020

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

@zanchey
Copy link
Author

zanchey commented Aug 14, 2020

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
Copy link
Contributor

tbolon commented Aug 14, 2020

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
Copy link
Contributor

weltkante commented Aug 14, 2020

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
Copy link
Author

zanchey commented Aug 15, 2020

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
Copy link
Member

Pinging @cston and @KathleenDollard on this.

@RussKie RussKie added this to the 6.0 milestone Sep 3, 2020
@dreddy-work dreddy-work modified the milestones: 6.0, 7.0 Aug 27, 2021
@dreddy-work dreddy-work modified the milestones: .NET 7.0, .NET 8.0 Aug 15, 2022
@ddddpppp
Copy link

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

@menees
Copy link

menees commented Jan 12, 2023

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
Copy link
Member

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

@JeremyKuhne JeremyKuhne modified the milestones: .NET 8.0, .NET 8.0 RC2 Aug 16, 2023
@KlausLoeffelmann
Copy link
Member

KlausLoeffelmann commented Aug 29, 2023

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

@JeremyKuhne, @merriemcgaw FYI.

@KlausLoeffelmann
Copy link
Member

If we decided to get this addressed, we should address this, too:
#3936

elachlan added a commit to elachlan/winforms that referenced this issue Apr 23, 2024
…r use in TryCreatePipeServer to avoid system wide blocking. Used in conjunction with PipeOptions.CurrentUserOnly
@elachlan elachlan linked a pull request Apr 23, 2024 that will close this issue
@dotnet-policy-service dotnet-policy-service bot added the 🚧 work in progress Work that is current in progress label Apr 23, 2024
@elachlan elachlan added tenet-compatibility Incompatibility with previous versions or with WinForms for .NET Framework blocking-migration An issue that is preventing the developer from migrating from .NET Framework or earlier .NET 🪲 bug Product bug (most likely) labels May 7, 2024
@KlausLoeffelmann KlausLoeffelmann modified the milestones: .NET 9.0, .NET 10.0 Jul 23, 2024
@KlausLoeffelmann
Copy link
Member

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
Copy link
Member

@KlausLoeffelmann the PR: #11258 has mentioned all issues together about WindowsFormsApplicationBase.IsSingleInstance. Do you mean we create a new issue with writing these issues together?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VisualBasic 🪲 bug Product bug (most likely) blocking-migration An issue that is preventing the developer from migrating from .NET Framework or earlier .NET 🚧 work in progress Work that is current in progress tenet-compatibility Incompatibility with previous versions or with WinForms for .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.