[wix-devs] Burn support for WM_SETTINGSCHANGED to refresh environment variables

[Sean Hall]

I don't want to acknowledge that the user environment variables in the
elevated process is something we should worry about. That would require
more mitigation actions than we are currently doing such as purging the
user environment variables from the process. If Windows includes user
environment variables in the process when created from ShellExecute with
"runas" then I don't think there's anything wrong with us updating them.

This feature was always about reading the new values from the registry. The
WM_SETTINGCHANGE message with the "Environment" parameter contains no
information except that the sender of that message thought that *something*
changed in environment variables.

I find it interesting that you're starting to talk yourself into declining
your own feature request. If we are deciding when to check the registry,
then we are effectively declining the issue since it was requesting we
update the environment whenever WM_SETTINGCHANGE is received.

On Thu, Jul 7, 2022 at 10:09 AM Rob Mensching <rob at firegiant.com> wrote:

> I was thinking about this a bit more and I think the elevated process
> should only be updated with changes to the system environment changes.
> Otherwise, if (for some reason) a per-machine package in the bundle used an
> environment variable for a "secure" purposes, a user environment variable
> change could sneak into the per-machine execution.
>
> I think that means this feature boils down to using the Environment
> registry key to update environment variables in the currently executing
> Burn process. Right?
>
> If so, I like the proposal of having a list of "runtime updatable"
> Environment variables defined in the bundle. We could also exclude a list
> of "important" environments variables (but that may be a slippery slope to
> maintain).
>
> I think the "list of runtime updatable Environment variables" also helps
> mitigate the issues of env var override behavior. If an Environment
> variable is specified to be updatable, it'll be updated during the
> execution of the bundle (overwriting any specially crafted environment
> block when the Burn process was started).
>
> The "list of runtime updatable Environment variables" could also be
> specified per-package to further narrow down the impact of changes. This
> would really narrow how often the Environment registry key is checked.
>
>
> -----Original Message-----
> From: wix-devs <wix-devs-bounces at lists.wixtoolset.org> On Behalf Of Sean
> Hall via wix-devs
> Sent: Monday, June 27, 2022 11:36 AM
> To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> Cc: Sean Hall <r.sean.hall at gmail.com>
> Subject: Re: [wix-devs] Burn support for WM_SETTINGSCHANGED to refresh
> environment variables
>
> I don't have a concrete scenario off the top of my head. It's the way
> environment variables are supposed to work. System environment variables
> are the defaults. User environment variables are layered on top, overriding
> system environment variables if they are defined in both places (with the
> exception of a small number of path variables that get merged instead). New
> process environment blocks are supposed to be created with those defaults
> and then the process can override them if it wants to. If the process
> modified an environment variable then in general that modified value
> shouldn't be overwritten if the default changed.
>
> On Mon, Jun 27, 2022 at 12:45 PM Rob Mensching <rob at firegiant.com> wrote:
>
> > > For example, let's say `MYVAR` has a default value of `1` but in the
> > current process `MYVAR` has a local value of `2`. After receiving the
> > `WM_SETTINGCHANGE` message, the default value is now `3`. There are
> > going to be some cases where the local value of `2` needs to continue
> > to override the default, while in other cases the new default of `3`
> > is supposed to overwrite the local value.
> >
> > I was trying to think of situations where Burn should not accept a
> > WM_SETTINGCHANGE update to its environment but wasn't sure what would
> > be considered "bad". Do you have a concrete scenario where this is a
> problem?
> >
> >
> > -----Original Message-----
> > From: wix-devs <wix-devs-bounces at lists.wixtoolset.org> On Behalf Of
> > Sean Hall via wix-devs
> > Sent: Friday, June 24, 2022 2:08 PM
> > To: WiX Toolset Developer Mailing List <wix-devs at lists.wixtoolset.org>
> > Cc: Sean Hall <r.sean.hall at gmail.com>
> > Subject: Re: [wix-devs] Burn support for WM_SETTINGSCHANGED to refresh
> > environment variables
> >
> > This came across my radar when figuring out my options for working
> > around https://github.com/dotnet/runtime/issues/68180. There are
> > various other projects that have looked into keeping the environment
> > block of the current process up to date:
> >
> > https://github.com/chocolatey/choco/blob/f924d47fb4177a9a34ff0c2bf9959
> > 38b5c12800b/src/chocolatey.resources/redirects/RefreshEnv.cmd
> > ,
> > https://github.com/Maximus5/ConEmu/issues/468,
> > https://github.com/microsoft/terminal/issues/1125,
> > https://github.com/microsoft/terminal/issues/9741. There's basically
> > two options - throw away the current environment block and get a fresh
> > one or merge the changes that were made with the current one.
> >
> > The first option of getting a fresh one has a fatal flaw - there's no
> > documented way to get a fresh one with all the variables that would
> > normally be set when launching from explorer. ::CreateEnvironmentBlock
> > is the documented way to create a fresh one but it doesn't include
> everything.
> > If you get this wrong then parts of Windows might not work correctly,
> > for example [The case of the SHGet­Folder­Path(CSIDL_COMMON_DOCUMENTS)
> > that returned ERROR_PATH_NOT_FOUND](
> > https://devblogs.microsoft.com/oldnewthing/20200520-00/?p=103775).
> > There is an undocumented shell32 function `RegenerateUserEnvironment`
> > that is explained at
> https://textslashplain.com/2018/10/11/shellexecute-doesnt/.
> >
> > The other problem with the fresh option is that it's probably not the
> > right thing to do. The user may have set an environment variable that
> > it wanted to propagate for the entire lifetime of the process but this
> > will undo it.
> >
> > The second option of merging the changes has the same issue. For
> > example, let's say `MYVAR` has a default value of `1` but in the
> > current process `MYVAR` has a local value of `2`. After receiving the
> > `WM_SETTINGCHANGE` message, the default value is now `3`. There are
> > going to be some cases where the local value of `2` needs to continue
> > to override the default, while in other cases the new default of `3`
> > is supposed to overwrite the local value.
> >
> > There is theoretically a third option here - add an attribute to
> > executable packages to make Burn provide a fresh environment block to
> > ::CreateProcessW. Of course, this has the same problem as the first
> > option in that there's no documented way to get a fresh environment
> > block. And it would mean that the executable would never get
> > environment variables from the Burn process, though that could be
> > mitigated if users could explicitly pass certain environment variables
> > (by implementing https://github.com/wixtoolset/issues/issues/2697).
> > Note that this only would help executable packages and not `MsiPackage`
> or `MspPackage`.
> >
> > A more targeted approach might be to add the ability to specify a
> > specific environment variable that gets overwritten in the current
> > process by the default after a package has run.
> >
> ____________________________________________________________________
> WiX Toolset Developer Mailing List provided by FireGiant
> http://www.firegiant.com/
>