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

Dynamic Dependencies doesn't support Elevation #567

Closed
DrusTheAxe opened this issue Mar 11, 2021 · 2 comments · Fixed by #2066
Closed

Dynamic Dependencies doesn't support Elevation #567

DrusTheAxe opened this issue Mar 11, 2021 · 2 comments · Fixed by #2066

Comments

@DrusTheAxe
Copy link
Member

Dynamic Dependencies relies on PackagedCOM, but PackagedCOM types are not visible to elevated processes. Thus the Dynamic Depednencies and Bootstrapper APIs fail when called by an elevated process.

Elevated processes need to be supported.

DynDep uses of PackagedCOM:

  1. DataStore - DynamicDependencies API uses a PackagedCOM OOP Server to store persisted package dependency definitions in Project Reunion's Main (PRmain) package's ApplicationData if MddCreatePackageDependencyOptions::ScopeIsSystem is not specified (definitions are directly written to a location under Program Data if ScopeIsSystem is specified, and nothing's persisted if MddPackageDependencyLifetimeKind::Process is specified).
  2. LifetimeManager - the LifetimeManager is a PackagedCOM OOP Server to indicate to the Deployment pipeline a specific framework package is in use (and thus do not remove it).

MddCore::DataStore::GetDataStorePathForUser() can be changed to get the path to PRmain's ApplicationData via Windows.ApplicationModel.ApplicationDataManager rather than asking the PackagedCOM if the process is elevated (or possibly in all cases).

The LifetimeManager isn't so easily solved. DynDep needs a process running with PRddlm's identity from the call to MddBootstrapInitialize until MddBootstrapShutdown (or process death). PackagedCOM provides a convenient solution to reliable lifetime management e.g. what if someone doesn't call MddBootstrapShutdown. An alternative mechanism is needed.

One option is a defining the DDLM process as an application with full-trust RuntimeBehavior=packagedClassicApp. ActivateApplication would be needed with a parameter (e.g. process id) so the process can WaitOnSingleObject(OpenProcess(ProcessIdToHandle(args.pid)), INFINITE) to detect if the MddBootstrapInitialize caller terminates without calling MddBootstrapShutdown and self-terminate.

ProcessId isn't a great answer as Windows can reuse them e.g. caller terminates and a new process created between CreateProcess(ddlm) and OpenProcess(ProcesIdToHandle(args.pid)). A better answer is a uniquely named IPC mechanism e.g. CreateNamedPipe(name); CreateProcess(ddlm.exe, cmdline=name);.

It's possible for Deployment to terminate the DDLM process (if a DeploymentOptions 'Force' option is specified). The termination semantics have changed over the years so we'd need to make sure our PackagedCOM replacement provides the expected outcome regardless the Windows version we run on.

It seems plausible an alternative to PackagedCOM can be found. Further investigation into the fine print is needed.

@DrusTheAxe DrusTheAxe self-assigned this Mar 11, 2021
@ghost ghost added the needs-triage label Mar 11, 2021
@BenJKuhn BenJKuhn added this to the 0.8 (2021 Q2) milestone Mar 11, 2021
DrusTheAxe added a commit that referenced this issue Mar 19, 2021
…) APIs if Elevated. Remove when we address Issue #567 #567
DrusTheAxe added a commit that referenced this issue Mar 19, 2021
…) APIs if Elevated (#617)

* Add temporary trap to block calling DynamicDependencies (or Bootstrap) APIs if Elevated. Remove when we address Issue #567 #567

* Incorporated feedback
@DrusTheAxe DrusTheAxe removed this from the 1.0 (2021 Q4) milestone Sep 1, 2021
@shelllet
Copy link

when will this bug be fixed?

@DrusTheAxe
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants